cmd: add LLGo-aware LLDB launcher with schema validation - #2211
Conversation
There was a problem hiding this comment.
Review summary
Solid, well-tested PR. The llgo lldb launcher is cleanly decomposed into small unit-tested functions, lldbImportCommand correctly escapes the temp plugin path, the temp plugin is written with 0600 under os.MkdirTemp, and the migration from comment-parsed expectations to an explicit TEST_CASES table + LLDB_BREAK markers (with the stricter GetNumLocations() != 1 breakpoint check) is a real robustness improvement. runtest.sh now exercises supported-v1, plain-C, and unknown-v2 marker states end to end.
No blocking issues. Findings below; two are inline.
Minor (not inline)
cmd/llgo/lldbtest/test.py:593— the__main__fallback default plugin path isgo_lldb_plugin.py, but the file isllgo_plugin.py. Pre-existing and off theruntest.shpath (which supplies the plugin viallgo lldb), so CI is unaffected, but this dead default becomes more misleading now that the plugin was relocated tocmd/internal/lldb/.- Usage strings diverge —
cmd/internal/lldb/lldb.go:48setsUsageLine: "llgo lldb [-lldb path] [--] executable [lldb arguments...]", whilecmd/llgo/lldb_cmd.gox:20registers the vagueruse "lldb [flags] executable [lldb arguments...]". The.goxline is the one users actually see (the base command is never dispatched directly), yet it omits the-lldbflag and the--passthrough convention the README documents. Consider aligning them. cmd/internal/lldb/llgo_plugin.py(inspect_target, ~L85) — a target carrying both a supported (v1) and an unknown (v2) marker collapses tosupported = Falsebecause a schema is resolved only whenlen(marker_versions) == 1. Defensible, but undocumented and untested; a short comment on the intent would help.cmd/internal/lldb/lldb.go:57-60— oncmd.Flag.ParseerrorrunCmdreturns without a non-zero exit, so a mis-invocation likellgo lldb --batch ./out(the exact--footgun the README warns about) exits 0. Matches sibling commands, so not a regression, but exiting non-zero here would be friendlier.
| if target.GetNumModules() == 0: | ||
| return () | ||
| module = target.GetModuleAtIndex(0) | ||
| for index in range(module.GetNumSymbols()): |
There was a problem hiding this comment.
_marker_versions walks the entire symbol table of module 0 (GetNumSymbols() × GetSymbolAtIndex().GetName() + regex) on top of the targeted FindSymbols lookups above. For a Go/LLGo binary that is an O(N) scan over tens of thousands of symbols, each iteration crossing the Python/C++ SB API boundary twice. This runs on a hot path: inspect_target → _require_supported_target is invoked on every llgo print / llgo vars / llgo status, and test.py builds a fresh LLDBDebugger per test case. Consider caching LLGoTargetInfo per target (it doesn't change during a session) and/or probing only the fixed __llgo_debugger_marker_v{n} names via FindSymbols instead of a full table walk.
Separately, the if target.GetNumModules() == 0: return () guard at L59-60 sits after versions is already populated from FindSymbols, so it would discard collected results; and the regex scan only inspects module 0, so an unknown marker in a non-first module would be missed while known versions still resolve — inconsistent detection. Moving the empty-target guard to the top and scanning all modules would make the two paths consistent.
There was a problem hiding this comment.
Addressed in 0f9fe48. inspect_target now caches by target properties plus the loaded-module fingerprint, so the full scan runs once per stable module set and is invalidated when modules change. Marker detection now scans every module consistently, and the integration suite verifies both a cache hit and the ambiguous v1+v2 case.
| 227 // all variables: globalInt globalStruct globalStructPtr s i err | ||
| 228 // s.i8: '\x12' | ||
| -> 225 println(s.i8) // LLDB_BREAK: main_struct_updated | ||
| (lldb) v |
There was a problem hiding this comment.
This example uses (lldb) v and shows the LLGo plugin's custom output (Go-typed names, []int{...} slice rendering, <variable not available>), but this same PR moved that function off the bare v alias to llgo vars and the prose just above (L43-44) now states p/v are left as stock LLDB commands. After this change, typing v invokes stock LLDB's frame variable (C framing, different output), not the plugin. The transcript should use (lldb) llgo vars to match the shown output.
There was a problem hiding this comment.
Addressed in 0f9fe48: the transcript now uses (lldb) llgo vars, matching the namespaced command and shown LLGo-specific output.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Addressed the remaining review notes in
Local macOS arm64 / Apple LLDB 1703 and Linux arm64 / LLDB 19 execution suites both pass 198/198 assertions; |
|
CI follow-up in |
|
Follow-up review findings are addressed in
Validation: macOS/arm64 Apple LLDB 1703 and Linux/amd64 LLVM LLDB 19 both pass 198/198 assertions plus marker/fallback checks; |
Summary
llgo lldb, which embeds the LLGo Python adapter and supports LLDB 18+ discovery,LLGO_LLDB,-lldb, and argument pass-throughDW_LANG_C, with the LLGo producer and__llgo_debugger_marker_v1selecting the adapterllgonamespace so stock LLDBpandvremain unchangedllgo statusfor the target triple, pointer size, and byte ordercmd/llgo/lldbtestand run it on primary macOS and Ubuntu CI jobsThis keeps LLGo support inside the LLGo repository and does not require
DW_LANG_Go, a patched LLDB, or private LLDB APIs.Validation
go test -cover ./cmd/internal/lldb ./cmd/llgo:cmd/internal/lldb87.3% on both locally tested platformsRefs #2154 and #2164. Follow-up #2206.