Repository navigation
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
FennoAI Review — preserve Go storage and initialization for linknamed variables
The core change is sound and well-tested: InitSharedGo coalesces zero definitions under CommonLinkage, promotes a nonzero static initializer to ExternalLinkage, and correctly defers to a dynamic import only for zero storage — matching gc's "static definition beats dynamic import" semantics. The cross-target IR test matrix (cl/linkname_storage_test.go), the go/cgo integration suites, and the duplicate-definition negative test give good coverage. The C.-prefix migration of bare native linknames (runtime/..., clite/c.go) is consistent with the new varName routing where a bare symbol now defaults to goVar/define=true.
Findings below are non-blocking quality/robustness notes. No correctness defect was found in the generated-IR control flow, and the documentation/comment review found no inaccuracies.
Also considered (no action needed):
- Security: no exploitable memory-safety or trust-boundary defect.
CommonLinkageis only ever assigned to zero storage, so distinct symbols cannot silently merge; the duplicate-nonzero case panics rather than coalescing. Thesetup-depschange sources the repo's own vendoredinstall.shvia$GITHUB_ACTION_PATH(no remote fetch). - The duplicate-definition panic (
ssa/decl.go:291) is recovered and converted to an error byrunPackageJob, so the normal build reports it cleanly rather than crashing — see the inline note on its message quality.
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 |
Reuse the callback-store fix from c87f706 in PR xgo-dev#2895 without the unrelated linkname changes. Clearing or restoring a C callback must not write a two-word Go funcval over adjacent native storage. Add deterministic adjacent-slot and closure-rejection regressions, plus pointer-width IR checks on Linux, Darwin and all six Windows ABI targets.
Collaborator
Author
|
Fixed the macOS CI crash in a059027 by reusing #2895's native callback store fix. Clearing/restoring a C callback now writes one native pointer and preserves adjacent native slots; capturing Go closures are still rejected. The new adjacent-slot/closure regressions pass on macOS ARM64, and the IR checks pass for Linux, Darwin, and all six Windows ABI targets. Validation is complete for a059027: 116 checks succeeded; the tag-only release job was skipped. The completed LLGo rerun passes, including all six native Windows ABI/architecture lanes. |
cpunion
marked this pull request as draft
October 22, 2026 08:55
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow the variable-storage rules in #2879: ordinary
go:linknamerenames Go storage, including declarations without initializers.Zero definitions coalesce, one nonzero static definition wins, duplicate static definitions fail, and each dynamic initializer still runs. Function variables keep Go closures. Explicit cgo imports remain exceptions, including default package symbols and Windows import slots.
Use weak zero definitions on Wasm and Darwin (which preserves debugger addresses), indexed linkname-target lookup, and source-positioned duplicate diagnostics. Pin the C-prefixed bindings from goplus/lib#53 across all module roots so old native variables cannot become Go storage; migrate
castdumpto the maintainedclang-cbindings.Validated with shared gc/LLGo tests in
test/go, compiler/linker and six-target IR tests, Windows import lowering, Wasm checks, Linux process/environment probes, and the Darwin debugger suite (293/293). Built on merged #2882; native function rules follow in #2895. The broader #2878 audit remains separate.