Skip to content

runtime: preserve recover error values#1882

Draft
cpunion wants to merge 5 commits into
xgo-dev:mainfrom
cpunion:codex/goroot-panic-coverage
Draft

runtime: preserve recover error values#1882
cpunion wants to merge 5 commits into
xgo-dev:mainfrom
cpunion:codex/goroot-panic-coverage

Conversation

@cpunion
Copy link
Copy Markdown
Collaborator

@cpunion cpunion commented May 22, 2026

Summary

  • Panic with runtime error values instead of plain strings so recover() values assert to error and runtime.Error.
  • Build TypeAssertionError values for compiler-generated failed type assertions, and fix slice-to-array conversion panic bounds order.
  • Preserve direct recover semantics through compiler-generated method wrappers when the wrapper is itself the deferred call, while keeping nested wrapper calls non-recovering.
  • Preserve Go 1.26 embedded method-wrapper function pointer semantics for fixedbugs/issue73917.go and fixedbugs/issue73920.go: a recover in a real helper call reached through deferred promoted wrappers remains indirect, and the outer deferred recover still sees the panic.
  • Preserve local values across fault-triggered longjmp recovery by emitting volatile loads/stores for local SSA allocs in functions with recover blocks.
  • Add focused test/go coverage for recovered runtime errors, type assertion panic values, method-wrapper recover semantics, embedded wrapper function-pointer recover semantics, and fault recovery preserving named results; remove the now-passing recover2.go, zerodivide.go, recover4.go, fixedbugs/issue73917.go, and fixedbugs/issue73920.go GOROOT xfails.

Still XFail / Out of Scope

  • recover.go remains xfail in this PR. On this branch it still fails before the recover-10 path with the ssa: panic with runtime type assertion errors #1892 interface type identity issue: interface conversion: interface {} is func(*main.T1), not func(*main.T1) (types from different scopes).
  • This PR does not cherry-pick ssa: panic with runtime type assertion errors #1892. The recover-wrapper behavior exposed by missing recover 10 is covered independently by test/go.
  • The new embedded-wrapper test/go cases are Go 1.26+ semantic coverage. Go 1.24 has pre-Go 1.26 behavior for the upstream issue73917.go/issue73920.go programs, so those local tests skip under Go <1.26.
  • A full Go 1.26 standard-library run of test/go currently hits unrelated nil/range behavior in TestRangeOverNilArrayPointerCallIsEvaluated; this PR intentionally does not cover nil/range, chan, print, interface, reflect, GC, liveness, finalizer, or goroutine domains.

