Skip to content

Fix nil deref checks for address expressions#1896

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

Fix nil deref checks for address expressions#1896
cpunion wants to merge 4 commits into
xgo-dev:mainfrom
cpunion:codex/goroot-nilbounds-coverage

Conversation

@cpunion
Copy link
Copy Markdown
Collaborator

@cpunion cpunion commented May 22, 2026

Summary

  • Fix nil panic values so explicit nil deref checks and nil interface method dispatch panic with values implementing runtime.Error.
  • Add explicit nil checks for nil pointer-to-array slicing and large-offset field address chains, including call/parameter bases needed by nilptr.go.
  • Keep this PR scoped to nil/nilptr cases: typeparam/mdempsky/16.go remains xfailed for ssa: panic with runtime type assertion errors #1892; nil.go remains xfailed because the reproduced failure is nil channel/goroutine behavior; devirtualization_nil_panics.go remains xfailed because the current go1.26 failure reaches runtime.CallersFrames and panics todo after recovering the nil panic.
  • Add stable test/go coverage for generic nil interface method calls, nil interface method expressions, nil array-pointer slicing, and large-offset nil field reads.
  • Remove only the fixed nil/nilptr GOROOT xfails for typeparam/issue51521.go and nilptr.go.

Validation

  • go test ./test/go -count=1
  • go run ./cmd/llgo test -run TestNilPanicValuesAreRuntimeErrors ./test/go
  • go test ./test/goroot -count=1 -run TestGoRootRunCases -v -args -goroot "$(go env GOROOT)" -dirs .,typeparam -case '^(nil\.go|nilptr\.go|nilptr2\.go|typeparam/issue51521\.go)$' -directive-mode runlike
  • go test ./test/goroot -count=1 -run TestGoRootRunCases -v -args -goroot "$(go env GOROOT)" -dirs .,typeparam -case '^(nil\.go|nilptr\.go|nilptr2\.go|typeparam/issue51521\.go)$' -directive-mode runlike -xfail /tmp/llgo-empty-xfail.yaml (expected nil.go failure; nilptr2.go and typeparam/issue51521.go pass)
  • go test ./test/goroot -count=1 -run TestGoRootRunCases -v -args -goroot "$(go env GOROOT)" -dirs typeparam/mdempsky -case '^typeparam/mdempsky/16\.go$' -directive-mode runlike (expected xfail retained for ssa: panic with runtime type assertion errors #1892)
  • go test ./test/goroot -count=1 -run TestGoRootRunCases -v -args -goroot /opt/homebrew/Cellar/go/1.26.0/libexec -dirs . -case '^devirtualization_nil_panics\.go$' -directive-mode runlike -xfail /tmp/llgo-empty-xfail.yaml (expected non-nil-scope panic: todo from runtime.CallersFrames)
  • go test ./test/goroot -count=1 -run TestGoRootRunCases -v -args -goroot /opt/homebrew/Cellar/go/1.26.0/libexec -dirs . -case '^devirtualization_nil_panics\.go$' -directive-mode runlike (expected xfail retained)
  • go test ./ssa -count=1
  • go test ./cl -count=1

Notes

  • nilptr.go is build-tag excluded on local darwin/arm64 with Go 1.24, so this PR covers its root causes with test/go and removes the linux xfail for CI coverage.
  • go test ./runtime/internal/runtime -count=1 and go test ./runtime/... -count=1 are not directly runnable in this module layout because runtime/... is outside the main module package path.

@cpunion
Copy link
Copy Markdown
Collaborator Author

cpunion commented May 22, 2026

Updated head: 8595dbe

Root cause:

  • p.value for a large field-to-interface copy goes through MakeInterfaceFromPtr. The previous nil guard could check the derived field address instead of the original base pointer. For a nil base plus a large field offset, the derived pointer is non-null, so the expected nil-pointer panic was skipped.

Fix:

  • assertNilDerefAddr now walks field-address chains, and pointer-array index-address chains, back to the base pointer before emitting the nil-deref guard.
  • Tightened cl coverage so fieldIface specifically has a base-pointer null check, not just any AssertNilDeref elsewhere in the module.
  • Existing test/go coverage includes TestNilPointerLargeFieldToInterfacePanics, which is the failing CI case.

Local tests:

  • PASS: go test ./test/go
  • PASS: go test ./cl -run 'TestCompileLargeNilDerefInterfaceGuards|TestUnusedDerefNilChecks' -count=1\n- PASS: go test ./ssa -run TestMakeInterfaceFromPtrKinds -count=1\n- PASS: go test ./ssa\n- PASS once with the current worktree-built llgo: LLGO_ROOT=/Users/lijie/source/goplus/llgo-wt-goroot-nilbounds /tmp/llgo-nilbounds-bin/llgo test -timeout=20m -run TestNilPointerLargeFieldToInterfacePanics github.com/goplus/llgo/test/go\n\nLocal note:\n- Broader local darwin llgo test attempts hit a linker issue for unrelated generic local symbols in test/go (undefined symbol: *_llgo_github.com/goplus/llgo/test/go.local[...]). The pushed fix targets the Linux CI runtime failure above.\n\nCI status:\n- New checks are running/queued on head 8595dbea; PR remains draft.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 23, 2026

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

Codecov Report

❌ Patch coverage is 92.27642% with 19 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
cl/compile.go 91.66% 11 Missing and 6 partials ⚠️
ssa/memory.go 86.66% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@cpunion
Copy link
Copy Markdown
Collaborator Author

cpunion commented May 24, 2026

Update for fixedbugs/issue38496.go (commit 315b17e):

  • Classification: same nil check / nil pointer deref root. The case validates that bounds check elision for a known-in-bounds array element does not also erase the nil pointer load panic from var m [2]*int; _ = *m[1].
  • Code impact: no additional compiler/runtime change was needed on top of this PR's nilptr fixes; the case already passes when run without xfail on this branch.
  • Coverage: added stable test/go coverage in TestNilPanicValuesAreRuntimeErrors/array element pointer load.
  • GOROOT xfail: removed only the covered fixedbugs/issue38496.go xfails for darwin/arm64 and linux/amd64.
  • Still out of scope / untouched: nil channel/goroutine output differences, chan/print/recover/GC/finalizer/liveness areas, and the previously paused xfails remain unchanged.

Local validation:

  • PASS: go test ./test/goroot -count=1 -run TestGoRootRunCases -v -args -goroot /Users/lijie/sdk/go1.26.0 -dirs fixedbugs -case '^fixedbugs/issue38496\.go$' -directive-mode runlike
  • PASS: go test ./test/go -count=1
  • PASS: go run ./cmd/llgo test -run TestNilPanicValuesAreRuntimeErrors ./test/go
  • PASS: go test ./ssa -count=1
  • PASS: go test ./cl -count=1
  • PASS/no tests: (cd runtime && go test ./internal/runtime -count=1)

Note: direct go test ./runtime/internal/runtime -count=1 from the repo root is still not a valid main-module package path; the runtime submodule command above is the runnable form.

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