Superseded by #2316: preserve nil checks for unused dereferences - #2256
Superseded by #2316: preserve nil checks for unused dereferences#2256cpunion wants to merge 1 commit into
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: preserve nil checks for unused dereferences
The core change is correct and well-tested: collapsing the large-value / slice-conversion special cases into a single unconditional guard path ensures _ = *p still panics on a nil pointer, and the added compiler-IR and runtime regression tests (TestUnusedDerefEmitsNilGuard, TestUnusedNilDerefOperationsPanic) lock in the behavior. The retained skipUnusedArrayDeref carve-out is a correct, spec-mandated exception (ranging over / len of a *[N]T must not dereference the pointer). No correctness, security, or documentation-accuracy issues were found — the removed CHECK lines in cvar/varinit and the two dropped issue38496.go xfails are consistent with the new behavior.
Two non-blocking quality/performance findings are noted inline. Both concern emitting nil guards that are provably dead in specific cases; neither affects correctness.
Optional follow-up: test coverage for the collapsed branches
The new tests cover a small direct pointer and an array-element load, but not the two cases whose dedicated handling was removed — an unused deref of a large non-pointer struct, and an unused zero-length slice-to-array-conversion deref. Adding those would lock in the merged branch's behavior for exactly the paths that were deleted.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
|
Handled both review findings in
Fresh validation passes: focused compiler/fixture tests, the LLGo runtime nil-deref regression, and upstream |
50c5332 to
3220195
Compare
cpunion
changed the title
|
Expanded and rebased this PR in
Direct nil pointer receivers remain legal and are covered by a control test. The final implementation also removes redundant checks on already-derived addresses. Fresh validation passes for the focused compiler suite, all new LLGo runtime cases, all affected LLVM snapshots, and upstream |
|
The first CI run exposed a conservative-GC interaction rather than a nil-check correctness failure: both Ubuntu LLGo matrix jobs failed the ESP32-C3 Fixed in
Fresh local validation passes: full |
|
The prior branch-only change fixed
With the exact Espressif QEMU setup used by CI, the complete |
|
Addressed the patch-coverage failure in |
|
Added focused fail-closed coverage in The previous Codecov report had 61 missed/partial lines in the new |
|
Codecov has now processed |
cpunion
added
the
go-test-compat
714f0ee to
a321a7e
Compare
a321a7e to
55a36cb
Compare
|
Full GOROOT regression passed at
All 16 shard jobs and the summary job passed. The first full run found one stale expectation: |
There was a problem hiding this comment.
Review: nil-pointer-dereference check rework
Overall this is a carefully written change. The new dataflow analysis is conservative by default — unknown aliases, calls, joins, and loops all stop every "known non-nil" proof — which is the right posture for a memory-safety-critical elision. A dedicated soundness pass over isKnownNonNilAt, mayAliasMemoryAddresses, localStoredValue, memoryUnchangedBetween, and equivalentPointerValues found no unsound elision paths: distinctness is only ever claimed between two different Alloc/Global roots, load equivalence requires both equivalent addresses and unchanged memory, and unsafe-pointer conversions correctly never earn a local root. The pre-boxed memoryError and the branch-based PanicNilDeref (avoiding a spill that could retain a dead object under a conservative collector) are both well-motivated.
Findings below are non-blocking. Inline comments carry the concrete diff-line items.
Summary of findings
- Dead production code:
isSafeDerivedAddressis referenced only by a test. (inline) - Doc drift:
isKnownNonNilAt's "only two classes of facts" comment omits address-based facts. (inline) - Compile-time perf: linear
instructionIndexrescans and a per-function O(N)nilDerefFactsscan can compound to super-linear cost on large functions. (inline) - Implicit coupling between
isNilDerefCoveredByAddressEvaluation(returnstrueunconditionally forIndexAddr) and theisArrayPointerGoType-guarded base check inemitNilDerefBaseCheck— the two must stay in sync but are enforced in different functions. (inline) - Asymmetric guard:
emitNilDerefBaseCheckskipsmethodReceiverBasesFieldAddr/IndexAddr, butassertNilDerefBase's FieldAddr arm has no equivalent guard. (inline)
Minor (no reliable inline anchor)
ssa/memory.go:AssertNilDerefBranchduplicates the early-return + nil-compare preamble ofAssertNilDeref(~lines 340-347 vs 355-362). Consider extracting a smallnilComparehelper so the two variants don't drift.cl/instr.goequivalentPointerValues: on revisiting aseenpair it returnstrue(assumes equivalence). SSA pointer expressions form a DAG so this branch should be unreachable, buttruefails unsafe if ever hit (asserts equivalence of possibly-distinct values → could suppress a needed check). A defensivefalsewould fail safe; worth a one-line justification either way.
| return false | ||
| } | ||
|
|
||
| func isSafeDerivedAddress(v ssa.Value) bool { |
There was a problem hiding this comment.
Dead production code. isSafeDerivedAddress has no caller in cl/compile.go or cl/instr.go — its only reference is cl/builtin_test.go:419. The production deref paths use isNilDerefCoveredByAddressEvaluation, isKnownNonNilAt, and localPointerRoot instead, and the Alloc/Global-root check here is already expressed inline in definitelyDistinctPointerRoots/mayAliasMemoryAddresses. Recommend deleting the function (and its test assertion), or wiring it into a real caller if it was meant to replace one of those inline checks.
| return false | ||
| } | ||
|
|
||
| // isKnownNonNilAt proves only two deliberately small classes of facts: |
There was a problem hiding this comment.
Doc drift. The comment says the function "proves only two deliberately small classes of facts," but the very first line returns true for a third, address-based class: isKnownNonNilAddr (fresh Alloc/Global/non-empty SliceToArrayPointer and their derivations) and isLocallyDerivedNonNil (FieldAddr/IndexAddr rooted at a known-non-nil base) — neither involves a load or a store. Suggest broadening the comment (e.g. "plus values whose address is inherently non-nil, such as allocations and globals") or dropping the "only two" framing.
| return xok && yok && xv == yv | ||
| } | ||
|
|
||
| func instructionIndex(block *ssa.BasicBlock, target ssa.Instruction) int { |
There was a problem hiding this comment.
Compile-time hot path. instructionIndex linearly scans block.Instrs (O(B)) on every call, and it's invoked from instructionDominates (once per fact in the hasDominatingNilDerefFact scan), localStoredValue (which then re-loops the block), and memoryUnchangedBetween (twice per call). Combined with the per-function O(N) nilDerefFacts scan, worst-case cost is super-linear (≈O(N²·B)) for large generated functions. Highest-leverage fix: build a per-block map[ssa.Instruction]int once per function and reuse it in these three callers — removes the B factor everywhere in a small, self-contained change. Correctness is fine today; this is purely about scaling.
| return p.hasDominatingNilDerefFact(v, instr) | ||
| } | ||
|
|
||
| func (p *context) hasDominatingNilDerefFact(v ssa.Value, instr ssa.Instruction) bool { |
There was a problem hiding this comment.
hasDominatingNilDerefFact linearly scans the entire p.nilDerefFacts slice on every query, and recordNilDerefFact appends one entry per emitted check, so this is ≈O(N²) fact comparisons per function (each comparison also runs instructionDominates + equivalentPointerValues). The slice is correctly reset/restored per function (compile.go:697, saved at 655/672), so it doesn't leak across the build — good. But for very large functions consider keying facts by block or by root value to bound this.
| return !isKnownNonNilAt(v.X, v) | ||
| } | ||
|
|
||
| func isNilDerefCoveredByAddressEvaluation(v ssa.Value) bool { |
There was a problem hiding this comment.
Implicit coupling. This returns true unconditionally for *ssa.IndexAddr (asserting the base is checked elsewhere), but the actual pointer-to-array base check lives in emitNilDerefBaseCheck's IndexAddr arm and only fires when isArrayPointerGoType(addr.X.Type()) is true. The two predicates are consistent today, but they're enforced in different functions — a future change to either could silently drop a required base check. Worth a cross-reference comment tying this true to the isArrayPointerGoType guard, or unifying the predicate.
| } | ||
|
|
||
| func (p *context) assertNilDerefBase(b llssa.Builder, addr ssa.Value) { | ||
| func (p *context) assertNilDerefBase(b llssa.Builder, addr ssa.Value, instr ssa.Instruction) { |
There was a problem hiding this comment.
Asymmetric methodReceiverBases guard. emitNilDerefBaseCheck early-returns for FieldAddr/IndexAddr bases found in p.methodReceiverBases (instr.go), but this assertNilDerefBase FieldAddr arm has no equivalent guard and will emit a NilDerefCheck + record a fact. If both routines can be reached for the same receiver-base FieldAddr, that's a redundant check the other path was written to avoid. Please confirm the two entry points are mutually exclusive for method-receiver bases; if not, add the same guard here.
83db0cc to
1373066
Compare
cpunion
changed the title
cpunion
changed the title
1 participant
Superseded by #2316, which contains the same load-only commit and validation on a clean pull request.