PR Review — fix: dynamic return

Decision: Request Changes

The core approach is sound — repurposing X15 as a header register so that BR/BR_IF/BR_TABLE can now compile even when the assembler's virtual stack is non-empty is a coherent fix. The new asm.Value type and multi-index Signature are well-motivated. However, there are blocking gaps that need to be addressed before merge.

🔴 Blocking

1. PR description is empty

All three sections (Changes Made / Related Issues / Additional Information) are blank. This is a non-trivial ABI redesign touching 17 files. Without knowing what the original bug was, it is not possible to verify the fix is complete or correct.

Minimal fix: Describe the bug (what broke when BR carried live stack values), link the tracking issue if one exists, and briefly explain the approach (X15 as in/out header register, dynamic return-site encoding via c.ret()).

2. CI is still in progress

The Check ubuntu-24.04 @ Go 1.25 run had not completed at review time. Please wait for it to go green before merge.

3. No integration test for the primary fix

The guards that previously caused BR / BR_IF / BR_TABLE to bail out when the stack was non-empty have been removed:

// REMOVED from BR, BR_IF, BR_TABLE:
if len(c.assembler.Returns()) > 0 { ... return false, false }

The new TestCaller_CallUsesUpdatedHeader and TestAssembler_SignatureIndexes tests verify the low-level ABI mechanism, but no test at the interp/ level exercises a JIT-compiled function where one of these branch instructions fires with a live value on the stack. That is the scenario this PR exists to fix, and it has no end-to-end coverage.

Minimal fix: Add a JIT interpreter test that runs a function containing a conditional branch with a live value on the stack and verifies the returned value is correct.

4. c.ip-2 magic number in boxI64 call

// interp/jit_arm64.go
- boxed = c.boxI64(r0)
+ boxed = c.boxI64(r0, c.ip-2)

It is unclear whether -2 represents bytes already consumed for this specific instruction, an opcode width, or something else. If it points to the wrong PC, the interpreter re-enters at the wrong instruction when the slow path fires.

Minimal fix: Add a one-line comment explaining why -2 is correct here, or derive fallbackIP from a named variable saved before consuming the instruction bytes.

🟡 Important

5. Returns(idx) has a hidden mutation side-effect

// asm/assembler.go
func (a *Assembler) Returns(idx int) []VReg {
    if idx == a.Index() {
        regs := append([]VReg(nil), a.stack...)
        a.returns[idx] = regs   // ← mutation on every call
        return regs
    }
    ...
}

Calling Returns(Index()) to inspect the stack also registers a return site. Any future caller that only wants to observe the stack will silently create an entry in a.returns, which affects signature() and assign(). The existing call in linkable() happens to be safe because linkable() is only called after instructions have been emitted (so Index() has already advanced past any prior return sites), but this is fragile.

Recommendation: Split or rename — e.g., keep Returns(idx) as a pure read and introduce a separate RecordReturn(idx) for the mutation, or at minimum make the side-effect explicit in the method name/doc.

6. X15 header contract is implicit and unenforced

The comment in caller.go states:

Any JIT callee must write R15 before returning to provide the output header.

A callee that misses c.ret() (or a non-JIT callee) will silently use the initial header (sig.Returns(sig.Entry)) and return wrong types or counts with no error signal.

Recommendation: Document this as an explicit ABI requirement near the trampoline, and consider a debug-mode assertion (e.g., checking the returned header's nReturns is within the expected range).

💡 Suggestions

• Signature.Entry is always 0 and is never explicitly set by signature(). Either make it a named constant or document it as reserved for future multi-entry support.
• Saving startIP := c.ip before consuming instruction bytes (a common pattern) would make fallback IPs self-documenting throughout jit_arm64.go, avoiding the need for magic offsets like -2.
• The removal of the Korean comment in reg.go is unrelated to the fix — fine to keep, but a small scope stretch.

Readiness checklist

Minimal path to merge

1. Fill in the PR description with bug context and a linked issue.
2. Wait for CI to pass.
3. Add an interp/-level integration test for BR with a non-empty stack.
4. Explain or rename c.ip-2 in the boxI64 call site.
5. Document or refactor Returns(idx) to make the mutation side-effect explicit.

Generated by Claude Code