cl compileFunc: cPubName, lookup manglingName - #707
Conversation
There was a problem hiding this comment.
Review: //go:linkname-based function binding generation
This PR wires up NameLookup to skip functions absent from archives, emits //go:linkname directives, adds the cPubName public-name rewriter and forceImportUnsafe, and removes large blocks of legacy commented-out code. The direction is clean and the legacy-code removal is a real readability win. A few robustness and portability items below.
Main themes
- Robustness:
nameLookupand two string operations (cPubName'sname[0],manglingName[1:]) can panic or misbehave on nil/empty inputs. Inline comments flag the specific spots. - Portability: the unconditional
manglingName[1:]underscore strip assumes the Darwin/Mach-O leading-underscore convention; on Linux/ELF it can drop a real symbol character. See inline. - Minor / non-blocking:
- New helpers
cPubName,getPubName,forceImportUnsafe(cl/blockctx.go) lack doc comments;cPubNamein particular encodes a user-observable naming rule (F,X_g,Xprintf) worth a one-line doc. lib/clang/clang.goreceives hand-reflowed doxygen comments. This file looks generated by llcppg's own binding generator (verbatim libclang docs +//go:linkname///llgo:typedirectives); if so, these cosmetic edits will be overwritten on regeneration and are best reverted.- Performance (low, codegen path):
fn.Mangling()(a cgo round-trip + string copy) now runs for every function declaration before thenameLookupskip filter, so it's paid even for the many system-header functions that get skipped. IfnameLookupcould key on the cheaper spelling,Mangling()could be deferred past the filter.m.Dispose()is correctly called on all paths — no leak.
- New helpers
| manglingName := c.GoString(m.CStr()) | ||
| m.Dispose() | ||
|
|
||
| if _, ok := ctx.nameLookup(manglingName); !ok { |
There was a problem hiding this comment.
[P1] nil NameLookup panics compileFunc for callers that omit it
ctx.nameLookup(manglingName) is called unconditionally, but nameLookup is copied verbatim from the exported Config.NameLookup field (compile.go:134), which has no default and is not validated in NewPackage/loadFile. The doc comment (compile.go:83-85) doesn't state the field is required. Any caller constructing a Config without setting NameLookup will hit a nil-function call and panic on the first FunctionDecl. Only the test mock sets it today. Suggest either guarding (if ctx.nameLookup == nil { ... }, treating nil as "always found" or skip), validating in NewPackage with a clear error, or at minimum documenting that NameLookup is mandatory.
| ctx.forceImportUnsafe() | ||
| f.SetComments(pkg, &ast.CommentGroup{ | ||
| List: []*ast.Comment{ | ||
| {Text: "\n//go:linkname " + fnName + " C." + manglingName[1:]}, |
There was a problem hiding this comment.
[P1] manglingName[1:] assumes a leading underscore (not portable to ELF)
"C." + manglingName[1:] unconditionally strips the first byte of the mangling name. This is correct only where clang prepends a leading underscore (Darwin/Mach-O, e.g. _g -> mangling __g -> C._g, matching the expected out.go). On Linux/ELF, C symbols typically have no leading underscore, so [1:] drops a real, significant character (e.g. xprintf -> printf), producing an incorrect linkname target. It also silently truncates single-char symbols. Suggest stripping the underscore conditionally (strings.TrimPrefix / check the prefix) or gating on the target object format, and guarding the empty-string case. A short comment explaining the convention would also prevent a future regression.
| } | ||
|
|
||
| func cPubName(name string) string { | ||
| if r := name[0]; 'a' <= r && r <= 'z' { |
There was a problem hiding this comment.
[P2] cPubName indexes name[0] with no empty-string guard
cPubName reads name[0] without checking for an empty string; name == "" panics with index-out-of-range. fnName originates from clang.String(fn) on parsed headers, so an anonymous/empty name (or an unexpected clang result) triggers the panic via getPubName. Cheap to guard: if name == "" { return name } at the top. (The ASCII byte-arithmetic uppercasing and the X prefix for leading _ are otherwise correct and match the expected output.)
No description provided.