ssa,runtime: make panic and recover Go-compatible - #2034
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
9662dbc to
3076e79
Compare
f4d93ca to
207675c
Compare
207675c to
bd5f8b5
Compare
5ee4bd8 to
a7b4d8c
Compare
a1dbac0 to
934dc07
Compare
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
9077388 to
e0f1b54
Compare
Re-expresses xgo-dev#1918 on the xgo-dev#2023 base (its remaining ~11k diff lines were the pre-xgo-dev#2012 funcinfo draft, superseded by the stage-5 chain): - recover() only succeeds when called directly by a deferred function (gc semantics): the panic node records the owning Defer frame at rethrow (panicKey/panicNode + GoDeferData), and Recover checks the caller is that frame's direct deferred call. Closure wraps carry StartRecoverFrameAlias/EndRecoverFrame so method-value and closure adapters stay transparent to the ownership check. - Rethrow keeps the xgo-dev#2023 PanicTraceback hook on the unrecovered path. - xfail: retire fixedbugs/issue4066 (2m-timeout entries; now runs in ~2.7s), fixedbugs/issue73916 and issue73916b (go1.26 recover semantics), validated on darwin/arm64 go1.26. Supersedes xgo-dev#1918.
e0f1b54 to
16aab70
Compare
|
@fennoai review |
|
FennoAI is processing your request, please wait a moment... |
There was a problem hiding this comment.
Code Review — panic/recover Go-compatibility
Reviewed the diff (c9515d8...72e9f90) across correctness, performance, security, and doc accuracy. Overall this is a high-quality, well-tested change: the activation-token recover model, linked panic records, and Goexit/type-assert/SIGBUS handling are carefully documented against the upstream Go runtime semantics (abi.FuncIDWrapper, goexit _panic, TypeAssertionError), and the new tests (recover_activation_test.go, recover_metadata_test.go, recover_defer_fixedbugs_test.go, recover_fault_unix_test.go) are strong regression guards — including the IR-ordering assertions for the raw-load-vs-checked-load interaction.
Two non-blocking observations below (both inline). Nothing here blocks merge.
Strengths
moveToDefercorrectly frees superseded panic nodes and clearsgp.recoverPanicwhen freeing the suspended node, avoiding a dangling pointer.abortPanicscleanly drains linked panics and resets recover state for the Goexit transition.functionMayRecoverSeenguards recursion via aseenset; the GlobalDCE capability check is retained even on the recover-token path (ssa/interface.go), which the comment explains well.
Notes on pre-existing behavior (not introduced here, so not flagged inline)
- The panic value is stored in a
c.Mallocnode (z_rt.go) — the same non-GC-heap pattern the base already used forpanic_. If bdwgc does not scan the libc-malloc heap, a panic value with no other live reference could in principle be reclaimed mid-unwind. Worth a separate look, but it predates this PR. galready embedded the ~512-bytepanicPCStoreby value before this PR; the per-goroutine footprint is unchanged by these changes.
Keep may-recover facts in a compilation-scoped, concurrency-safe Go SSA cache so closure values and dependency archive cache hits are classified without LLVM attributes. Make the lower-level Builder conservative, report the actual missing interface method at runtime, document bare-metal Goexit behavior, and cover the cross-package cache path.
iwasm does not provide the setjmp/longjmp imports used by LLGo defer. Keep the WASM fixture free of that defer path, and execute native artifacts after every cache scenario so dependency archive hits still validate cross-package recover.
Return the first missing interface method separately from the implementation boolean. This removes the empty-string sentinel from Implements and only resolves the method name on type-assertion error paths.
Summary
This consolidates the panic/recover compatibility work previously split across #2033 and #2252 into this PR.
This PR resolves 15 unique upstream GOROOT compatibility cases and removes 35 matching expectation records across the supported native lanes:
recoveronly from the direct deferred activation, including transparent compiler-generated method, closure, interface, and reflect wrappers;runtime.Goexitsupersede an active panic, matching the standard runtime'sgoexit _panicsemantics;runtime.TypeAssertionErrorvalues with Go-compatible source, concrete, asserted, and missing-method metadata;The review counterexample for
fixedbugs/issue8047b.goand theGoexit-during-panic counterexample are included as regression tests. They were not existing xfails and are not counted among the 15 resolved GOROOT cases.Implementation
Recover ownership
Recover permission is keyed by the deferred activation instead of only by the function. Direct function, method, interface-method, closure, and reflect-created defers carry that identity through compiler-generated transparent wrappers; indirect, nested, or recursively re-entered calls remain unable to recover another activation's panic.
The compiler-side wrapper classification mirrors the standard runtime's
abi.FuncIDWrapperrule. Recover-aware interface wrappers retain the GlobalDCE capability check while carrying the raw itab code address only as transient runtime bookkeeping data.Panic and Goexit state
LLGo now keeps linked panic records, so a nested panic can be recovered without losing an outer panic owned by another deferred activation. A panic replaced while unwinding the same defer frame is discarded and cannot resume later.
The standard runtime represents
Goexitwith agoexit _panicthat aborts linked active panics. LLGo keeps its existingg.goexitrepresentation and performs the equivalent state transition before starting the longjmp-based defer unwind. This keepsrecovernil during Goexit while preserving the existing behavior where a panic raised during Goexit may be recovered and Goexit then resumes.Type assertion panics and faults
Failed non-comma-ok assertions call
PanicTypeAssert(source, concrete, asserted, missingMethod)and recover a real*runtime.TypeAssertionError. Source ABI metadata is emitted only for non-empty source interfaces and only in the cold panic block.SIGBUS uses the same panic/unwind path as SIGSEGV where the native platform defines it, preserving named results and the
runtime.Errorclassification of recovered faults. Wasm and DWARF behavior are unchanged.Conformance
The 15 resolved GOROOT compatibility cases are:
recover.gorecover1.gorecover2.gorecover4.gozerodivide.gofixedbugs/issue18911.gofixedbugs/issue26094.gofixedbugs/issue4066.gofixedbugs/issue52072.gofixedbugs/issue73916.gofixedbugs/issue73916b.gofixedbugs/issue73917.gofixedbugs/issue73920.gointerface/embed3.gotypeparam/mdempsky/16.goValidation
Exact expected-removal matrices
Every case whose expectation is removed was rebuilt and executed with both
xfailandnot_applicableset to empty files. Runner list mode independently confirmed the selected distribution.run, 2rundirrun, 2rundirrun, 2rundirrun, 2rundirThe two Darwin
fixedbugs/issue4066.gotimeout removals were also rerun separately with the runner's default 1-minute program timeout on Go 1.26.5 and Go 1.24.11; both passed in about 10 seconds including build setup.Focused and regression checks
Goexit-during-active-panic counterexample; the pre-fix LLGo binary reproduced the bug by recovering"outer", and the final Darwin and Linux LLGo binaries pass it withrecover() == nil.TestReflect*andTestRecover*tests intest/go, plus the existing Goexit lifecycle/state tests.go test ./ssa ./cl -count=1 -timeout=20mpassed; the affected Goexit compiler/IR subtest also passed independently.go test ./test/goroot -count=1passed after the expectation cleanup.CI
At head
72e9f900d, pull-request CI completed with 41 successful checks, 1 expected conditional release skip, and 0 failures. Codecov reports 99.15% patch coverage against an 89.59% target.