fix(pclntab): preserve Darwin line sites with DWARF - #2157
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
e38170d to
d9bdbba
Compare
d9bdbba to
a0b796a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
a0b796a to
67e2ff5
Compare
67e2ff5 to
d4df865
Compare
92925e0 to
6252be4
Compare
There was a problem hiding this comment.
FennoAI Review — PR #2157 (DWARF/pcln runtime line sites)
Overall a careful, well-tested change. The moveInstr → moveInstrsAfter rewrite preserves stable relative order, DebugRefs correctly move with the values they describe (and are excluded from the moveWouldBreakSSA value set since *ssa.DebugRef is not an ssa.Value), the nonDebugReferrers conversion faithfully preserves the prior nil/length/identity checks, and the comment rewrites match the new gating. No correctness, security, or memory-safety defects found. A few items worth confirming, noted inline plus below.
Behavioral change to confirm (highest-impact item in this PR):
internal/build/build.go:532—passOptis nowshouldRunLLVMPasses(mode)(mode != ModeGen). The previousif emitDebugInfo || mode == ModeGen { passOpt = false }guard is gone, so debug/DWARF builds now run the full LLVM pass pipeline (RunPasses+VerifyModule) where they previously ran none. BecauseemitDebugInfoandOptLevelare independent, a normal DWARF-producingllgo buildstill defaults toO2, so the common debug path moves from no passes to fulldefault<O2>per package. This looks intentional (it pairs with the newSetDebugInfoOptimized(passOpt && OptLevel != optlevel.O0)atbuild.go:540and the "precise DWARF under optimization" goal), but it is a real per-package compile-time increase on the default path and is not obvious from the diff — please confirm it is the intended tradeoff. If a fast no-pass debug path at-O0is still desired, notedefault<O0>is still invoked (verify + minimal passes) rather than skipped.
Stale doc reference (adjacent to edited code):
internal/build/build.go:1195— therewritePrebuiltFuncTabcomment (whose body you edit just below) referencesdoc/design/pclntab-linkphase.md, which does not exist in the repo. The same dangling reference also appears inchore/pclnpost/main.go:19,internal/pclnpost/binary.go:18, anddoc/design/stage5-handoff.md. Consider restoring the doc or dropping the references.
Remaining inline notes are minor/latent.
6252be4 to
56e64ed
Compare
|
Review follow-up after rebasing onto merged #2143:
Focused tests and the dependent-stack full test suite pass. |
|
CI follow-up: the Go workflow and LLGo compatibility shard had the same compile failure: the new acceptance test referenced the obsolete private helper Verified with:
|
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Compared with |
Problem
Darwin builds with DWARF currently disable every compiler-emitted funcinfo site record. That avoids an LLDB lexical-scope regression from inline assembly at function entry, but also removes final-PC anchors used for precise runtime line tables.
runtime.Caller,runtime.CallersFrames, logging prefixes, and panic locations can then fall back to an earlier address.Function-entry address records and PC-line anchors have different debugger effects: entry records alter the initial line-table row, while a PC-line anchor is safe when its inline-assembly call has no debug location.
Changes
ssa.Builder.InlineAsmNoDebug, clearing only the emitted call location while preserving the builder location;-ldflags=-w=falseexecution coverage forruntime.Caller,runtime.CallersFrames, andlog.Lshortfile;This does not change pclntab mode selection, default
-w, runtime table format, or LLDB language behavior.Fixes #2115.
Dependency update
#2143 and #2215 are merged. This branch is rebased directly onto current
main.Verification
go test ./internal/build ./ssa ./cl;go test ./test/go -run 'TestDWARF.*PCLN|TestPCLN.*DWARF' -count=1;go test ./...passes on the dependent [Based on #2157] build: match cmd/link DWARF defaults #2142 stack;git diff --check.