Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
cpunion
marked this pull request as ready for review
August 3, 2026 01:24
Collaborator
Author
|
Dependency audit: this root PR has no cpunion predecessor. xgo-dev/main already contains the relevant baseline PRs #2141 (DWARF foundation), #2211 (LLDB launcher/schema), and #2240 (strings/slices runtime views); none are duplicated here. The next cpunion layer (#112) depends on this PR and will not be submitted upstream until this root layer is accepted. |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review summary
This is a clean, well-tested refactor of DWARF debug-info handling. Replacing the boolean AlwaysOmit with a typed DebugInfoCapability + CanRetain() and splitting PreserveLinkFlags/OmitLinkFlags reads clearly, and the change is backed by thorough unit tests plus a new end-to-end Cortex-M ELF test. No correctness regressions found across code-quality, performance, security, and documentation passes.
Confirmed as correct-by-design:
- The
-Svs-Wl,-Ssplit is intentional: theuse()wasm/native path links through the clang driver (-Wl,-S), whiletargetDebugInfoPolicy(viaUseTarget, directld.lld/wasm-ld) uses bare-S. - Removing the unconditional
-SfromUseTargetldflags is safe: omission still flows throughOmitDWARFByDefault→dwarfLinkerArgs→["-S"]for retainable targets, covered by thefixed target wtest. - No security concerns: flags are hardcoded constants appended as argv elements (no shell/injection surface); test
exec.Commandcalls use fixed tool names guarded byLookPath.
The findings below are all low-severity maintainability/documentation notes, left inline.
39 tasks
cpunion
marked this pull request as draft
August 3, 2026 02:50
cpunion
force-pushed
the
codex/debug-artifact-policy-2164-20260801
branch
from
August 9, 2026 19:21
c9c9171 to
a16528c
Compare
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
5 of 11 tasks
cpunion
changed the title
cpunion
force-pushed
the
codex/debug-artifact-policy-2164-20260801
branch
from
September 30, 2026 21:43
a16528c to
664b553
Compare
cpunion
marked this pull request as ready for review
September 30, 2026 21:44
cpunion
force-pushed
the
codex/debug-artifact-policy-2164-20260801
branch
from
September 30, 2026 21:50
664b553 to
fb8eda4
Compare
This was referenced Sep 30, 2026
cpunion
force-pushed
the
codex/debug-artifact-policy-2164-20260801
branch
from
September 30, 2026 22:43
421e68e to
b486675
Compare
4 of 7 tasks
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
cpunion
force-pushed
the
codex/debug-artifact-policy-2164-20260801
branch
3 times, most recently
from
October 1, 2026 09:27
7d0081e to
f34d646
Compare
cpunion
force-pushed
the
codex/debug-artifact-policy-2164-20260801
branch
3 times, most recently
from
October 1, 2026 19:01
7355101 to
001190d
Compare
cpunion
force-pushed
the
codex/debug-artifact-policy-2164-20260801
branch
from
October 1, 2026 19:32
57db254 to
c0263bf
Compare
visualfc
approved these changes
Oct 2, 2026
visualfc
reviewed
Oct 2, 2026
visualfc
left a comment
visualfc
left a comment
Collaborator
There was a problem hiding this comment.
Review
Artifact roles, llgo debug DWARF/-O0 policy, host-ELF vs flash images, and Wasm sidecar pairing look solid. Default target builds still omit DWARF, so hello-world size is unchanged.
P2 items below can break a production QEMU/OpenOCD session or Windows Wasm rewrite. CI currently hides the empty-probe case behind qemu_load_proxy.py.
| } | ||
| connection, err := net.DialTimeout("tcp", address, 100*time.Millisecond) | ||
| if err == nil { | ||
| connection.Close() |
Collaborator
There was a problem hiding this comment.
[P2] Empty TCP probe can consume the GDB stub connection
waitReady dials the gdb port, sends no RSP, and closes. test/debug/embedded/qemu_load_proxy.py exists specifically for this (llgo probes readiness without sending an RSP packet and skips an empty first recv).
Production llgo debug -target=cortex-m-qemu talks to QEMU -gdb tcp: directly, without that proxy. Many stubs accept one client; this probe can take the only slot or reset the stub before GDB/LLDB attach.
Prefer a ready signal that is not the gdb port, or keep the probe from acting as a GDB client.
| } | ||
|
|
||
| func freeTCPPort() (int, error) { | ||
| listener, err := net.Listen("tcp", "127.0.0.1:0") |
Collaborator
There was a problem hiding this comment.
[P2] Port is released before QEMU/OpenOCD bind
freeTCPPort listens on :0, closes, then the port is interpolated into gdb_port / -gdb tcp:127.0.0.1:{debug-port}. Another process can take the port in between, so debug-server startup fails intermittently.
Have the server pick the port and parse it from logs, or pass a still-open listener.
| if err := tmp.Close(); err != nil { | ||
| return err | ||
| } | ||
| return os.Rename(tmpPath, path) |
Collaborator
There was a problem hiding this comment.
[P2] os.Rename cannot replace an existing file on Windows
This helper rewrites the live .wasm (and the sidecar). Unix rename replaces the destination; Windows os.Rename fails when path already exists. external/embedded Wasm packaging will error on Windows after the first write of modulePath.
Remove path before rename, or fall back when Rename fails with os.ErrExist.
| goos, goarch, llvmTarget = target.GOOS, target.GOARCH, target.LLVMTarget | ||
| } | ||
| if goarch == "wasm" || strings.HasPrefix(llvmTarget, "wasm") { | ||
| if goos == "js" || strings.HasPrefix(conf.Target, "wasm") { |
Collaborator
There was a problem hiding this comment.
[P3] Target names starting with wasm are classified as browser
strings.HasPrefix(conf.Target, "wasm") sends wasm-unknown (and any WASI target named wasm*) to backendBrowser, which then errors as unimplemented. The unit test uses Target: "wasm" with GOOS: "js", so the prefix check is what decides browser.
Classify from GOOS == "js" / the Wasm profile instead of the target name prefix.
|
|
||
| // Cmd is the llgo debug command. | ||
| var Cmd = &base.Command{ | ||
| UsageLine: "llgo debug [-backend auto|lldb|gdb|wasmtime|browser] [-target platform] [build flags] [package] [-- debugger arguments...]", |
Collaborator
There was a problem hiding this comment.
[P3] Help lists wasmtime/browser backends that immediately error
UsageLine advertises -backend auto|lldb|gdb|wasmtime|browser. Selecting wasmtime or browser returns not available yet. Docs call them reserved; the flag help still looks implemented. cmd/llgo/debug_cmd.gox repeats the same usage string.
Keep auto/lldb/gdb in UsageLine, and mention the reserved backends in the long help.
| func EnsureBuildID(module []byte) ([]byte, []byte, error) { | ||
| if id, ok, err := BuildID(module); err != nil { | ||
| return nil, nil, err | ||
| } else if ok { |
Collaborator
There was a problem hiding this comment.
[P3] Existing build_id does not cover the debugger ABI section
finalizeDebugArtifact runs SetDebuggerRecord then EnsureBuildID. If the linker already emitted build_id, this branch keeps it, so the id does not hash the ABI record that was just added. When no id is present, SHA-256 is over the full module, including that record.
Sidecar pairing still matches (same bytes on both modules). Stale-sidecar checks that treat build_id as a content identity will not see ABI-section edits. Recompute, or hash after the ABI write.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add
llgo debugand explicit debug-artifact policies for native, embedded and Wasm builds.-debug-artifact=embedded|external|host|none, option validation, sidecar cleanup and separate debug/deployment/runtime-symbol size reporting. Native macOS/Linux/Windows retain platform-toolchain DWARF packaging.-load.external_debug_info, preserving executable sections and Emscripten glue.Validation: artifact/session regression tests and all PR checks pass. The embedded CI matrix covers Linux/macOS/Windows × GDB/LLDB; QEMU acceptance checks image loading, source breakpoints, variables, backtraces, DWARF validity and unchanged flash bytes.
Physical hardware has not been tested. Native dSYM/debug-file generation is outside this PR's scope.