[Plan D Task 111b] Sync ABI threading — terminal_out as trailing param

[boldfield]

Summary

Second of 4 PRs implementing Task 111 (TLS → caller-owned terminal channel). Brian-authorized 4-PR breakdown 2026-05-03.

ABI change. Every Sync user fn signature gains a trailing terminal_out: *mut TerminalResult parameter:

Sync ABI: (closure_ptr, user_params..., terminal_out) -> ret_ty

The pointer is threaded end-to-end through Sync → Sync calls, into the main shim's stack-allocated TerminalResult, and forwarded into every sigil_run_loop call site that originates from a Sync user fn. TLS dual-write (added in 111a) remains in place at runtime; pointer-side becomes authoritative for Sync ABI propagation. 111c will thread terminal_out through the Cps + synth fn ABIs; 111d switches handle-exit reads to the caller-owned pointer and removes the TLS path.

Codegen sites updated

• Sync ABI signature emit (UserFnAbi::Sync arm + Sync shim emit): +1 trailing pointer_ty AbiParam in both the declaration-time signature and the body-emit signature.
• Lowerer struct: new terminal_out_param: Option<Value> field. Some(value) for Sync user fn body emit (block-param-bound at fn entry). None for Cps body emit, synth-arm-fn emit, synth-cont emit, post-arm-k synth fn emit, chained-let-bind synth-cont emit (8 sites total) — those contexts pass null until 111c.
• Main shim: allocates a 16-byte stack slot (matching the runtime's repr(C) { value: u64, tag: u64 } layout) and passes its pointer to user_main as the trailing Sync ABI arg.
• lower_call's Sync direct-call branch: threads self.terminal_out_param (or null) as the trailing arg.
• lower_call's Cps interop wrapper: forwards self.terminal_out_param (or null) into sigil_run_loop's out slot.
• Expr::ClosureRecord callee branch (IIFE / lambda-as-value): looks up callee ABI; threads terminal_out for Sync callees, keeps the 3-arg shape for Cps callees (111c plumbs Cps).
• Indirect call (call_indirect via closure record's code_ptr): fn-typed values resolve to Sync ABI callees (hoisted user fn or Sync shim), so the indirect signature gets +1 pointer_ty AbiParam and the call args get +1 trailing pointer.
• Per-Cps-fn Sync shim body emit: reads the trailing block param as terminal_out_v and forwards it into sigil_run_loop instead of null.
• Five sigil_run_loop call sites inside Lowerer methods (handle body's nested run_loop, pre-Phase-4g return-arm dispatch, perform-side run_loop drive, branched k-call dispatch, Slice B fallback): switch from null_terminal_out to self.terminal_out_param-or-null.

Test plan

• cargo build --workspace — clean.
• cargo test --workspace — 274 passed; 3 failed (pre-existing perf timing tests fib_perf / fib_cps_perf / tree_example whose debug-build wall-clock under parallel test-runner contention exceeds the floor; all three fail identically on pre-111b state, verified via git stash + re-run).
• cargo clippy --workspace --all-targets — clean.
• cargo fmt --all -- --check — clean.

🤖 Generated with Claude Code

[boldfield]

Code Review — Plan D Task 111b

Reviewed @ HEAD (6143fd7). Net assessment: structurally sound, ABI threading is symmetric across all changed call sites, alignment/size for the main-shim slot is correct. Six concrete issues, ordered by severity.

1. Silent ABI fallback at Expr::ClosureRecord is a footgun (medium)

compiler/src/codegen.rs:18502-18506:

let callee_abi = self
    .user_fns
    .get(code_fn_name)
    .map(|e| e.abi)
    .unwrap_or(UserFnAbi::Sync);

The unwrap_or(UserFnAbi::Sync) silently defaults to Sync when code_fn_name is missing. Two lines up (18488), user_fn_refs.get(code_fn_name) uses unreachable!() on miss. user_fn_refs is built directly from user_fns (line 20760), so on the happy path the keys always agree and the fallback is dead. Today.

But if a future refactor ever puts a key in one map and not the other, the Sync default silently emits a (closure_ptr, args..., terminal_out) call against a Cps callee whose actual ABI is (closure_ptr, args_ptr, args_len). That is a stack-corrupting ABI mismatch with no debug-build trip-wire — it would surface as a hard-to-trace runtime crash or, worse, miscomputed results.

Fix: mirror the unreachable!() discipline used at the direct-Sync branch (compiler/src/codegen.rs:17613-17619):

let callee_abi = match self.user_fns.get(code_fn_name) {
    Some(e) => e.abi,
    None => unreachable!(
        "codegen: ClosureRecord code_fn_name `{code_fn_name}` registered in \
         user_fn_refs but missing from user_fns — pre-pass invariant violated"
    ),
};

The unreachable!() at 14245 documents the same map-pair invariant explicitly. Be consistent.

2. Sync shim body has implicit length precondition (low, trivial fix)

compiler/src/codegen.rs:14524-14530:

let block_params: Vec<Value> = builder.block_params(entry_block).to_vec();
let closure_ptr_v = block_params[0];
// block_params layout: [closure_ptr, user_params..., terminal_out].
// The Sync ABI signature emit guarantees length >= 2.
let terminal_out_idx = block_params.len() - 1;
let terminal_out_v = block_params[terminal_out_idx];
let user_args: Vec<Value> = block_params[1..terminal_out_idx].to_vec();

The comment is load-bearing. Add a debug_assert!(block_params.len() >= 2, "...") so the invariant is checked, not asserted via prose. Same for the analogous indexing at compiler/src/codegen.rs:9733:

let terminal_out_param = block_params[f.params.len() + 1];

This also assumes the trailing block param exists. A debug-mode bounds check is cheap insurance against a future refactor that drops the trailing param from one signature site but not the others.

3. No test exercises the new threading semantically (medium)

This is +187 lines of ABI-shape change spanning ~13 codegen sites with zero new tests. The existing suite (passing per PR description) verifies we did not break the runtime, but the runtime still treats TLS as authoritative — the pointer-side write in sigil_run_loop is a no-op for correctness right now. So a call site that passes garbage instead of terminal_out (e.g., uninitialized stack slot, wrong block param index, mis-arity'd indirect-call sig) would not produce an observable failure until 111d flips the load-bearing path.

That defers all ABI-threading bug discovery from this PR to 111d, where the failure mode will be: "111d is broken because of something 111b shipped." Hard to root-cause through the merged 111b → 111d delta.

Recommendation: add one targeted test that observes the threading. The cheapest version is a debug-only counter pair (non-null-out invocations, null-out invocations) in sigil_run_loop plus an assertion that for a known-shape program, the non-null/null ratio matches the predicted call graph. A heavier alternative: add a debug assert in sigil_run_loop that when both out is non-null and TLS write fires, the values being written agree (catches "wrong slot threaded"). Either reduces 111d's blast radius dramatically.

If you intentionally chose to defer all observable testing to 111d, please call that out in the PR description and the task tracker so future reviewers do not look for tests that aren't there.

4. Indirect-call signature unconditionally Sync — load-bearing comment, no enforcement (low)

compiler/src/codegen.rs:18597-18624:

The signature appends terminal_out always. The justification is that closure-record code_ptr slots only ever store Sync ABI entries (either a hoisted Sync user fn or a Sync shim wrapping a Cps user fn) — see lower_closure_record at compiler/src/codegen.rs:18901-18907.

That invariant is correct today, but it crosses two functions and is enforced only by reviewer attention. A future change that stores a Cps-ABI func_addr directly in a closure record's code_ptr (bypassing the shim) would silently break every indirect call site without any test or assertion catching it.

Fix: add a debug_assert! in lower_closure_record immediately before the store(code_ptr, closure_ptr, 8) that confirms the resolved code_fn_ref came from either sync_shim_refs or a Sync-ABI entry in user_fns. Costs nothing in release builds; saves the indirect-call site from being silently mis-typed.

5. Comment duplication across the 5 sigil_run_loop sites (low, ergonomic)

The same boilerplate (// Plan D Task 111b — forward caller's terminal_out (Sync) or null (Cps/synth, until 111c).) plus the identical unwrap_or_else snippet appears at five sites: 15211-15222, 15585-15596, 15983-15994, 16952-16963, 17328-17338, 17515-17522, 17749-17761. Plus three more in lower_call's Sync-direct (17634-17640) and Sync→Cps wrapper (17762-17768) and Expr::ClosureRecord (18510-18514) and indirect-call (18630-18632). That's ~10 instances of the same 5-line pattern.

A trivial method on Lowerer deduplicates it and centralizes the rationale:

/// Plan D Task 111b — yield `terminal_out` for forwarding into a
/// Sync-ABI call or `sigil_run_loop`. Returns the threaded pointer
/// when the surrounding fn is a Sync user fn, otherwise null (Cps /
/// synth-cont contexts; 111c threads them too).
fn terminal_out_or_null(&mut self) -> Value {
    self.terminal_out_param
        .unwrap_or_else(|| self.builder.ins().iconst(self.pointer_ty, 0))
}

Replaces ~30 lines with ~10. When 111c lands and the None branch goes away, you delete one method instead of editing ten sites.

6. Stack slot in main shim — correct, but uninitialized (informational)

compiler/src/codegen.rs:10020-10024:

let terminal_slot =
    builder.create_sized_stack_slot(StackSlotData::new(StackSlotKind::ExplicitSlot, 16, 3));
let terminal_out_v = builder.ins().stack_addr(pointer_ty, terminal_slot, 0);

Size 16 ✓ (matches repr(C) { value: u64, tag: u64 }). align_shift = 3 → 8-byte alignment ✓ (matches align_of::<TerminalResult>() == 8 and the runtime's debug_assert! at runtime/src/handlers.rs:1833-1837).

The slot is uninitialized when passed to user_main. That is fine only because sigil_run_loop always writes the full 16 bytes before any read, AND the shim never reads the slot back. Both conditions hold today. Worth a short comment in the shim noting "slot is write-only from main()'s perspective" so a future maintainer doesn't add a read-back path that picks up garbage.

---

Summary

Blocking: none. The PR is mergeable.

Should-fix before merge: issue 1 (silent ABI fallback) — replace with unreachable!() to match sibling code's discipline.

Should-fix this PR or 111c: issue 3 (no semantic test for threading). At minimum, add a sentence to the PR body acknowledging that ABI-threading verification is deferred to 111d.

Nice-to-have: issues 2, 4, 5, 6 — all defensive/ergonomic.

The terminal_out_param: None discipline at the 8 synth/lifted sites is correctly identified and consistent with the multi-PR rollout plan. The runtime's null-tolerance contract (per runtime/src/handlers.rs:1804-1814) makes the transitional state safe.

[boldfield]

R1 review fixes pushed in 531f783. All 6 issues addressed in one commit.

#	Severity	Status	Resolution
1	medium	✅ fixed	unwrap_or(Sync) → match+unreachable!() mirroring sibling code's discipline at the direct-Sync branch
2	low	✅ fixed	debug_assert! added at both Sync shim body emit and Sync user fn body emit (length / f.params.len()+2 invariants)
3	medium	✅ fixed	sigil_run_loop reads *out back after writing at DONE + DISCHARGED terminals and asserts equality with LAST_TERMINAL_* TLS — documents the dual-write invariant during the 111a→111d transition
4	low	✅ fixed	debug_assert! in lower_closure_record confirms the resolved code_fn_ref is Sync ABI (sync_shim_refs entry OR UserFnAbi::Sync in user_fns)
5	low	✅ fixed	New Lowerer::terminal_out_or_null helper; 10 call sites collapse to single-line invocations
6	info	✅ fixed	Comment added documenting "slot is write-only from main()'s perspective"

Net diff: +149 / -71 across compiler/src/codegen.rs + runtime/src/handlers.rs.

Verification: cargo build --workspace, cargo clippy --workspace --all-targets, cargo fmt --all -- --check all clean. cargo test --workspace shows 274 pass / 3 fail (the 3 pre-existing perf timing tests fib_perf / fib_cps_perf / tree_example under parallel test-runner contention — identical failure shape pre- and post-111b). CI re-running on 531f783.