ssa/wasm: use selectors for Rund and Reth continuations - #2208
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
93eb05e to
37a88f0
Compare
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: wasm static defer continuation dispatch
The change is well-scoped and correct in shape: wasm-specific dispatch is cleanly isolated behind storeRunDefersTarget / jumpRunDefersTarget, and the non-wasm path is unchanged (still IndirectJump on a block address). Switching from indirectbr to an index-based switch is the right choice for wasm, which has no computed-goto. I confirmed the panic path is consistent: index 0 routes to nexts[0] == rethrowBlk. Performance: no concerns (dispatch build is O(n), same as before, and runs once per endDefer). Security: no regression — the unreachable default matches the prior indirectbr semantics on an out-of-range value, and the index round-trips at a consistent uintptr width, so no truncation/wrong-case is possible.
Findings are inline. The most important is that the test does not actually exercise the wasm pointer width.
No blocking issues; event is COMMENT (non-approving).
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
62e773d to
96b82b9
Compare
cpunion
changed the title
There was a problem hiding this comment.
Review summary
This PR cleanly splits defer/panic continuation dispatch into two encodings: native keeps blockaddress/indirectbr, while wasm uses a dense integer selector lowered to an LLVM switch (wasm has no blockaddress). I verified the selector index bookkeeping is correct and dense in both endDefer (Reth: 0→procBlk, 1→rethrow, 2..n→intermediates) and the incremental RunDefers/initDeferState path (Rund grows monotonically). The n==0 edge case is safe, the unreachable default is exhaustive-by-construction, and applyFlagsFile is transactional (commits next only on success) and well tested. Comments and the new littest CHECK directives accurately match the code.
Findings below are minor (maintainability, a small test gap, and two low-severity codegen smells). No correctness, security, or performance regressions.
Test gap (not inline): TestDeferContinuationDispatch only exercises DeferAlways. The loopDeferDrainer resume path (ssa/eh.go:470) stores a resume selector to rethPtr and relies on the same wasm dense-index bookkeeping, but no test combines DeferInLoop with a wasm target to assert the switch cases for the loop-drain resume. Since the index bookkeeping is the trickiest part of this change, a wasm loop-defer dispatch assertion would meaningfully reduce risk. (test/defer_test.go and the wasm-runtime littest exercise loop-defer behavior at runtime, but not the wasm IR shape.)
Codegen smells (low, not inline): On wasm the selector is a pure integer forced through the pointer-typed Defer.Reth/Rund fields, so every store emits inttoptr and every dispatch emits load+ptrtoint+switch (ssa/eh.go:585-611). LLVM typically folds the constant cast pair, so this is a smell rather than a hot-path cost. Separately, jumpDeferTarget allocates a fresh single-instruction unreachable default block on every call (twice per deferring function on wasm); a shared per-function unreachable block would avoid the minor IR bloat. Both optional.
| target := b.Load(ptr) | ||
| if b.Prog.target.GOARCH != "wasm" { | ||
| blocks := make([]BasicBlock, len(targets)) | ||
| for i, target := range targets { |
There was a problem hiding this comment.
Confusing shadowing (maintainability). The outer target (line 593) is the loaded Expr (selector / block address), but this loop and the one at line 606 re-bind target to a deferTarget. The code is correct today — IndirectJump(target, blocks) uses the outer Expr because the loop var is loop-scoped — but this is exactly the shadowing that invites a bug if a future edit moves a use of target into or out of the loop. Consider renaming the outer variable (e.g. loaded/addr) or the loop variable (e.g. t).
There was a problem hiding this comment.
Fixed in 756a51c: the loaded Expr is now named loaded, so the deferTarget loop variables no longer shadow it.
| nexts := self.rundsNext | ||
| if len(nexts) == 0 { | ||
| rundTargets := self.rundTargets | ||
| if len(rundTargets) == 0 { |
There was a problem hiding this comment.
Dead guard. rundTargets is always seeded with one element in initDeferState ({index: 0, block: rethrowBlk}) and only ever appended to, and endDefer already returned early when self == nil. So len(rundTargets) can never be 0 here. Harmless, but a reader tracing the index bookkeeping may waste time reasoning about an impossible empty case — consider dropping it or replacing it with a comment documenting the invariant.
There was a problem hiding this comment.
Kept the guard and clarified its invariant in 756a51c. Removing it makes the existing ssa.TestEndDefer panic because endDefer deliberately tolerates a partially constructed aDefer before initDeferState seeds target 0. go test ./ssa -count=1 passes with the documented guard.
|
@fennoai The latest revision is ready for another review. All CI checks are green, including the Native/Wasm target matrix, and Codecov patch coverage is 100%. Please take another look when available. |
Re-review complete — LGTMI re-reviewed the latest revision over the locked range Verified statically:
Not done: couldn't compile/run the suite here — the LLVM C headers for |
There was a problem hiding this comment.
Re-review: ssa/wasm defer continuation selectors 
Conclusion: LGTM. The latest revision is a clean, correct, well-tested change. I found no blocking issues. Reviewed statically over the locked range d39ed76…dcff207.
What I verified
Core dispatch logic (ssa/eh.go)
- Selector density & defaults.
rundTargetsindices are0,1,2,…(seed0=rethrowBlk, one perRunDefers);rethTargetscovers{0,1,…,n}(0→procBlk,1→terminal rethrow,2..n→intermediate continuations). Both wasm switches are dense and carry anunreachableinvalid default viajumpDeferTarget. The committed goldens confirm this: reth switch cases1,2,0, rund switch cases0,1. - Native path is behavior-preserving.
deferTargetValuereturnsblock.Addr()on native, sostoreDeferTarget/jumpDeferTargetemit exactly the priorblockaddress+indirectbrIR. The refactored loop-deferresumeargument is provably the same block as the olddrainEntry(both arerethTargets[i+1]/rethsNext[i+1], the blockendDeferopens before invokingstmts[i]), so the native panic-during-drain resume target is unchanged. - Reth-selector preservation while draining is threaded correctly through
appendDeferStmt/appendLoopDeferDrainer→loopDeferDrainer(self, resume), ensuring a nested panic resumes at the right continuation. initDeferState: native reth initdeferTargetValue({0, procBlk})== oldprocBlk.Addr(); the new wasm-only rund seed (selector 0 = rethrowBlk) is consistent withrundTargets[0].
Runtime ABI (runtime/internal/runtime/z_rt.go) — comment-only; field types, order, size, and offsets unchanged. TestDeferContinuationDispatch additionally asserts fields 3/4 stay unsafe.Pointer across all targets.
Flags path (internal/llgen/llgenf.go) — applyFlagsFile parses GOOS=/GOARCH=/-target (all spellings), resolves -target to GOOS/GOARCH (with -target=wasm → js/wasm), and mutates a copy next, committing only on success. Error handling is atomic: on a bad/unknown/valueless -target the original config is left untouched — locked in by TestApplyFlagsFileErrorIncludesPath.
ModeGen short-circuit (internal/build/build.go) — the early return nil is placed after LLVM transforms, opt passes, and emitFuncInfoEntrySites, so the in-memory module is fully formed; it only skips cgo/asm/archive steps. This is what lets litgen emit wasm goldens with emcc absent.
Test coverage — strong and targeted:
TestDeferContinuationDispatch: native (blockaddress + 2indirectbr, no switch) vs wasm (no blockaddress/indirectbr, 2switch i32, ≥2unreachable), plus the empty-RunDefers-before-any-defer edge case.TestPanicWhileDrainingLoopDefersand the wasm-runtime fixture exercise a panic raised mid-drain (order [2,1,0], recovered value checked) — the exact path this refactor touches.- Golden files match every claim: the two wasm goldens have identical CHECK lines; native retains block addresses.
Notes (non-blocking)
- I could not compile or run the suite in this sandbox — the LLVM C headers (
llvm-c/Core.h) forgithub.com/xgo-dev/llvmaren't present, sogo build ./ssa/...fails at the cgo binding. My review is therefore static: source reasoning plus the committed golden IR. I'm relying on your report of green CI + 100% patch coverage for the dynamic confirmation. - Precedence when a
flags.txtsets bothGOOS=/GOARCH=and-target:-targetwins (overwrites GOOS/GOARCH). Reasonable and matches the PR description; just worth a one-line comment if that combination is ever expected in practice.
Nice work on keeping the Native ABI and codegen untouched while making the Wasm path lowerable.
1 participant
Implements #2276.
Problem
LLGo's defer lowering preserves function-local continuations in runtime.Defer.Rund and runtime.Defer.Reth. Native targets encode them as LLVM blockaddress values and resume with indirectbr.
That representation is valid LLVM IR, but WebAssembly has structured control flow rather than arbitrary computed gotos. Its SelectionDAG rejects any residual BlockAddress or BRIND operation.
LLVM schedules IndirectBrExpandPass for WebAssembly to convert blockaddress/indirectbr into integer selectors and a switch. That late fallback is not sufficient as LLGo's correctness boundary: with LLVM 19, the affected runtime IR deterministically reaches WebAssembly instruction selection with the computed-goto form still present and crashes while compiling runtime.EnsureLocalInitializer after #2079. This is a compiler crash before any WebAssembly module is produced, not a crash in the generated program and not a flaky build.
LLVM 22.1.8 does not remove the limitation. It still rejects computed gotos and still relies on the materially unchanged IndirectBrExpandPass. A version-specific crash may change, but LLGo still needs to guarantee that WebAssembly defer IR contains no computed-goto form.
The previous implementation also needed to cover both continuations: converting Rund alone leaves Reth, including panic during loop-defer draining, exposed to the same failure.
Design
Why Native keeps blockaddress
Native targets retain the existing blockaddress and indirectbr path.
Their LLVM backends can lower this form directly to a label address and register-indirect branch. Replacing it with selectors everywhere would add a different CFG representation, pointer/integer conversion, an invalid/default edge, and a switch that may become a compare chain or jump table.
Keeping the Native path:
This is a target-capability decision, not an operating-system decision. Another architecture should use selectors only if its backend cannot reliably lower computed gotos.
Supporting changes
Benefits
Validation