debug: consolidate native DWARF correctness and defaults - #2142
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
4141240 to
f227cd8
Compare
abf8473 to
703cae6
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
adf396b to
9aef2de
Compare
723ec11 to
8211f5a
Compare
8e466a2 to
e00bb4f
Compare
There was a problem hiding this comment.
Review: match cmd/link DWARF defaults
This is a well-structured change. Making DWARF preserved by default is cleanly factored in internal/build/link_options.go, the -s implies -w precedence is order-independent and matches cmd/link, and the ripple effect of DWARF now being on by default (SSA DebugRefs present) is handled consistently through nonDebugReferrers and explicit *ssa.DebugRef handling. Test coverage of the new policy branches is thorough. Security is fine — the acceptance tests use t.TempDir() and constant source templates, and defaulting DWARF on matches Go's own cmd/link behavior.
A few findings below; the inline comment is the only code-level item, the rest are stale docs.
Findings
1. Stale design doc — DWARF default is now inverted
doc/design/pclntab-packaging.md:28-64 still documents the removed policy: "LLGo temporarily omits DWARF by default … LLGo currently behaves as if -w=true", ModeGen framing, and "Broad CI uses the DWARF-free default". After this PR, linked builds emit DWARF unless -w/-s is passed (only Darwin c-shared defaults to omit). This section should be rewritten to the Go-standard default; otherwise the primary design doc for this feature contradicts the shipped behavior.
2. Stale design doc — Darwin builds are no longer "site-free"
doc/design/pclntab-packaging.md:138-142 still says "Darwin embedded builds that emit DWARF keep the historical site-free path". This PR splits site emission: shouldEmitRuntimePCLineSites no longer consults DWARF (Darwin DWARF builds now do emit PC-line sites), and only shouldEmitRuntimeAddressSites suppresses entry/stub address sites on embedded Darwin DWARF. The in-code comments (build.go, funcinfo_table.go) are correct; only this doc is stale. Update it to say PC-line sites are retained and only address sites are suppressed.
See the inline comment for the one code-level note.
d6ca262 to
06891a2
Compare
|
Review follow-up after restacking on the updated #2157:
The focused tests and |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
06891a2 to
3cafd6b
Compare
5cb4ceb to
a7bcb45
Compare
There was a problem hiding this comment.
Review: DWARF default Go-compatible
Solid PR. The DWARF-by-default / stable-parameter-home / PC-line-site policy changes are well-factored and come with strong test coverage (native LLDB acceptance suite, TestNativeDWARFDefaults, TestDebugParameterHomes, TestParamHomeReusePreservesDebugStorage, and the pcln/link-option tables). The refactors are net improvements — the constructDebugAddr change drops a dead per-variable Load, and collectDebugAllocObjects replaces an O(n) linear scan (hasDebugAlloc) with an O(1) set lookup on the compile hot path.
No correctness or security blockers found. Inline notes below are minor. A few non-inline maintainability suggestions:
ssa/di.go—DIParamvsDIParamWithHome: the two exported methods now delegate to the samediParamand differ only in whether the homeExpris returned/discarded. Consider collapsing to one method (callers can ignore the result) to prevent drift.cl/compile.godebugRef: the value-extraction block (v.X.(instrOrValue)→p.bvals[iv]/p.compileValue) is now duplicated across the stable-param and existing branches. Extracting a small helper would keep the two from diverging.cl/debug_alloc.gohasDebugAlloc: after this PR its only remaining caller is the test file (debugParamsnow usesdebugAllocObjects). Consider removing it or marking it test-only so it doesn't read as leftover code.internal/build/link_options.goomitDWARFRequested: the four-level nested boolean (fixed target / wasm / darwin c-shared) is hard to audit; a nameddefaultsToOmitDWARF(conf)helper or a per-clause comment mapping to the design-doc rationale would help.cmd/llgo/debugtest/native/runtest.sh: paths ($script_dir, artifacts) are string-interpolated into LLDBscript/command script importargs, which is an eval boundary. Safe for CI-controlled paths, but passing them via env vars read inacceptance.pywould be more robust. Not blocking.
|
Addressed the five suggestions in the review body in 5955a9c86:
Validation: the existing CL debug-metadata/alloca, SSA parameter/map-location, and build DWARF/default/linker-policy tests pass. Rebuilt the compiler and reran the real native panic/divide/nil plus O2 inline-frame/step-over acceptance with both the source path and temporary artifact directory containing spaces, single quotes, and double quotes; it reports |
|
Fixed the Windows ARM64 MSVC The generated Lowering now removes the replaced global and its package/debug indexes. The operation is idempotent for alternate cgo source passes and protects shared zero-sized aliases. Real function addresses and DWARF remain enabled. Validation:
The new Windows CI run is still required to confirm the original platform end to end. No test or DWARF generation was disabled. |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
visualfc
left a comment
There was a problem hiding this comment.
Review
The native DWARF default, O0 parameter homes, Darwin PC-line sites, cgo COFF cleanup, and test isolation look solid. A few issues around default-path cost and cgo revisit handling:
- P2
reuseParamHomeis skipped for every function with a subprogram. Native DWARF is now default, so optimized builds lose param-home reuse too. - P2 cgo's
__cgo_*fallback stillNewFuncs when the placeholder is already gone. - P3 alloc-object
DebugRefs are suppressed only at O0. - P3 native panic/inline acceptance is not run on Windows CI.
Inline notes below.
Native builds omit DWARF by default, and explicit debug builds can lose current parameter/local values or reliable Darwin source locations. This PR preserves native DWARF by default and fixes those correctness issues while keeping optimized C ABI parameter-home reuse.
Based directly on main
f06143ba2, with no unmerged prerequisites. Implements the native DWARF part of #2154 and #2164; native packaging follows the platform toolchain.-w/-w=falsetakes precedence; ordinary Wasm/embedded defaults remain unchanged.test/debug/native, including exact panic/divide/nil caller locations, O2 inline frames/step-over, and current aggregate local/parameter values after pointer mutation.Consolidates #2235, #2148, #2157 and #2202 plus the relevant native acceptance from cpunion#140.
Fixes #2206.
Fixes #2115.
Fixes #2119.
Validation for
08d0aef2e:The dedicated panic/inline suite is currently a Linux/macOS CI gate. Windows retains runtime LLDB acceptance and
TestDWARFPCLNLineSites, but those do not establish the separate panic/inline guarantees; this limit is documented in the native suite README.