Repository navigation
build: integrate naked LLVM assembly and dynamic import libraries - #2623
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
zhouguangyuan0718
changed the title
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
aaa2e97 to
858ed14
Compare
zhouguangyuan0718
changed the title
zhouguangyuan0718
changed the title
|
/review Please review the current head 43ae165. The earlier review predates the removal of Go object parsing. Focus on correctness of the shared textual LLVM IR compilation path, native naked inline-asm ABI preservation, DATA binding and archive extraction, LTO, and dynamic import handling. Please report actionable findings against this head; this request is for review only, not code changes. |
There was a problem hiding this comment.
Review: native inline-asm compilation path (head 43ae165)
Reviewed the current head against base 3df0491. The Go-object parsing path is gone; this review covers the shared textual LLVM IR path, native naked inline-asm ABI preservation, DATA binding, dynamic import handling, and the darwin linker-flag translation.
Overall the design is sound. Module ownership is correct (native and non-native modules both reach mod.Dispose() at plan9asm.go:104, and the DATA-binding error path disposes before returning); externalizePlan9DataGlobals has solid guards (thread-local mismatch, size mismatch, and the "both Go initializer and DATA" conflict all reject before mutating the Go module); the native gate correctly skips lowerLargeAggregates/TransformModule so native carriers keep their physical-register ABI; and the darwin gating, dedup, and framework/dylib fallbacks are reasonable. Test coverage (LTO Off/Thin/Full, cross-compile object inspection, the Go 1.27 trailing-quote case) is strong.
A few findings below, the first being the most important.
Findings
[P1] Raw library paths flow unvalidated into linker args (cgo_pragmas.go) — In goCgoLinkArgs, any //go:cgo_import_dynamic library value that doesn't match the framework or /usr/lib/lib*.dylib shapes is appended verbatim to the link command. A value beginning with - or @ (e.g. -Wl,..., -force_load,..., @file) is interpreted by clang/ld as an option rather than an input file. Since these directives are read straight from package source, this is new linker-flag injection surface (Go's own toolchain guards the equivalent LDFLAGS path via an allowlist / CGO_LDFLAGS_ALLOW). Consider rejecting lib values starting with -/@ and constraining the raw-path branch to absolute existing .dylib/.tbd/.a paths.
[P2] collectGoCgoPragmas(pkg.Syntax) recomputed per .s file — translateForeignNativeAsm re-scans every Go AST file/comment and rebuilds the imports map on each iteration of the compilePkgSFiles loop, though its input and output are package-invariant. For packages with many .s and .go files this is O(sfiles × gofiles) redundant comment scanning plus per-iteration allocations. shouldCheckDarwinDynimportTrampolineAsm already computes this once per package; consider hoisting/memoizing (matching the existing plan9asmSigs cache pattern). Bounded by the SupportsNativeTarget short-circuit, so darwin-only.
[P3] splitDirectiveArgs does not handle backslash-escaped quotes — The manual scanner treats every " as a field boundary regardless of a preceding \. For input like "a\"b" it mis-splits before strconv.Unquote runs. The comment claims to "match cmd/compile's pragma field boundaries," and cmd/compile respects backslash escapes. Real cgo directives don't contain escaped quotes, so impact is low, but consider skipping \-escaped quotes inside a quoted region (or documenting the divergence with a test).
[P3] Supply-chain: fork pin in go.mod — replace github.com/xgo-dev/plan9asm => github.com/zhouguangyuan0718/plan9asm v0.2.2-... redirects the asm→LLVM backend (high-trust, emitted into user binaries) to a personal fork by pseudo-version pending PR #42. Fine as a temporary pin (covered by go.sum), but track moving it back to an org-owned tagged release, and note the pinned v0.2.2-... is lower than the required xgo-dev/plan9asm v0.5.3.
| name := strings.TrimSuffix(strings.TrimPrefix(path.Base(lib), "lib"), ".dylib") | ||
| ldflags = append(ldflags, "-l"+name) | ||
| } else { | ||
| ldflags = append(ldflags, lib) |
There was a problem hiding this comment.
Linker-flag injection via raw library path. Any //go:cgo_import_dynamic library that doesn't match the framework or /usr/lib/lib*.dylib shapes is appended verbatim here. A value beginning with - or @ (e.g. -Wl,..., -force_load,..., @file) is treated by clang/ld as an option, not an input file. These directives come from package source, so this is new linker-flag injection surface — Go's toolchain guards the equivalent LDFLAGS path with an allowlist. Suggest rejecting values starting with -/@ and constraining this branch to absolute existing .dylib/.tbd/.a paths.
There was a problem hiding this comment.
Fixed in 95e0da8: the dynamic-import library field now rejects leading - and @, and propagates the diagnostic from both ordinary and alternate packages. Added regression cases for linker-option/response-file values and valid relative, literal ./@, and space-containing library paths. Kept SDK-based framework/dylib resolution and did not require files to exist on the host: system dylibs may only be available through SDK stubs. Focused native/Plan 9/dynamic-import tests, including real callback links with LTO off/Thin/Full, pass.
| if !extplan9asm.SupportsNativeTarget(ctx.buildConf.Goos, ctx.buildConf.Goarch) { | ||
| return llvm.Module{}, false, nil | ||
| } | ||
| _, decls := collectGoCgoPragmas(pkg.Syntax) |
There was a problem hiding this comment.
collectGoCgoPragmas(pkg.Syntax) (and the imports map built below) is recomputed on every .s file in the compilePkgSFiles loop, but its input and output are package-invariant. For packages with many .s/.go files this is O(sfiles × gofiles) redundant AST/comment scanning plus repeated allocations. shouldCheckDarwinDynimportTrampolineAsm already computes this once per package; consider hoisting or memoizing (matching the plan9asmSigs cache pattern). Bounded by the SupportsNativeTarget short-circuit, so darwin-only.
There was a problem hiding this comment.
The repeated per-file scan is real, but is a non-blocking optimization for these small callback files; keeping the focused correctness fix separate. SupportsNativeTarget covers both Linux and Darwin (amd64/arm64), so it is not Darwin-only. The personal-fork dependency is an immutable go.sum-verified pin explicitly pending plan9asm #42; the go.mod comment tracks removal once the upstream change is available.
|
|
||
| func splitDirectiveArgs(s string) []string { | ||
| fields := strings.Fields(strings.TrimSpace(s)) | ||
| // Match cmd/compile's pragma field boundaries: quoted regions are single |
There was a problem hiding this comment.
The comment claims to "Match cmd/compile's pragma field boundaries," but the scanner treats every " as a field boundary regardless of a preceding backslash, so a backslash-escaped quote (e.g. "a\"b") mis-splits before strconv.Unquote runs. cmd/compile respects backslash escapes. Real cgo directives don't contain escaped quotes so impact is low; consider skipping \-escaped quotes inside a quoted region, or documenting the divergence with a test.
There was a problem hiding this comment.
Checked this against Go 1.27.0: pragmaFields also toggles its quoted state on every double quote and does not special-case backslashes. See https://github.com/golang/go/blob/go1.27.0/src/cmd/compile/internal/noder/lex.go#L151 . The field-boundary behavior here is intentional, including ignoring an unmatched trailing quote (needed by runtime/sys_darwin.go). I am keeping compatibility with that parser rather than introducing different escaped-quote boundaries.
|
Confirmed on Linux/amd64 at this PR's head ( Minimal reproducer (paths below are inside the Linux container):
module example.com/llgo-dynimport-repro
go 1.27.0
package main
//go:cgo_import_dynamic imported answer "/repro/libprobe.so"
var entry uintptr
func main() { println(entry) }
#include "textflag.h"
TEXT bridge<>(SB), NOSPLIT, $0
JMP imported(SB)
GLOBL ·entry(SB), RODATA, $8
DATA ·entry(SB)/8, $bridge<>(SB)
int answer(void) { return 42; }From I also checked the cause with a control build: a separate imported cgo package supplying Update: I repeated the failing build with a |
3 participants
TypeScript Go tsc's FSEvents backend contains raw C callbacks and register-shuffling assembly without Go declarations. Translate these bounded native entries to
naked noinlineLLVM functions with function-local inline asm. Both native and ordinary Plan 9 translation feed the same textual.lloutput, configured IR compiler, object/bitcode, archive and final-link pipeline. Native carriers retain their physical-register interface and bypass signature-based aggregate/C ABI rewrites.Honor libraries declared by
go:cgo_import_dynamicon both Darwin and Linux. Darwin frameworks/system dylibs resolve through SDK linker options. Linux preserves explicit library paths and uses exact-name ELF library lookup for bare names, including versioned SONAMEs. Reject library fields beginning with-or@, validate assembly DATA against Go storage, and preserve quoted directive arguments. Windows and WebAssembly retain their existing import handling. This fixes the review reproducer where Linux compiled native assembly but omitted the shared library at final link.Supported native targets are Linux and Darwin on amd64/arm64. Entries manage their own native frames, arguments and results; no unknown Go/C prototype is inferred. Use upstream plan9asm v0.6.1, which includes merged xgo-dev/plan9asm#42; no fork replacement is needed. No Go assembler object dependency is introduced. Unsupported forms fail explicitly.
Regression coverage:
undefined symbol: answerwith the old library handling and pass with the fix in every tested path/LTO combination. Focused native/Plan 9/dynamic-import tests pass on both Linux architectures and Darwin/arm64.Assembly instructions remain opaque to IR optimization. Go ABI adapters, runtime transitions and unwind metadata are outside this contract. Full Linux tsc builds and the recorded watch-mode issue are not established by these tests.