Testing

  • go test ./test/go -run 'TestRecoverThrough|TestRecoverAfterFault' -count=1
  • go run ./cmd/llgo test -run 'TestRecoverThrough|TestRecoverAfterFault' -count=1 ./test/go
  • go test ./test/goroot -run TestGoRootRunCases/recover4.go -count=1 -args -goroot "$(go env GOROOT)" -dirs . -case '^recover4\.go$' -xfail /tmp/llgo-empty-xfail.yaml
  • go test ./test/goroot -run TestGoRootRunCases/recover4.go -count=1 -args -goroot "$(go env GOROOT)" -dirs . -case '^recover4\.go$'
  • go test ./test/goroot -run TestGoRootRunCases/recover.go -count=1 -args -goroot "$(go env GOROOT)" -dirs . -case '^recover\.go$' (passes via retained xfail)
  • go test ./test/goroot -run TestGoRootRunCases/recover.go -count=1 -args -goroot "$(go env GOROOT)" -dirs . -case '^recover\.go$' -xfail /tmp/llgo-empty-xfail.yaml (expected failure: ssa: panic with runtime type assertion errors #1892 interface type identity blocker)
  • go test ./test/go -run 'TestDeferredEmbedded.*MethodWrapperKeepsIndirectRecoverNil|TestRecoverThrough' -count=1
  • /Users/lijie/sdk/go1.26.0/bin/go test ./test/go -run 'TestDeferredEmbedded.*MethodWrapperKeepsIndirectRecoverNil' -count=1 -v
  • /Users/lijie/sdk/go1.26.0/bin/go run ./cmd/llgo test -run 'TestDeferredEmbedded.*MethodWrapperKeepsIndirectRecoverNil|TestRecoverThrough' -count=1 ./test/go
  • go test ./test/goroot -run TestGoRootRunCases -count=1 -args -goroot /Users/lijie/sdk/go1.26.0 -dirs fixedbugs -case '^fixedbugs/issue739(17|20)\.go$'
  • go test ./test/go -count=1
  • (cd runtime && go test ./internal/runtime -count=1)
  • go test ./ssa -count=1
  • go test ./cl -count=1
  • /Users/lijie/sdk/go1.26.0/bin/go test ./test/go -count=1 (known unrelated failure: TestRangeOverNilArrayPointerCallIsEvaluated nil array pointer range SIGSEGV)

GOROOT CI

Full GOROOT CI is disabled/too slow for regular PR validation in this repo and the GOROOT workflow is manual-only. This PR keeps stable coverage in test/go and lists the exact targeted GOROOT commands above. I will monitor the available PR checks after pushing the fork head branch; I will not push branches to xgo-dev/llgo.

@cpunion
Copy link
Copy Markdown
Collaborator Author

cpunion commented May 22, 2026

CI note: normal PR CI is running from the fork branch cpunion:codex/goroot-panic-coverage.

I attempted to dispatch the manual-only GOROOT workflow for the PR head branch, but GitHub rejected the upstream dispatch because the ref only exists on the fork:

HTTP 422: No ref found for: codex/goroot-panic-coverage

I also attempted to dispatch .github/workflows/goroot.yml on cpunion/llgo at the fork branch, but GitHub returned:

HTTP 404: workflow .github/workflows/goroot.yml not found on the default branch

Per branch policy, I did not push an upstream xgo-dev/llgo ref. Local targeted GOROOT commands are listed in the PR body.

@cpunion cpunion force-pushed the codex/goroot-panic-coverage branch from 699e9cf to 82b8a1a Compare May 22, 2026 08:59
@cpunion
Copy link
Copy Markdown
Collaborator Author

cpunion commented May 22, 2026

Updated PR after rebasing onto current xgo-dev/main and adjusted the new genericembediface FileCheck expectation for the runtime.TypeAssertError path introduced by this branch.

Additional tests run locally:

  • go test ./cl ./ssa -run 'TestRunAndTestFromTestgo/genericembediface|TestFromTestgo/genericembediface' -count=1
  • go test ./test/go -run 'TestRecoveredRuntimePanicsAreErrors|TestRecoveredTypeAssertionPanicsAreRuntimeErrors' -count=1
  • go test ./test/goroot -run TestGoRootRunCases -count=1 -timeout=10m -args -goroot "$(go env GOROOT)" -dirs . -case '^(recover2|zerodivide)\.go$' -run-timeout=60s -build-timeout=3m
  • git diff --check

The earlier Ubuntu gpg: no valid OpenPGP data found failure occurred during dependency setup before repository tests ran, so the new push should trigger regular PR CI again from the fork branch.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 22, 2026

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 80.67227% with 23 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
ssa/eh.go 70.45% 12 Missing and 1 partial ⚠️
cl/instr.go 54.54% 8 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

@cpunion
Copy link
Copy Markdown
Collaborator Author

cpunion commented May 24, 2026

Update from commit 6b65c2c:\n\n- Fixed direct vs indirect recover scoping and nested panic stack handling; removed the recover1.go xfail entries covered by this PR.\n- Kept recover.go xfails: the remaining failure is reflect/interface type identity in test9reflect2, not the recover/defer panic root.\n- Kept recover4.go xfails: SIGBUS is now routed to the panic path, but the remaining mismatch is stale local/named-result state after fault recovery (memcopy returned 0 vs 131067), which I am not mixing into this recover/defer PR.\n- Kept deferprint.go and fixedbugs/bug409.go xfails: remaining mismatches are float print exponent formatting (+e+00 vs +e+000).\n\nLocal validation:\n- go test ./test/go -count=1\n- go test ./test/goroot -count=1 -run TestGoRootRunCases -args -goroot $(go env GOROOT) -dirs .,fixedbugs -case '^(recover.go|recover1.go|recover4.go|deferprint.go|fixedbugs/bug409.go)$' -run-timeout=30s\n- go test ./ssa ./cl -count=1\n- cd runtime && go test ./internal/runtime -count=1

@cpunion
Copy link
Copy Markdown
Collaborator Author

cpunion commented May 24, 2026

Update after 77ceb09:

  • Added focused recover/defer coverage in test/go for deferred method wrappers and direct-defer recover semantics.
  • Added fault recover coverage for preserving a named result after a protected-memory fault path.
  • Fixed recover-frame forwarding through recover-transparent wrappers and made recover-function local alloc loads/stores volatile so fault longjmp keeps observable locals.
  • Removed the recover4.go goroot xfails now that it passes independently on this branch.
  • Kept recover.go xfail: without ssa: panic with runtime type assertion errors #1892 it still stops first on the interface type-identity panic, so this PR does not cherry-pick the interface fix. The recover-wrapper behavior is covered independently in test/go.

Focused tests passed locally: test/go recover cases via Go and llgo, recover4.go goroot with and without xfail, recover.go goroot with retained xfail, go test ./test/go, go test ./ssa, go test ./cl, and runtime/internal/runtime inside the runtime module.

@cpunion
Copy link
Copy Markdown
Collaborator Author

cpunion commented May 24, 2026

Update for commit 66a820936f3baab3164393d63f95d944fd9566fb:

  • Classified fixedbugs/issue73917.go and fixedbugs/issue73920.go as Go 1.26 embedded method-wrapper function-pointer defer/recover semantics. Both are in the same recover/defer wrapper scope as this PR.
  • Added test/go coverage for value and pointer promoted method wrappers reached through function pointers, keeping the helper recover indirect so the outer deferred recover still receives the panic.
  • Removed the now-passing Go 1.26 xfails for both cases on darwin/arm64 and linux/amd64.
  • Left nil/range, interface ssa: panic with runtime type assertion errors #1892, GC/liveness/finalizer/goroutine, chan, print, reflect, and related non-wrapper domains untouched.

Focused checks passed:

  • go test ./test/goroot -run TestGoRootRunCases -count=1 -args -goroot /Users/lijie/sdk/go1.26.0 -dirs fixedbugs -case '^fixedbugs/issue739(17|20)\.go$'
  • /Users/lijie/sdk/go1.26.0/bin/go test ./test/go -run 'TestDeferredEmbedded.*MethodWrapperKeepsIndirectRecoverNil' -count=1 -v
  • /Users/lijie/sdk/go1.26.0/bin/go run ./cmd/llgo test -run 'TestDeferredEmbedded.*MethodWrapperKeepsIndirectRecoverNil|TestRecoverThrough' -count=1 ./test/go
  • go test ./test/go -count=1
  • (cd runtime && go test ./internal/runtime -count=1)
  • go test ./ssa -count=1
  • go test ./cl -count=1

Known unrelated non-covered check: /Users/lijie/sdk/go1.26.0/bin/go test ./test/go -count=1 fails in TestRangeOverNilArrayPointerCallIsEvaluated with a nil array pointer range SIGSEGV, which is outside this recover/defer wrapper PR.

@cpunion
Copy link
Copy Markdown
Collaborator Author

cpunion commented May 24, 2026

Update for fixedbugs/issue43835.go at 5fc53956f98a7718eab8399fd905756b7f058ece:

  • Classified as the same recover/defer/named-result-after-panic family: the nil deref is only the panic trigger; after deferred recover, partial true writes from an unfinished assignment/return must not be observed.
  • Implemented a compiler-side ordering fix for recover-capable functions: explicit nil-deref assertion before deref loads that could otherwise be eliminated/moved past result writes; existing volatile result-slot handling remains scoped to recover blocks.
  • Added stable test/go coverage for assignment, unnamed return, and named return expression forms.
  • Removed only the now-covered fixedbugs/issue43835.go xfails for darwin/arm64 and linux/amd64. No GC/liveness/finalizer/goroutine, chan, print, interface, or broader nil-domain changes are included.

Local validation:

  • go test ./test/goroot -run TestGoRootRunCases -count=1 -timeout 30m -args -goroot /Users/lijie/sdk/go1.26.0 -dirs fixedbugs -case '^fixedbugs/issue43835\\.go$' -directive-mode runlike -run-timeout 2m\n- go test ./test/go -count=1\n- go test ./ssa -count=1\n- go test ./cl -count=1\n\nRuntime package tests were not rerun for this increment because no runtime code changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants