Repository navigation
fix(runtime): preserve recovered panic source locations - #2567
Merged
Merged
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: fix panic line (recovered/re-panicked traceback)
The core fix is sound and well-tested. Removing the Windows-only guard around recordPanicSite is correctly scoped downstream (recordPanicSite only emits the PC-line label when p.panicSiteFuncs[p.goFn] is set), so it adds no per-store IR for programs that don't observe recovered-panic stacks. The recoverActive + interface-identity matching in takeRecovered is memory-safe: recoveredData is a real unsafe.Pointer field embedded in the GC-visible g, and it is only ever compared for identity, never dereferenced. The AssertNilDeref block rewiring to the continuation edge is correct.
One issue must be addressed before merge, plus two minor notes below (inline).
[P1] Removing the issue34123 xfail entries breaks an existing unit test
test/goroot/xfail.yaml drops the linux/amd64 and darwin/arm64 fixedbugs/issue34123.go entries, but test/goroot/runner_unit_test.go:362 still asserts that this case must match an xfail entry on linux/amd64:
{version: "go1.27.0", platform: "linux/amd64", tc: testCase{RelPath: "fixedbugs/issue34123.go", Directive: "run"}},TestObservedFailuresHaveXFailClassifications calls cfg.Match(...) and t.Errorf when !match. With the yaml entry gone, Match returns false, so this test will now fail. That line is not part of this PR's diff, so the removal leaves the test suite inconsistent. Remove the stale assertion at runner_unit_test.go:362 as part of this change. (The complementary windows-msvc/amd64 "must not match" assertion at line 388 stays consistent with the removal.)
Collaborator
Author
|
Addressed the P1 consistency issue in 2601197: removed the stale Linux observed-failure fixture for |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
Collaborator
Author
|
Updated in 5daef74 after investigating the paired benchmark comment. The +80 B in Same-path local macOS/arm64 comparison against Validation includes the full @fennoai Please re-review the size-conscious nil-check lowering and recovered-panic lifetime changes. |
Contributor
Re-review: size-conscious nil-check lowering & recovered-panic lifetimeRe-reviewed at head Nil-check lowering (
|
Collaborator
Author
|
Thanks for the re-review. Follow-up All four scenarios pass locally with Go and LLGo default/full LTO. As a negative control, the unmodified The previous CI round finished before this push: every other check passed, including 100% patch coverage and all paired benchmarks, but the Qiniu Linux LLGo shard hit its one-hour execution limit. GitHub currently returns no job log for that shard, so I cannot yet attribute the timeout to a particular test or claim the entire round passed. The follow-up starts a fresh CI round. The PR body now includes the cross-platform size results and the precise scope of the regular-CI regression coverage. All existing inline review threads remain resolved. |
cpunion
force-pushed
the
codex/fix-panic-line-20260912
branch
from
September 12, 2026 15:16
7aa6c5f to
cab2663
Compare
cpunion
force-pushed
the
codex/fix-panic-line-20260912
branch
from
September 12, 2026 16:16
cab2663 to
148f3d0
Compare
cpunion
force-pushed
the
codex/fix-panic-line-20260912
branch
from
September 12, 2026 23:32
148f3d0 to
cf807ff
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Dependency and targeted CI
Rebased onto #2575 at
a7863ee05, which independently fixes a captured weak-cleanup reentrancy deadlock in the Qiniu Linux shard-0 test. The original five panic-location commits remain patch-identical after rebase. The GitHub base remainsmainbecause both contributions come from a fork; the dependency's changes will drop out of this diff once #2575 is merged. The complete affected Qiniu Ubuntu 24.04 large / LLVM 22 / Go 1.27 shard-0 job passed for this stack in run 34702086188: the first test phase completed in 136 seconds, symbol checks in 12 seconds, build-mode checks in 602 seconds, all later integration checks passed, and the new weak-runtime pressure regression passed three times in 0.16 seconds, without ptrace sampling. #2575 separately passed the same complete job plus its old-runtime negative control in run 34703427423. The ordinary matrix for148f3d099completed with 64 successful checks, no failure, and only the expected tag-gated release job skipped; its normal affected Qiniu job independently passed in 22m26s, and Codecov patch coverage is 100% (26/26). No diagnostic workflow or stress retry is added to this PR.The current head
cf807ffb1rebases those same five patches onto #2575's source-comment update.git range-diffreports all five patches unchanged; compared with the previously validated148f3d099, the source diff contains only weak-runtime comments. CI results above belong to the named earlier revisions, not a new full-matrix result for this rebased head.Summary
testing.tRunner, while tying reuse to the still-active recover activation so a later panic of the same value gets a fresh traceback.fixedbugs/issue34123.goxfails.Fixes #2566. Addresses the
fixedbugs/issue17381.goandfixedbugs/issue27201.goregressions reported by #2564.Cause
LLGo longjmp-unwinds the original panic frames and reconstructs them from a per-goroutine PC snapshot. A same-value
panic(recover())previously replaced that snapshot with the re-panic site, sogo testfailures stopped attesting.go:2123. Separately, the nil-check cold-block self-loop introduced by #2538 allowed LLVM to infer that a static nil-dereference caller never returns; this both exposed a return-PC/function-entry boundary misattribution inissue17381and moved the recovered panic line inissue27201. Native pointer stores also lacked the scoped PC-line carrier already emitted on Windows, leavingissue34123at the earlier pointer-load line.The two recent regressions were traced to #2538 commit
cdaa60ff7: both original GOROOT programs pass with its parent88a698364and fail with that commit on macOS/arm64 with Go 1.27. Same-value repanic retention is a longer-standing omission in the snapshot design introduced by #2026, not a new #2538 regression. The pointer-store metadata was added during #2405 development and then restricted to Windows before merging, leaving the existing Linux/macOS gap unresolved; the former Windows-only runtime assertion is now enabled on every platform.Size and performance
The original version added 80 bytes of symbol-index records to
cprintffor five new runtime helpers and approximately 11 KiB of macOS machine code to non-LTOfmtprintfby joining nil-check failures back into the normal path. This revision removes those five helpers, moves snapshot bookkeeping behind the existing public-runtime hook so tiny programs do not link it, reuses the existing recover activation token, and leavesrecoverStateat its original two-pointer ABI. The snapshot retains three words of per-goroutine state without allocating a separate recovery object. Returning from a defer or transparent wrapper andGoexitclear the retained value.Local paired builds of base
daada50d2270and the updated source used the same checkout path, compiler options, and macOS/arm64 host. The code column below is the Mach-O__textsection; file sizes include alignment and all metadata.The remaining full-LTO code increase is explicitly not zero: keeping the panic helper potentially returning prevents return-PC boundary misattribution even after whole-program optimization. The protection is confined to the helper, emits no assembly instructions itself, and adds no runtime call to successful nil checks. CI's paired benchmarks remain the cross-platform size and timing acceptance check; local timing samples were too sensitive to host load to support a speedup claim.
The paired CI measurements for
5daef74c420bnow confirm no file or Text growth forcprintforcprintf-ltoon any native platform;printlnandprintln-ltoare unchanged or smaller. Non-LTOfmtprintffile deltas range from -3,584 B to +512 B, while full-LTO Text deltas are +3,104 B to +5,632 B and full-LTO file deltas are 0 B to +6,144 B. The report's Text metric is distinct from the local Mach-O__textmeasurement above. The broad macOS timing slowdown also affects unchanged Go timer controls, so this single run does not isolate an LLGo performance regression or establish a speedup. The LTO size cost is a limitation of this approach, not a proven lower bound.Validation
test/gosuite, compatible with bothgo testandllgo test.TestRuntimeStatementLineInfo/panic_callerchecks rawruntime.CallersPCs still resolve to the recovering caller after a statically panicking leaf (issue17381);load_panic_linerequires the nil load's exact line rather than the following observable statement (issue27201);store_panic_linerequires the pointer store's line rather than the earlier pointer load (issue34123) on every platform.TestCallerRepanicTracebackuses nestedtesting.T.Runto reproduce llcppg's same-value repanic. These extend the existing test binary without additional build-driver tests, GOROOT subprocess builds, workflow changes, nightly opt-in, or xfail allowances.test/gocases pass with host Go and LLGo default/full LTO. A negative control using the unmodifiedcdaa60ff7compiler and its runtime fails all four scenarios: the caller is missing, the nil load is reported two lines late, the store two lines early, and the nested test panic losescallerRepanicOrigin. These are behavioral checks, not merely compilation checks; the added in-process subtests take less than the test logger's 0.01-second resolution locally.go test ./ssa -run '^TestAssertNilDeref(ZeroExprNoPanic|ColdCall)$' -count=1go test ./cl -run '^TestCompileRuntimeCaller(StorePanicPCLineMetadataOnNativeTargets|PanicPCLineMetadata)$' -count=1go test ./test/go -run '^(TestCallerRepanicTraceback|TestCallerPanicTraceback|TestRuntimeStatementLineInfo)$' -count=1llgo test -v -run '^(TestCallerRepanicTraceback|TestCallerPanicTraceback|TestRuntimeStatementLineInfo)$' ./test/gofixedbugs/issue17381.go,fixedbugs/issue27201.go, andfixedbugs/issue34123.gopass on macOS/arm64 with both default compilation and full LTO, without xfail allowances.llgo test -timeout=5m ./test/go, plus full-LTO caller/recover/statement-line tests; repanic coverage includes transparent method wrappers, indirect recover attempts, uncomparable slice values, replacement panics, and later reuse of the same value.go test ./ssa ./cl -skip '^TestRunAndTestFrom' -timeout=5m -count=1; targeted SSA tests model whole-program argument propagation and emit objects for Linux/amd64, Darwin/arm64, and Windows/amd64, 386, and arm64, while retaining the original WebAssembly failure path.goplus/llcppg@dev-panic-atreproduction now prints the complete application stack instead of stopping attesting.tRunner; its innermost frame is the exact nil access atgo/ast/ast.go:520, followed bygogenand llcppgcompile_test.go:49andcompile_test.go:77.The open #2530 debug-location work was tested on current main and does not fix either GOROOT regression, so this PR does not depend on it.
A separate full local
go test ./clexceeded its five-minute whole-suite budget while advancing through native run fixtures; this is not listed as a full local pass. CI covers the complete fixture suites.