runtime: replace superseded panics across nested defer frames - #2295
Conversation
cpunion
added
the
go-test-compat
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
Clean, well-reasoned fix for the nested-defer-panic case. The core change — advancing gp.defer_ together with node.defer_ inside movePanicToDefer, and calling it unconditionally when a panic exists instead of only when link == gp.defer_ — correctly addresses the case where a siglongjmp reaches a parent's rethrow block before the child's normal defer cleanup restored gp.defer_. Duplicate (*panicNode)(ptr) casts were consolidated, the default/baremetal Rethrow paths now consistently set gp.defer_ = link, and no references to the old moveToDefer name remain.
I verified the change across four dimensions (correctness, memory safety, performance, docs/comments):
- Memory safety: the
movePanicToDeferfree loop only unlinks-then-freesprevnodes strictly belownode, nevernode/gp.panic_itself, and clears the livegp.recoverPanicwhen it points at a freed node — no double-free or danglinggp.panic_. - Performance: the free loop is amortized O(active-panics) over an unwind (each node unlinked before free), not O(n²); the added assignment is trivial on a non-hot path.
- Docs/comments & test: the new
movePanicToDeferdoc comment accurately describes updating both cursors; the newcl/_testgo/nesteddeferpanictest'sCHECK-LABEL/LITTESTdirectives andexpect.txtordering are correct.
Non-blocking observations
runtime/internal/runtime/z_default.go(Rethrow,link == nilpath):movePanicToDefernow also runs on the unrecovered-fatal path beforeTracePanic(node.arg)/c.Exit(2). This is safe — the loop frees only theprevchain, sonode.argremains valid forTracePanic— but it is a newly-reached code path for the fatal case and worth an explicit confirmation.- The baremetal
Rethrowchange (z_baremetal.go) mirrors the default path but is not exercised by a dedicated test; the newnesteddeferpanictest only runs under the non-baremetal build. Consider noting this coverage gap or adding baremetal coverage if feasible.
No blocking issues found.
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
The failed Go workflow was caused by the new native regression fixture being picked up by both embedded emulator suites. Both targets stop after the first outermost recovered panic ( |
53a1cc5 to
ae34bfe
Compare
2 participants
Summary
fixedbugs/issue43942.goRoot cause
The generated normal defer cleanup restores the goroutine defer head, but a panic raised by a nested deferred call can longjmp directly to the parent rethrow block and bypass that cleanup. The panic node then reaches the parent frame while the goroutine cursor still points at the child frame. The old
link == gp.defer_guard consequently skipped panic replacement, allowing the older panic to resume after the newer panic was recovered.Keep this transition in the runtime rethrow abstraction: moving to the parent frame now updates both cursors and removes older panic nodes already unwinding that frame. The same invariant is maintained for Goexit and the bare-metal rethrow implementation.
Coverage
The new LLGo-executed fixture covers:
issue43942shape)Validation
Runtime sources were exercised only through LLGo-built programs; host
go testcommands below are compiler/test drivers.fixedbugs/issue43942.goand adjacentissue48898.gopassed when run with LLGofixedbugs/issue43942.gopassed with LLGoissue43942binaries passed 20/20 runs on Go 1.26.5; the Go 1.24.2 source also passed 20/20 on both platformsgo test ./ssa -count=1 -timeout=20mThis fixes one unique GOROOT case and removes three records that represented that case on different Go versions/platforms.
Embedded scope
The new regression fixture is excluded from the existing ESP32 and ESP32-C3 emulator suites. Their bare-metal runtime still terminates after an outermost panic is recovered (the same limitation already documented for other panic fixtures); changing that runtime and its constrained panic-node allocator is outside this native GOROOT fix. Native macOS and Linux execution remains mandatory.