Skip to content

runtime: classify recovered runtime panics#1904

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

runtime: classify recovered runtime panics#1904
cpunion wants to merge 5 commits into
xgo-dev:mainfrom
cpunion:codex/goroot-recover3-coverage

Conversation

@cpunion
Copy link
Copy Markdown
Collaborator

@cpunion cpunion commented May 22, 2026

Summary

  • fix recovered runtime panics so recover3.go classifies them as runtime.Error
  • add stable test/go coverage for divide-by-zero, nil pointer, bounds, and type assertion recover paths
  • remove the now-passing recover3.go darwin/arm64 xfail entry

Coverage

This PR keeps the feature covered under test/go, so it remains checked even when the slow GOROOT workflow is disabled.

Tests

  • go test ./test/go -run 'TestRecoverRuntimeErrorClassification' -count=1
  • go test ./test/goroot -run TestGoRootRunCases -count=1 -timeout=10m -args -goroot "$(go env GOROOT)" -dirs . -case '^recover3\.go$' -xfail /tmp/llgo-empty-xfail.yaml -run-timeout=60s -build-timeout=3m
  • go test ./test/goroot -run TestGoRootRunCases -count=1 -timeout=10m -args -goroot "$(go env GOROOT)" -dirs . -case '^recover3\.go$' -run-timeout=60s -build-timeout=3m
  • go test ./ssa -run 'TestTypeAssert|TestMakeInterface|TestBinOp' -count=1
  • go run -tags=dev ./cmd/llgo test -run 'TestRecoverRuntimeErrorClassification' -count=1 ./test/go
  • (cd runtime && go test ./internal/runtime -count=1)
  • git diff --check

CI note

The GOROOT CI workflow is slow and currently disabled. The PR is draft so regular PR CI can run while review stays gated. Manual GOROOT workflow_dispatch cannot resolve fork-only refs unless the branch exists in xgo-dev/llgo; this PR intentionally keeps the branch only in cpunion/llgo.

@cpunion
Copy link
Copy Markdown
Collaborator Author

cpunion commented May 22, 2026

Pushed d49f9e4 to cpunion/codex/goroot-recover3-coverage.

Root cause: the recover3 runtime panic changes intentionally lower failed type assertions to runtime.PanicTypeAssert (and nullable array pointer indexing to AssertNilDeref), but several source FileCheck golden comments still expected the old panic-string construction and old IR numbering.

Updated the affected test/go coverage-backed IR checks under cl/_testgo, cl/_testrt, and cl/_testdata. Local verification:

  • go test -timeout 20m ./cl -run '^(TestRunAndTestFromTestgo|TestRunAndTestFromTestrt|TestRunAndTestFromTestdata)$/(abimethod|ifaceprom|reader|reflect|tpinst|any|funcdecl|index|makemap|tpabi|typed|vargs)$'\n- go test -timeout 20m ./ssa\n- go test -timeout 20m ./test/go -run '^TestRecoverRuntimeErrorClassification$'\n- go test ./internal/runtime (from runtime/)\n- git diff --check\n\nNote: I did not use xgo-dev as a push target; the PR remains draft.

@cpunion
Copy link
Copy Markdown
Collaborator Author

cpunion commented May 22, 2026

Follow-up pushed: ef99d1c39e2a07908d1421390567a0b9f6eed4980 to cpunion/codex/goroot-recover3-coverage.

The d49f9e4 CI failure in Go / test (ubuntu-latest, 19) was not an environment issue. Job 77407357733 failed in ssa/TestFromTestgo/genericembediface because that test arrived from current xgo-dev/main during PR merge testing and still expected the old type assertion panic string. I merged current xgo-dev/main into the branch and updated genericembediface to expect runtime.PanicTypeAssert.

Local verification after the merge:

  • go test -timeout 20m ./ssa
  • go test -timeout 20m ./cl -run '^(TestRunAndTestFromTestgo|TestRunAndTestFromTestrt|TestRunAndTestFromTestdata)$/(abimethod|genericembediface|ifaceprom|reader|reflect|tpinst|any|funcdecl|index|makemap|tpabi|typed|vargs)$'\n- go test -timeout 20m ./test/go -run '^TestRecoverRuntimeErrorClassification$'\n- (cd runtime && go test ./internal/runtime)\n- git diff --check\n\nPush target was origin (cpunion/llgo) only; PR remains draft.

@cpunion
Copy link
Copy Markdown
Collaborator Author

cpunion commented May 22, 2026

Follow-up pushed: 64cbd644a8f9965f0a92f9cb80a880e98f927e68 to cpunion/codex/goroot-recover3-coverage.

The ef99d1c Ubuntu Go failure was not CI/environmental. Job 77411188389 failed in cl/TestRunAndTestFromTestgo/closureall with an undefined symbol for github.com/goplus/llgo/cl/_testgo/closureall.cSqrt. Root cause: the earlier regenerated CHECK-LINE comments sat between //go:linkname cSqrt C.sqrt and func cSqrt, so the Go directive was no longer adjacent to its declaration and linkname was ignored.

Fix: moved the CHECK-LINE comments away from the directive/declaration pair and restored the IR expectation to @sqrt.

Local verification:

  • go test -timeout 20m ./cl -run '^(TestRunAndTestFromTestgo|TestRunAndTestFromTestrt|TestRunAndTestFromTestdata)$/(abimethod|closureall|genericembediface|ifaceprom|reader|reflect|tpinst|any|funcdecl|index|makemap|tpabi|typed|vargs)$'\n- go test -timeout 20m ./ssa -run '^(TestFromTestgo|TestFromTestrt|TestFromTestdata)$/(abimethod|closureall|genericembediface|ifaceprom|reader|reflect|tpinst|any|funcdecl|index|makemap|tpabi|typed|vargs)$'\n- go test -timeout 20m ./test/go -run '^TestRecoverRuntimeErrorClassification$'\n- (cd runtime && go test ./internal/runtime)\n- git diff --check\n\nPush target was origin (cpunion/llgo) only; PR remains draft.

@cpunion
Copy link
Copy Markdown
Collaborator Author

cpunion commented May 22, 2026

Follow-up CI check for 64cbd644a2ac92382f02312c5b6411198cb7dfa7:

  • Go / test (ubuntu-latest, 19) job 77414738781 completed successfully.
  • The previously failing Test step passed; embedded emulator env and std symbol coverage steps also passed.
  • PR is still draft; remaining macOS jobs are still queued in GitHub Actions at the time of this comment.

@codecov-commenter
Copy link
Copy Markdown

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

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

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