plan9asm: support Go aliases and undeclared trampolines#21
plan9asm: support Go aliases and undeclared trampolines#21zhouguangyuan0718 wants to merge 5 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
This PR relaxes two previously-fatal "missing Go declaration" errors into best-effort FuncSig{Ret: Void} signatures, and adds *types.Alias lowering (via a reflect probe for Go 1.21 compatibility). The alias support and the localSigs two-pass design are clean, and ManualSig precedence is correctly preserved. Good test coverage was added for the headline scenarios.
The main concern is soundness of the widened Void fallback in addReferencedFuncSigs: it applies to every unresolved referenced symbol, not just the opaque C symbols described in the comment. See inline notes.
Findings
go_translate.go:270-274— UnconditionalVoidfallback for referenced symbols downgrades a hard error into a silently-wrong zero-arg/void call (inline).go_translate.go:186-191 / 224—Voidfallback for<>locals that may take args; redundant lazy init oflocalSigs(inline).go_translate.go:538—reflect.TypeOfon every call; can be moved to thedefaultbranch (inline).
No security (injection/authz/secret) issues in the diff. Documentation-accuracy notes about README.md/GOARCH mentioning arm were pre-existing and outside this diff, so omitted.
| // assembly without a Go declaration. Their ABI is deliberately opaque | ||
| // to the Go type checker; keep a conservative declaration so the | ||
| // backend can still emit the trampoline. A ManualSig takes precedence. | ||
| b.sigs[targetResolved] = FuncSig{Name: targetResolved, Ret: Void} |
There was a problem hiding this comment.
The old code skipped (continue) any referenced-but-unresolved symbol, so a non-tail CALL foo(SB) to an undeclared symbol failed loudly downstream (amd64_lower_branch.go:266 "amd64 call missing signature"). This else branch now assigns FuncSig{Ret: Void} (zero args) to any such symbol, not only cgo_import_dynamic/opaque-C symbols as the comment implies.
Consequence: a genuine CALL realfunc(SB) whose callee takes register args / returns a value, but has no discoverable Go declaration, now lowers as a zero-arg void call — register arguments are dropped and the return is not captured, i.e. a silently-wrong call instead of a diagnostic (a typo'd or forgotten decl no longer fails fast).
Suggest scoping this fallback to the intended cases (tail-jumps / recognizably-opaque C symbols) and letting genuine unresolved intra-ABI CALLs keep erroring — or at minimum surfacing the synthesized-Void symbols so callers can tell "intentional trampoline" from "missing decl."
| obj := b.scope.Lookup(declName) | ||
| if obj == nil { | ||
| if strings.HasSuffix(textSym, "<>") { | ||
| if b.localSigs == nil { |
There was a problem hiding this comment.
Two notes on this block and the related post-pass:
-
Redundant lazy init.
b.localSigsis already unconditionally initialized ingoSigsForAsmFile(localSigs: make(map[string]bool)), the only production caller. This nil-check + lazymakeonly fires for hand-builtgoSigBuilders in tests, creating split-brain initialization. Prefer a single source of truth (drop the lazy init and init in the test, mirroring howsigsis handled). -
Void for
<>locals that take args (post-pass at lines 186-191): a file-local<>TEXT symbol not inferred via a tail jump getsFuncSig{Ret: Void}. That sig is used to emit the function definition header too, so a<>helper that actually reads argument/frame registers is defined with an empty param frame — arg/frame loads resolve against nothing. The comment states these are "only reached through a raw function pointer," but the relaxation is unconditional;ManualSigis the only escape and no diagnostic tells the caller one is needed.
| // module. Detect it by its stable reflect identity so the package still | ||
| // compiles with Go 1.21, while newer go/types implementations can lower an | ||
| // alias through its RHS just like a named type. | ||
| if rt := reflect.TypeOf(t); rt != nil && rt.Kind() == reflect.Ptr && |
There was a problem hiding this comment.
reflect.TypeOf(t) plus the rt.Elem() string comparisons run on every call to this recursive, per-type hot-path function, including the common *types.Basic/*types.Pointer/*types.Named cases. Since *types.Alias is a distinct concrete type it never matches an existing case and would otherwise fall to default, so this probe can be moved into the default branch — behavior-identical, but the reflect + string compares no longer run for the types the switch already recognizes.
1 participant
go/typescan materialize ordinary aliases as*types.Alias, and macOS/CGO assembly commonly defines file-local<>trampolines without Go declarations.This change makes the Go-aware Plan 9 asm signature pass lower aliases, retain conservative signatures for undeclared local trampolines, and declare opaque external asm branch targets. The behavior is generic and is not coupled to a package path or a specific architecture; existing manual signatures and tail-jump inference remain authoritative where available.
Validation:
GOWORK=off GOCACHE=/tmp/plan9asm-pr-tsgo-cache go test ./...GOWORK=off GOCACHE=/tmp/plan9asm-pr-tsgo-cover-cache go test -coverprofile=/tmp/plan9asm-pr-tsgo-cover.out ./...cmd/plan9asmscan: 87.8%