Skip to content

ssa: panic on static nil dereference#1890

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

ssa: panic on static nil dereference#1890
cpunion wants to merge 3 commits into
xgo-dev:mainfrom
cpunion:codex/goroot-order-coverage

Conversation

@cpunion
Copy link
Copy Markdown
Collaborator

@cpunion cpunion commented May 22, 2026

Summary

  • emit explicit runtime nil-deref panics for statically nil pointer loads/stores and nil field/array bases
  • add test/go assignment-order coverage for RHS-before-map-insert and multiple assignment map update before later nil store
  • remove fixed GOROOT xfails for fixedbugs/issue22881.go and fixedbugs/issue23017.go

Notes

  • GOROOT CI is slow/disabled in this repo, so validation here is intentionally targeted.
  • The new stable coverage is in test/go/assignment_order_test.go. A full llgo test ./test/go package build is currently blocked by an existing unrelated generic-local-type link error; the new file was run directly under llgo test.

Targeted commands

  • Reproduced before fix: go test ./test/goroot -run TestGoRootRunCases/fixedbugs/issue22881.go -count=1 -args -goroot "$(go env GOROOT)" -dirs fixedbugs -case '^fixedbugs/issue22881\.go$' -xfail /tmp/llgo-no-xfail.yaml -run-timeout 30s timed out in llgo run.
  • go test ./test/go -count=1
  • timeout 90s go run ./cmd/llgo test -run 'TestMapUpdateRHSNilDerefOrder|TestMultipleAssignmentMapUpdateBeforeNilStore' ./test/go/assignment_order_test.go
  • go test ./ssa -count=1
  • go test ./cl -run TestCompileLargeNilDerefInterfaceGuards -count=1
  • go test ./test/goroot -run TestGoRootRunCases/fixedbugs/issue22881.go -count=1 -args -goroot "$(go env GOROOT)" -dirs fixedbugs -case '^fixedbugs/issue22881\.go$' -xfail /tmp/llgo-no-xfail.yaml -run-timeout 30s
  • go test ./test/goroot -run TestGoRootRunCases/fixedbugs/issue23017.go -count=1 -args -goroot "$(go env GOROOT)" -dirs fixedbugs -case '^fixedbugs/issue23017\.go$' -xfail /tmp/llgo-no-xfail.yaml -run-timeout 30s
  • go test ./test/goroot -run TestGoRootRunCases -count=1 -args -goroot "$(go env GOROOT)" -dirs fixedbugs -case '^fixedbugs/(issue22881|issue23017)\.go$' -run-timeout 30s

@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

❌ Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
ssa/memory.go 60.00% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@cpunion
Copy link
Copy Markdown
Collaborator Author

cpunion commented May 22, 2026

Update on a9ab6096efedd67638e790091e7c05c634ede357.

Root cause:

  • The reported failed job llgo (ubuntu-latest, 19, 1.24.2) at run 26268180787 failed during dependency setup, before building/testing this PR. The job log shows wget could not resolve apt.llvm.org and exited while adding the LLVM apt key, so that particular failure is CI infrastructure/DNS, not an order/evaluation regression.
  • The actual order/evaluation issue covered by this PR is static nil pointer dereference codegen: constant nil loads/stores/field/index addresses could reach LLVM without LLGo's runtime nil-deref panic path, which could change assignment/map-update ordering behavior or crash incorrectly.

Fix:

  • Existing head 642917bc... adds static nil-deref assertions in SSA Load, Store, FieldAddr, and array-pointer IndexAddr, and removes the corresponding goroot xfails.
  • I added test/go coverage in a9ab6096e for the goroot issue22881/issue23017 style cases: RHS panic must happen before map insertion, multiple assignment must expose earlier assignments before later panics, and address-taking on the LHS must use pre-assignment values.

Local tests:

  • go test ./test/go -run 'Test(MapUpdateRHSNilDerefOrder|MapUpdateAppendRHSOrder|MultipleAssignmentMapUpdateBeforeNilStore|MultipleAssignmentOrderBeforeLaterPanic|MultipleAssignmentAddressUsesPreAssignmentValue)$'
  • go run ./cmd/llgo test -run 'Test(MapUpdateRHSNilDerefOrder|MapUpdateAppendRHSOrder|MultipleAssignmentMapUpdateBeforeNilStore|MultipleAssignmentOrderBeforeLaterPanic|MultipleAssignmentAddressUsesPreAssignmentValue)$' ./test/go
  • go test ./test/go
  • go test ./ssa ✅ (ok, then cached after test-only commit)
  • go test ./cl ✅ (ok, then cached after test-only commit)
  • git diff --check

CI:

  • New head CI has started for a9ab6096e; current checks are queued/in progress. I will keep monitoring and iterate if any real failure appears.

@cpunion
Copy link
Copy Markdown
Collaborator Author

cpunion commented May 24, 2026

Update for fixedbugs/issue23017.go coverage on commit 0bff43bf6d485100e48432daa99ec950c07095f7:

  • Classification: same root as the assignment order/evaluation/store semantics work. On the pre-fix baseline (642917bc3^) with xfail disabled, fixedbugs/issue23017.go timed out in llgo run; current HEAD passes the focused goroot case.
  • Stable coverage: test/go/assignment_order_test.go covers the issue23017 shapes: earlier map/pointer store surviving a later nil/index/nil-map panic, nil-map assignment preventing later stores, and LHS address evaluation using the pre-assignment pointer for field/nested-field stores and swap.
  • Xfail handling: fixedbugs/issue23017.go is removed from xfail because the covered goroot case now passes. No additional xfail was removed in this update.
  • Not covered/changed here: no GC/liveness/finalizer/goroutine work, and no chan/print/recover/runtime-domain fixes beyond the existing assignment-order nil-deref behavior needed for these cases.

Local validation:

  • go test ./test/goroot -count=1 -run TestGoRootRunCases -args -goroot /Users/lijie/sdk/go1.26.0 -dirs fixedbugs -case '^fixedbugs/issue23017\\.go$' -directive-mode runlike passed.
  • go test ./test/go -count=1 passed.
  • go test ./ssa ./cl -count=1 passed (cl took 586.759s).

CI after push is triggered and currently queued/pending.

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