Repository navigation
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review summary
This PR cleanly separates native linkname entries (C. / stdcall.), Go-ABI
placeholder bindings (llgo:link), and shared Go variable storage, and backs it
with a thorough diagnostic/test matrix plus a new doc/linkname.md. The core
behavior change — a bare variable go:linkname target now means Go storage
instead of a C extern — is applied consistently, and the ~20 in-tree bare-symbol
variable bindings were correctly migrated to the explicit C. prefix, so no
production caller silently changes meaning.
Reviewed across code quality, performance, security, and documentation. The
security pass found no issues: symbol-name/prefix parsing is guarded, the
*types.Pointer / *ast.ValueSpec assertions in cl/linkname_native.go are
safe, and the new C test harnesses are memory-safe. InitSharedGo's
zero/nonzero coalescing and the wasm WeakAnyLinkage fallback look correct.
The inline notes below are all minor/low severity — none block merge.
Additional minor note: the "$stdcall" type-callconv suffix is now spelled
as a string literal in three places (cl/import.go initLink,
cl/linkname_native.go collectNativeLinkDefinitions, and nativeVariableType).
A shared constant/helper would keep the three sites from drifting apart.
Findings without inline locations
doc/linkname.md:62: The new variable rule (bare target = Go storage; native needs an explicitC./stdcall.prefix) is a breaking change from the prior behavior, where a bare variable target denoted a C extern. This PR had to migrate ~20 in-tree files to theC.prefix to preserve semantics. A one-line migration/compatibility note here would help out-of-tree code that relied on the old meaning.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This was referenced Oct 21, 2026
Collaborator
Author
|
Addressed all review threads and the reproduced CI failures. The follow-up fixes cover stdcall callback nil/native-pointer assignment, Darwin shared-storage debug addresses, migrated native-variable dependencies, and the removed The latest Ubuntu failure exposed one more migration requirement: Validation: full local |
Collaborator
Author
|
Fixed the six Windows failures in c87f706.
Validation:
The earlier macOS Intel artifact failure was a GitHub DNS timeout downloading newlib; the latest-head job now passes. Merged current upstream main (0578518) in 47141cb to resolve the LLGo-Link.md conflict introduced by #2903. The resolution retains the new upstream document structure, adds the function-placeholder case alongside methods, and preserves the native-entry restrictions, literal-symbol rules, explicit cgo imports and regression references from this PR. This merge changes documentation only. All six focused linkname IR/diagnostic test groups pass, relative documentation links resolve, and embedme verification passes. Final CI validation for 47141cb: all 12 workflow runs have completed, with 115 successful checks and one expected skipped tag-publication job. Native/compiler, Go, browser, all six Windows artifacts, and WinGet candidate validation all pass. The candidate job passed after a job-only retry of its artifact download failure. |
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 |
cpunion
added a commit
to cpunion/llgo
that referenced
this pull request
Oct 21, 2026
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.
cpunion
added a commit
to cpunion/llgo
that referenced
this pull request
Oct 21, 2026
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.
cpunion
added a commit
to cpunion/llgo
that referenced
this pull request
Oct 21, 2026
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.
cpunion
added a commit
to cpunion/llgo
that referenced
this pull request
Oct 21, 2026
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.
cpunion
added a commit
to cpunion/llgo
that referenced
this pull request
Oct 21, 2026
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.
cpunion
marked this pull request as draft
October 22, 2026 08:55
cpunion
force-pushed
the
fix/linkname-native-entries-20261021
branch
from
October 24, 2026 14:45
47141cb to
235cc7b
Compare
cpunion
marked this pull request as ready for review
October 24, 2026 14:50
Contributor
There was a problem hiding this comment.
Review: native linkname entries
Solid, well-scoped change. The core mechanics check out across all four review passes:
- Correctness — The
HasLinknameTargetrewrite from an O(n) scan to the reference-countedlinknameTargetsmap is correct, including the rebind-to-same-target net-zero case and delete-at-zero. Error propagation fromcompileGlobal→processPkg→newPackageExis complete and wrapped with source position. TheInitSharedGocoalescing path is correctly gated ongoVar+LocalityNone, so extern and thread-local storage are not disturbed. - Security / robustness — No new defects. The type assertions in
nativeVariableTypeand the*ast.ValueSpecassertions are protected by their enclosing invariants, and everypanicsite is the project's established panic-as-diagnostic pattern recovered at the package-build boundary. - ABI boundary —
DefineNativeExportis correctly shared between the package-local and final-link paths, which are mutually exclusive viaCExportWrappers; the native wrapper signature is guaranteed non-nil wherever it is consumed.
Findings below are documentation accuracy and a couple of maintainability nits — none blocking.
Documentation accuracy
The two inline notes flag a doc-vs-code mismatch this PR re-touched: doc/LLGo-Link.md says a py./llgo. function with a body is "rejected with a compile error", but compileFuncDecl returns ignoredFunc for any non-goFunc ftype (cl/compile.go:634), so the body is silently discarded rather than rejected. The enforcement gap is pre-existing, but these doc lines were edited in this PR and still assert rejection.
Minor wording observation (no inline anchor): doc/LLGo-Link.md lines ~92/244 state that a stdcall. linkname on a non-Windows target is rejected "whether or not it has a body" and for variables. Enforcement is actually lazy — the error originates in ssa/stdcall.go stdcallCallConv only when the stdcall ABI is materialized (a call/callback/type construction). A declared-but-unused stdcall symbol on a non-Windows target may not trigger it. Worth confirming whether the doc intends to promise eager rejection.
Maintainability (optional)
ssa/package_syntax.goNativeExport: the local is namedcc(cc, ok := ...nativeExports[name]) but its type isNativeExportInfo, notCallConv.infowould read more clearly. (inline)cl/linkname_native.gonativeVariableType: a freshtypes.NewNamed(...+"$stdcall")is built on every call. Correctness holds because the stdcall callconv is keyed by the linkname string (not type identity) andNewVarcaches by name, but producing multiple identically-named*types.Namedinstances is surprising — a one-line comment or memoization would reduce the refactoring hazard. (inline)- The
stdcall.variable →$stdcalltype-callconv registration is implemented in two places (collectNativeLinkDefinitionsfor the compiled package andinitLinkfor imported packages). Both are needed for their scopes; a cross-reference comment would guard against drift.
Performance (informational)
No regressions — HasLinknameTarget is now a genuine improvement. The new passes in ParsePkgSyntaxWithOptions are all linear in source size. The only opportunistic cleanup worth noting: ParsePkgSyntaxWithOptions now sweeps file.Comments several separate times (validateInternalDirectives, collectPackageLinknames, and the new //go:cgo_import_* loop), and collectNativeLinkDefinitions recomputes astFuncName per decl and takes the packageSyntax lock twice even on the common no-linkname path. Constant-factor only; fold if compile time of comment-heavy/cgo packages ever shows up in a profile.
cpunion
force-pushed
the
fix/linkname-native-entries-20261021
branch
from
October 25, 2026 00:55
235cc7b to
ebd2b8a
Compare
Collaborator
Author
|
Full compiler testing after rebase caught main's new interface-kind predicate fixtures using bodyless llgo:link declarations. Updated only those fixtures with the required placeholder bodies in d541938, retaining the lowering checks and every invalid-argument diagnostic. The two failing tests now pass; the complete compiler suite is being rerun. |
Collaborator
Author
|
The complete compiler suite now passes (321.144s), including the interface-kind placeholder fixtures and reflection metadata cases; the complete SSA suite also passed. The isolated local failure was a shared in-process test cache archive from the other ABI branch; rerunning with the incompatible archive isolated resolved it without changing compiler behavior. |
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
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.
Implement the function/ABI rules in #2879, building on #2894.
go:linknamefunction withC.orstdcall.gets a native entry with foreign-thread/GC handling; its Go implementation retains the Go ABI. Prototypes remain external references.llgo:linkrequires a placeholder body, ignores it completely and never exports an implementation. Preserve native callback/storage widths and reject invalid native declarations before registration.goplus/lib v0.7.0across all module manifests, including older-Go CI; remove fork replacements and restore the six example/tool modules to Go 1.20. The release includes prototype-only Python imports such aspy.NewSet.Rebased on updated #2894 and main
05c2d6b4f. Validation: full SSA suite; targeted compiler/linker and Windows MSVC/MinGW IR tests; gc/LLGo storage, native entries, foreign-thread callbacks and Full LTO execution; C/Python bindings, Python set construction and castdump. Go 1.20.14 dependency/build checks and focused gc/LLGo regressions pass. Windows execution and the remaining matrix run in CI.Keep shared gc/LLGo cases in
test/go, native/placeholder cases intest/llgoext, and binding rules indoc/LLGo-Link.md.Review corrections: document ignored Python/instruction bodies and lazy non-Windows stdcall diagnostics; clarify symbol-keyed stdcall type metadata. Rebased onto the updated #2894; targeted compiler/SSA/native-link tests pass.