Repository navigation
fix(cgo): guard C exports in dependencies and native executables - #2752
Merged
Merged
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}
Contributor
There was a problem hiding this comment.
Review: fix(cgo): protect native C exports in dependencies and executables
This is a solid, well-scoped fix. Executables and dependency packages that expose //export functions now correctly link the runtime and wrap each export with foreign-thread registration (EnterForeignThread/ExitForeignThread + a one-time EnableForeignThreadRegistration), so callbacks entering Go from C-created threads are GC-safe. The linkedCExports/packageCExports refactor cleanly separates pure declaration extraction from the snapshot fallback, preserving prior validation semantics while surviving isolated-worker disposal.
What I verified (all sound):
needsCExportWrappersis applied consistently at both decision sites (linkedCExports,preparePackageModule).cSharedExportArgsintentionally uses a different, consistent symbol-retention mechanism for shared libraries.dceEntryRootCandidatesreadspkg.LPkgdirectly, but that is safe:applyDeadcodeDropOverridesruns only when deadcode drop is enabled, whereas snapshot-then-dispose runs only when it is disabled — the two are mutually exclusive, so no disposedLPkgis ever touched there.- Snapshot retention is safe:
cExport.sigis a*types.Signature(a go/types value owned by the build-scoped frontend), not an LLVM-backed object, so the snapshot does not keep LLVM graphs alive after disposal — matching the doc comment. - The new
threads.cfixture is correct:workers[3]/threads[3]are bounded,startedgates the join loop to successfully-created threads, every thread is joined/closed, and the result arithmetic ((20+22)×4×3 = 504, 24 total calls) matchesmain.go's assertions. - All rewritten comments accurately describe the new behavior.
Minor, non-blocking observations:
dceEntryRootCandidates(build.go:~2610) re-implements per-package export enumeration directly frompkg.LPkg.ExportFuncs()/FuncOf()and, unlike the newpackageCExportDeclarations, does not apply the foreign-export filter (strings.Contains(goName, ".") && !strings.HasPrefix(goName, pkg.Path()+".")). It's harmless today (an extra DCE root at worst keeps an already-defined symbol, and this path predates the PR), but the "which exports belong to this package" rule now lives in two places with slightly different logic. Consider routing it throughpackageCExports/packageCExportDeclarationsso the ownership rule is defined once.
No blocking issues found.
| return exports | ||
| } | ||
|
|
||
| func packageCExports(pkg *aPackage) []cExport { |
Contributor
There was a problem hiding this comment.
Nit (non-blocking): unlike the sibling snapshot accessors linkedModuleGlobals (build.go:~2711) and the loop in cSharedExportArgs (build.go:~2927), packageCExports dereferences pkg without a nil guard. No live bug — both current callers reject/skip nil before reaching here — but a if pkg == nil { return nil } guard would keep this helper consistent with its neighbors and robust for future callers.
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
marked this pull request as draft
October 7, 2026 23:29
cpunion
marked this pull request as ready for review
October 8, 2026 02:51
Contributor
There was a problem hiding this comment.
Re-review: fix(cgo): guard C exports in dependencies and native executables
Re-reviewed the current head (8b9982f) across code quality, performance, security/memory-safety, and documentation accuracy. The fix remains solid and well-scoped: executables and dependency packages exposing //export now link the runtime and wrap each export with foreign-thread registration (EnterForeignThread/ExitForeignThread + a one-time EnableForeignThreadRegistration), making C-created-thread callbacks GC-safe, while package C correctly retains its direct unqualified exports.
Verified sound this pass:
- Memory safety / threading —
defineCExportWrappersordering is correct, andcallbackInit(EnableForeignThreadRegistration) runs afterrtInitbut before any packageinit/main, so registration is enabled before C can obtain a Go function pointer. The absence of a deferredExitForeignThreadis not a leak: in the TLS-lifecycle path the real unregister happens in the thread-exit destructor, matching the pre-existing library wrapper behavior. - cgo address substitution (
cgo.go:217-222) — correctly rebinds C's taken address from a same-named/shadowing Go implementation to the public export's final-link wrapper, so the GC-safe registering entry is never bypassed. Well pinned byTestGenExternDeclsUsesPublicExportAddress. threads.cfixture — boundedworkers[3]/threads[3],startedgates the join loop, no uninitialized handles, no cross-worker races;(20+22)×4×3 = 504and 36 total calls matchmain.go.- Determinism —
ExportFuncs()map iteration order never leaks:linkedCExportsandcSharedExportArgssort before emitting, and the cgo.go loop breaks on an exact match. - Comments — all rewritten doc/comments (
cl/compile.go,build.go,main_module.go, and the ENTRY/CHECK FileCheck directives in the fixtures) accurately describe the new behavior.
Outstanding (already filed, non-blocking): the earlier nit on packageCExports (build.go:2646) — missing if pkg == nil { return nil } guard, unlike sibling accessors packageCExportDeclarations, linkedModuleGlobals, and cSharedExportArgs — is still unaddressed on this head. It's already tracked in the unresolved inline thread, so I'm not duplicating it here. No live bug (both current callers pre-filter nil); purely a consistency/robustness improvement.
No blocking issues found.
visualfc
approved these changes
Oct 8, 2026
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.
A C-created thread calling a
//exportfunction in an executable or imported package can crash withCollecting from unknown thread(macOS). Existing thread guards covered only main-packagec-shared/c-archiveexports.Extend the existing guards to ordinary Go exports in native Linux, macOS, and Windows builds:
C.Callbackfunction pointers also resolve to the guarded public entry. Retain export metadata through parallel package builds. Preservepackage C's direct symbols (Double,Xadd → add) and runtime-owned entries.Validation:
test/cgo. Four-level reentry passes on both Go and C-created threads, including GC after inner returns, outer allocation retention, and all defers.test/cgosuite executes the standalone fixture on Linux amd64, macOS arm64, and Windows amd64/arm64/386 (MSVC and MinGW), using the normal build cache.ENTRYchecks verify public wrappers and their Go targets; package-C ABI, export snapshots, and dead-code retention checks pass.test/cgoruns pass, including repeated runs with caching and the Go 1.20/1.27 version runners. Latest CI is pending.