debug: keep optimization and SSA rewrites DWARF-safe - #2143
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
8817c9d to
6e8d80a
Compare
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
31a3bbf to
d4f4386
Compare
1955174 to
7c57a58
Compare
04c6326 to
822efaa
Compare
There was a problem hiding this comment.
Review Summary
Solid, well-tested change. The core intent — running LLVM optimization passes even when DWARF is emitted, and making SSA reordering / static-init rewrites ignore DebugRef pseudo-instructions so enabling debug info never alters lowering decisions — is implemented correctly and thoroughly covered.
I verified the areas most likely to hide bugs and found them sound:
moveInstrsAfter(internal/build/ssa_order_fix.go) preserves the old "move the load right after the last call" semantics. The anchor is matched by identity, so removing the load first still lands the group immediately after the last call; the moved group keeps stable relative order; and all edge cases (emptymoving, nil anchor, anchor not found) return the input unchanged.includeDebugRefsForMovedValuescorrectly bounds its scan bythrough(==recvIdx) and runs beforemoveWouldBreakSSA, which skips already-moved indices — so aDebugRefusing a moved value is carried along rather than falsely reported as an SSA break.DebugRefs produce no value, so they never chain into the moved-value set.nonDebugReferrers(cl/static_init.go/cl/ssa_referrers.go) is behavior-preserving for the non-debug path:(nil, false)exactly whenReferrers()is nil, and identical to the old filtered list when noDebugRefs are present.deferInitBuilder(ssa/eh.go) guards the location copy behindp.diFunc != niland a non-nilloc.Scope, so an unset location is safely skipped.- Performance: the new allocations in
moveInstrsAfterandnonDebugReferrerssit on narrowly-gated, rarely-triggered recognition/repair paths and stay within the same asymptotic complexity as the code they replace — no meaningful compile-time regression. - Security: the new
test/go/dwarf_semantics_acceptance_test.gouses a static hardcoded arg list, a compile-time-constant temp file at0o600, and no untrusted input — no injection or unsafe temp-file concerns.
One minor doc-accuracy finding is inline. A minor naming nit (non-blocking): test/go/dwarf_semantics_acceptance_test.go reuses the helper findStringConversionRepoRoot, whose StringConversion-specific name is now misleading as a shared repo-root finder — consider renaming to findRepoRoot in a follow-up.
e792edd to
014b06d
Compare
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
There was a problem hiding this comment.
Review summary
This PR makes the SSA order-fix and static-init pipeline debug-info aware so that DebugRef metadata no longer blocks otherwise-safe source-order repairs, plus decouples DWARF Optimized marking from pass selection. The core approach is sound and well-tested (per-mode default/global-debug variants, an end-to-end acceptance test, and a crypto/x509.ParseOID regression). All four review passes (quality, performance, security, docs) found no blocking correctness, security, or performance issues. The notes below are low-severity maintainability/precision items.
Maintainability
internal/build/ssa_order_fix.go— the "move DebugRefs along with the value they describe" logic now exists in two forms: theincludeDebugRefsForMovedValueshelper (index-set driven, used bymoveAssignDepsAfterRecv) and the inline loop infixSSAOrderBlock(instruction-set driven). Both are correct but duplicative with different data-structure conventions; consolidating onto one helper would reduce future drift.internal/build/ssa_order_fix.go—includeDebugRefsForMovedValuesandmoveWouldBreakSSArun back-to-back inmoveAssignDepsAfterRecvand each independently rebuild the samemovedset by scanningmove. The second rebuild is required (the first mutatesmove), but the duplicated prefix scan is avoidable. Low impact — this path is gated behind the rare single-case-selectreceive-assign pattern.
Doc precision
internal/build/ssa_order_fix.go:24— thefixSSAOrderdoc comment says it moves "loads of local allocs used only for the final Return results," butfixSSAOrderBlockactually only checks that there is no intervening executable use between the load and the Return (DebugRefs are allowed and moved along). A load also referenced elsewhere without an executable use in the load→Return window would still move. Consider wording closer to "loads that feed a Return result and have no intervening executable use before that Return."
Reviewed against 8dbfab5...1470ef8.
29ba321 to
17ac3d3
Compare
This is the consolidated DWARF-safe optimization and SSA-rewrite change.
The effective implementation from #2183
(return-load ordering) and #2184
(single-case-select ordering) is included here exactly once. Those PRs are no
longer dependencies of this change; their unrelated CI-only commits are not
carried over.
Problem
DWARF must not change executable semantics. Enabling the normal optimization pipeline for debug builds exposed four places where debug metadata affected code generation:
!dbg, so full LTO verification aborted;ssa.DebugRefusers blocked return-load and single-case-select order repairs;ssa.DebugRefusers blocked valid static global slice folding.Full LTO with DWARF also exposes aggregate Go strings to the reflect
MethodByNameplugin. #2159, now inmain, owns that boundary; this PR contains no LTO plugin implementation changes.Changes
ModeGenas raw generated IR;ssa.DebugRefusers with repaired return loads and select dependencies, while rejecting executable early uses;DebugRefpseudo-users when proving a static slice initializer.This consolidates the effective changes from #2144, #2146, #2183, and #2184. It does not change the default
-wpolicy, Python debugging, pclntab policy, C ABI variable homes, LLDB language behavior, or non-debug optimization policy.Fixes #2118.
Fixes #2121.
Fixes #2122.
Verification
internal/buildSSA-order tests pass in both default andssa.GlobalDebugmodes;TestDWARFReturnOrderSemanticspasses with explicit-w=false;ssa,internal/build, and LTO/DWARFcltests pass on macOS arm64;git diff --checkpasses.