refactor: keep existing function directives on declaration objects - #2572
zhouguangyuan0718 wants to merge 15 commits into
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Source-level function contracts
This is a large, well-engineered feature adding //llgo:attribute source contracts that lower to LLVM attributes and llvm.assume facts, surviving large-aggregate and C-ABI transformations. The source model is cleanly separated from the LLVM encoding, test coverage is broad (parsing, ABI survival across amd64/arm64/386/wasm, invoke edges, JSON round-trips, optimization folding), and doc/Function-attributes.md is accurate against the parser and validator (verified selector-by-selector, including the prototype-spelling normalizations).
The findings below are refinements, not correctness bugs I can prove. The most notable is a build-reproducibility concern from map-order IR emission.
Positive observations
- Careful soundness handling: signed-interval
rangelowered via subtraction in source width; large-array GEP uses target-pointer-width indices to avoid a valid 64-bit index going negative; invoke facts split onto a normal edge with PHI repair. Mergedeterministically sorts and deep-clones mutable operands, with a test proving callers can't mutate merged state.- Declaration-vs-definition strength is guarded: imported declarations are widened uniformly under GC roots / cooperative safepoints, and unsupported signature-changing invoke conversions are rejected rather than silently mis-attributed.
- Docs and code comments match the implementation; no material documentation discrepancies found.
| for fn, plan := range plans { | ||
| if fn.IsDeclaration() { | ||
| continue | ||
| } | ||
| b.SetInsertPointBefore(fn.EntryBasicBlock().FirstInstruction()) | ||
| for _, contract := range plan.Contracts { | ||
| if contract.Target.Parameter >= 0 { | ||
| value := extractValue(b, fn.Param(contract.Target.Parameter), contract.Target.Path) | ||
| emitValueFact(b, value, contract, plan.PointerBits) | ||
| } | ||
| } |
There was a problem hiding this comment.
[P2] Nondeterministic IR emission from map iteration order
MaterializeValueContracts iterates for fn, plan := range plans (a map[llvm.Value]valuePlan) to emit entry llvm.assume/extractvalue facts. Go map iteration order is randomized, so the order in which entry facts are created across functions is nondeterministic. For a compiler this can defeat reproducible builds and undercuts the doc's promise that annotation-only cache hits "preserve the same contracts and optimization behavior." The idempotency test only covers a single function's byte-stable output, not cross-function ordering. Suggest collecting functions into a slice in module order (as the code already does for calls below) before emitting.
| switch typ.TypeKind() { | ||
| case llvm.StructTypeKind: | ||
| gepIndices = append(gepIndices, llvm.ConstInt(ctx.Int32Type(), uint64(index), false)) | ||
| offset += l.td.ElementOffset(typ, int(index)) | ||
| typ = typ.StructElementTypes()[index] | ||
| case llvm.ArrayTypeKind: | ||
| // Array indices are signed GEP operands. Use the target pointer | ||
| // width so a valid large 64-bit array index cannot become negative. | ||
| gepIndices = append(gepIndices, llvm.ConstInt(ctx.IntType(l.td.PointerSize()*8), uint64(index), false)) | ||
| typ = typ.ElementType() | ||
| offset += uint64(index) * l.td.TypeAllocSize(typ) | ||
| } | ||
| } |
There was a problem hiding this comment.
[P2] Missing default case in scalarizeLoadProjections GEP walk
The per-index switch typ.TypeKind() handles only StructTypeKind and ArrayTypeKind. If a traversal step ever lands on another aggregate kind (e.g. a vector or an unexpected type), the loop appends no GEP index and does not advance typ, then loads extract.Type() from a wrong/short address — a silent bad load rather than a panic. Not clearly reachable today (large-aggregate leaves are struct/array), but the sibling paths added in this PR (e.g. the invoke guard) fail loudly on unexpected shapes. Recommend a default: that bails out (fall back to the whole-value load) to make the invariant explicit.
| effects := NativeMemoryEffects(attr.Memory, HasHiddenPointerRoots(fn)) | ||
| // A contract assume models an already-required condition, not | ||
| // an observable storage access. LLVM's intrinsic-only control | ||
| // dependency does not widen the enclosing memory contract. | ||
| fn.AddFunctionAttr(ctx.CreateEnumAttribute(llvm.AttributeKindID("memory"), effects)) | ||
| if HasHiddenPointerRoots(fn) && attr.Memory.Args&^attr.Memory.Other != 0 { |
There was a problem hiding this comment.
[P3] HasHiddenPointerRoots(fn) computed twice per memory contract
In ApplyEffects, HasHiddenPointerRoots(fn) is called once for NativeMemoryEffects(...) and again immediately in the if HasHiddenPointerRoots(fn) && ... guard. It walks every parameter type and recurses through aggregate fields; the result can't change between the two calls. Hoist it into a local hidden := HasHiddenPointerRoots(fn).
| var owners []string | ||
| for source := range data.functionAttributes { | ||
| if source == name || resolve(source) == resolved { | ||
| owners = append(owners, source) | ||
| } | ||
| } |
There was a problem hiding this comment.
[P3] functionAttributes() scans the whole attribute map on every newFunc
functionAttributes(name) is called from newFunc for every function created in a package, including those with no contracts, and it iterates the entire functionAttributes map plus a sort.Strings and slice allocations on each call. Contracts are sparse so this isn't quadratic-in-module-size, but the common (no-contract) case still pays a full map walk in a heavily-annotated package like the runtime. Suggest an early if len(data.functionAttributes) == 0 { return nil, nil } and short-circuiting the alias scan when name is the only owner; the reverse linkname lookup could also be a precomputed resolved -> []owner index.
| func RemapFunction(from, to llvm.Value, m ABIMapping) error { | ||
| remapValues(m, from.GetEnumAttributeAtIndex, to.AddAttributeAtIndex, to.RemoveEnumAttributeAtIndex) | ||
| RemapFunctionEffects(from, to, m) | ||
| return nil | ||
| } | ||
|
|
||
| func RemapCall(from, to llvm.Value, m ABIMapping) error { | ||
| // ABI call replacements are new instructions and do not blanket-copy | ||
| // returned attributes, so dropping an incompatible one needs no removal. | ||
| remapValues(m, from.GetCallSiteEnumAttribute, to.AddCallSiteAttribute, nil) |
There was a problem hiding this comment.
[P3] RemapCall/RemapFunction return an error that is always dead + panicked on
RemapCall/RemapFunction are declared to return an error, but both return nil unconditionally, and every call site does if err := ...; err != nil { panic(err) } (large.go, cabi.go). The error return is dead API surface and the panic is unreachable, while the new hard-failures in these passes are bare string panics and CheckInstrumentation error-value panics — three styles in adjacent code. It's broadly consistent with the panic-based legacy ABI layer, but a malformed contract reaching this stage aborts with a stack trace instead of a located diagnostic (the generics test has to recover() and type-assert to convert). Consider dropping the unused error returns, or threading real errors and wrapping failures in the positioned Attribute.Error(...) form.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
zhouguangyuan0718
changed the title
zhouguangyuan0718
changed the title
zhouguangyuan0718
changed the title
Implement the v1 scope from xgo-dev#2518 with source selectors, cross-package contract propagation, ABI remapping for direct values, and runtime source annotations with build-specific contracts.
66ad7b6 to
1df08aa
Compare
|
@fennoai please review the current PR head, 1df08aa, against main. The PR now only refactors existing function declaration metadata (env, wasmimport, nointerface, function linknames and exports); it no longer introduces source contract attributes. Please focus on correctness across preloaded and one-shot compilation, explicit package patches, generic origins, imported functions, and independent source declarations that share a linker symbol. Report reproducible regressions and missing tests without modifying code. |
|
FennoAI is processing your request, please wait a moment... |
There was a problem hiding this comment.
FennoAI review — PR #2572
Reviewed the head 1df08aa against main across preloaded and one-shot compilation, package patches, generic origins, imported functions, and independent source declarations that share a linker symbol. This is a well-structured refactor; the new FunctionDeclaration API is uniformly nil-safe and the concurrency model holds (shared declaration/packageSyntax state is mutated only during single-threaded syntax preparation under lock, while concurrent backends take read-only paths).
I traced the one candidate correctness regression — funcName skipping the p.prog.Linkname fallback when a declaration exists but reports no linkname — and confirmed it is safe: DeclareFunction adopts and deletes any pending side-table entry, and SetLinkname routes to the declaration whenever one exists, so a name's linkname lives in exactly one place. No blocking findings.
The remaining items are minor: one clarity nit, a few steady-state performance observations, and two doc-comment accuracy nits. Details inline.
| source.declaration = p.functionDeclaration(fn) | ||
| } | ||
| declaration := source.declaration | ||
| v, linked := declaration.Linkname() |
There was a problem hiding this comment.
[P3] funcName: dead-ordered nil handling reads as a latent nil-deref
declaration.Linkname() is evaluated before the if declaration == nil check that overwrites it. This is only correct because FunctionDeclaration.Linkname() has a nil-receiver guard returning ("", false). Verified safe (not a regression): when declaration != nil the skipped p.prog.Linkname(orgName) fallback is unreachable, because DeclareFunction adopts-and-deletes any pending side-table linkname and SetLinkname routes to the declaration when one exists — so the linkname lives in exactly one place. Still, reordering as if declaration != nil { ... } else { ... } would make intent clear and avoid relying on the callee's nil tolerance for control flow.
| ret = &aFunction{Function: fn} | ||
| p.funcs[fn] = ret | ||
| } | ||
| if ret.declaration == nil { |
There was a problem hiding this comment.
[P3] Negative functionDeclaration results are never cached
functionDeclaration(fn) returns nil for a large class of functions (synthetic wrappers, cgo intrinsics, imported funcs not in the tables). Because nil is indistinguishable from "not yet resolved", ret.declaration stays nil and every subsequent p.function(fn) re-runs the full resolution (fn.Origin(), fn.Syntax() assertion, astFuncName string building, and an RLock-guarded slice scan). funcName (import.go:772-773) repeats the same guard, so both recompute per visit during lowering. Consider a resolved bool field or a sentinel so a nil result is cached once.
|
|
||
| // FunctionDeclaration owns the source information for one function or method. | ||
| // Distinct declarations remain independent even when they link to one symbol. | ||
| // It contains no LLVM state and is shared read-only after syntax preparation. |
There was a problem hiding this comment.
[P3] FunctionDeclaration doc overstates "read-only after syntax preparation"
The doc says the object is "shared read-only after syntax preparation," but declarations are still mutated after that point: DeclareFunction back-fills pkg/fset/pos and adopts pending linkname/export, ReplaceFunctionDeclarations writes fn.replacement (via BindPackageFunctionDeclarations), and source.declaration is set lazily during lowering. The real invariant is "read-only once backends run concurrently" — the phrasing already used at cl/function.go:200-201. Consider aligning the type doc to that to avoid implying an immutability that doesn't hold between syntax prep and backend start.
| for name, target := range p.packageSyntax.linknames { | ||
| links[name] = strings.TrimPrefix(target, "go:") | ||
| } | ||
| for name, entries := range p.packageSyntax.functions { |
There was a problem hiding this comment.
[P3] locality: scans all functions; name-keyed links may drop variants
Two related observations at this loop:
-
Wider scan: these loops now iterate the whole
packageSyntax.functionsmap (every declared function/method program-wide) instead of only thelinknamesentries as before.validateLocalitiesruns per package, so this becomes O(packages x all-functions); the body only cares about entries wherefn.Linkname()is true, so most iterations are wasted on the common no-linkname case. Worth confirming acceptable for large builds. -
Name flattening:
functions[name]can hold multiple declarations (original vs. patched variants), butlinksis keyed by the singlename; if two entries under one key carried different linknames, the last one iterated wins and the others are dropped from the reachability graph. Same-name variants normally share a linkname (or the patched variant supersedes viaEffective), so this is an edge case rather than a break — but it re-flattens a distinction the rest of the PR works to preserve.
1 participant
Existing function directives are stored in separate program-wide tables and looked up again during lowering. This refactor gives each source function or method a declaration object and carries that object with the frontend function.
The declaration owns the existing
//llgo:env,//go:wasmimport,//go:nointerface, function linkname and export information. Parsing creates the object first, then sets its properties. The dedicated env, wasm-import and nointerface tables are removed. Function links and exports move out of the shared tables when their declarations are bound; variable and unresolved-symbol entries retain their existing lookup paths.The existing frontend-to-backend association now stores an
aFunctionwrapper containing the Go SSA function, its source declaration and its backend entry. Declaration identity includes the package and source location. Two declarations that link to the same LLVM symbol keep independent metadata, while the existing backend function table still resolves symbols and checks closure-environment ABI compatibility. Function constructors keep their existing signatures.Generic instances retain their origin declaration, including private functions omitted from export-only preload. Both preloaded builds and the one-shot compilation entry point explicitly bind package-patch replacement declarations; original declarations retain their own information. The one-shot path selects replacements from the alternate SSA package, including declared methods, because its source files also contain the original declarations. Prepared declarations contain no LLVM state and are shared read-only by backend Programs.
Validation on macOS arm64 with Go 1.27.0 and LLVM 22.1.8:
cl,ssa, andinternal/buildsuites rerun after the one-shot patch-binding fix.clsuite withLLGO_BUILD_CACHE=off, plus completessaandinternal/buildsuites. These include existing env ABI, wasm import, nointerface, export and locality/linkname coverage.-O2with default GC andnogc.