Repository navigation
fix(cgo): support generated declarations under strict C11 - #3036
Merged
Merged
Conversation
Collaborator
Author
|
@fennoai Please review the strict-C11 Cgo fix, including function and macro symbol declarations and the executable regression. The new test fails on the baseline and passes with this change. |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: cgo type queries compatible with strict C11
The core fix is correct and minimal. Switching the emitted C from typeof(...) to __typeof__(...) is the right portable choice: typeof only became a standard keyword in C23, while __typeof__ is a GNU extension accepted in all GNU C modes including -std=c11. The /* template: */ comment in cgo.go and the internal/build/cgo_test.go assertions were updated consistently, and the new test/cgo/c11 fixture genuinely reproduces the regression — static int add_one is emitted with __typeof__(add_one)* and compiled under #cgo CFLAGS: -std=c11, which the old codegen would reject.
One point worth addressing (inline): the new test files omit the //go:build llgo && !wasm constraint that every sibling in test/cgo/ carries, which pulls the fixture into the stock-Go go build ./... CI step and leaves it un-gated for wasm.
Security, performance, and documentation-accuracy passes found no issues. A license header is correctly omitted, matching the convention for files under test/cgo/.
Contributor
|
All verification is complete. The regression test is present, discovered by the recursive One observation worth carrying forward from this review: the regression test exercises the function symbol path ( This is a managed-mode turn, so I return my final result as Markdown without publishing a GitHub comment. Review: strict-C11 cgo fix (PR #3036)I verified the fix against the three things you asked about — function symbol declarations, macro symbol declarations, and the executable regression — including the baseline-fails / fix-passes claim. The fix is correct. Baseline-fails / fix-passes — reproduced directly with clang 18,
So the regression is real, reproduces on the baseline for both symbol kinds, and is resolved by the change. Test coverage:
I could not execute the LLGo end-to-end run or the One point worth noting (coverage, not a correctness bug): the executable regression exercises only the function symbol path. The macro path ( Prior review item still open: the new |
Collaborator
Author
|
@fennoai Added |
Contributor
|
Everything checks out at the new head
This is a managed-mode turn, so I return the re-review as Markdown without publishing a comment. Re-review: build-constraint update (PR #3036 @
|
| Toolchain | Before this update | Now |
|---|---|---|
Stock Go (go build ./..., no -tags=llgo) |
compiled the fixture (broke sibling isolation) | build constraints exclude all Go files — excluded, matching every sibling |
-tags=llgo (CI core suite, go list -tags=llgo ./test/...) |
included | still CgoFiles=[cgo.go] TestGoFiles=[cgo_test.go] — executable regression still runs under LLGo |
So the fixture is now gated off the stock-Go build and off wasm, while still being exercised end-to-end by the LLGo core suite. The constraint is byte-identical to the native siblings (e.g. narrow.go).
Re-confirmed the fix still holds at this head:
- Core codegen:
internal/build/cgo.goemits__typeof__(%s)%s %s;; unit test pins both__typeof__(request_func)*and__typeof__(request_macro). - C-level regression under
-std=c11: baselinetypeoffunction decl FAILS, fix's__typeof__function and macro decls both PASS. - Both changed files are
gofmt-clean.
Open item from the earlier re-review (unchanged, non-blocking): the executable regression still exercises only the function symbol path; the macro path remains covered by the stubbed-clang unit assertion rather than a real -std=c11 cgo program. The fix is correct for macros regardless (re-verified above). Adding a #define-based macro used from Go in the same c11 fixture would close the end-to-end gap, but that's optional polish, not a blocker.
Note: I verified behavior at the C level with the system clang and via go list; the full LLGo end-to-end run and the internal/build package test still can't execute in this sandbox (no llvm-config/LLVM headers). Your report that native LLGo execution and the -tags=llgo control both pass is consistent with everything I was able to verify here.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Collaborator
Author
|
@fennoai The C11 regression now intentionally runs under both stock Go and LLGo, using |
Contributor
|
Everything is verified and passing. Let me reflect on one nuance worth surfacing honestly in the re-review: under stock Go, This is a managed-mode turn, so I return the re-review as Markdown. Re-review: shared stock-Go + LLGo coverage (PR #3036 @
|
| Configuration | Expected | Result |
|---|---|---|
stock Go, CGO_ENABLED=1 |
include | CgoFiles=[cgo.go] TestGoFiles=[cgo_test.go] ✓ |
-tags=llgo, CGO_ENABLED=1 (LLGo core suite) |
include | included ✓ |
stock Go, CGO_ENABLED=0 |
exclude | build constraints exclude all Go files ✓ |
wasm (GOOS=js GOARCH=wasm) |
exclude | build constraints exclude all Go files ✓ |
Ran the stock-Go execution directly here — CC=clang CGO_ENABLED=1 go test ./test/cgo/c11 → PASS (no special Go tag), confirming your claim that it runs clean under stock Go. The package is also gofmt-clean and go vet-clean under stock cgo. The core fix (__typeof__ in cgo.go) and both unit assertions remain intact.
One semantic clarification on what the stock-Go lane covers (not a defect — just so the coverage isn't over-read): under stock Go, C.add_one is resolved by Go's own cgo frontend, not by LLGo's genExternDeclsByClang — the function this PR actually changed. So the stock-Go execution proves the fixture is valid, compilable, and -std=c11-clean under a second toolchain (real, worthwhile shared-compatibility coverage), but it does not re-exercise the __typeof__ codegen line the fix touches. The regression property that matters — baseline typeof fails, __typeof__ passes under -std=c11 — is exercised only on the LLGo path (via the -tags=llgo core suite) and by the internal/build unit test. Widening to cgo && !wasm is a reasonable, deliberate choice that mirrors allocation.go; just worth being clear that the stock lane is a compatibility smoke test, not a second copy of the regression.
As before, I verified classification and the stock-Go run directly; the actual LLGo end-to-end codegen run still can't execute in this sandbox (no llvm-config/LLVM headers). Your report that native LLGo execution also passes is consistent with everything verifiable here, and the earlier C-level proof of the typeof → __typeof__ fix still holds at this head.
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
LLGo baseline benchmarks
String equality benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
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.
Use
__typeof__in generated Cgo declarations so packages selecting#cgo CFLAGS: -std=c11compile with GNU keywords disabled. Add a native C11 regression shared by Go and LLGo.Fixes #3034.
Validation: the new test reproduces the baseline failure; Go and LLGo C11 execution, the full native Cgo suite,
-O3 -lto=full, and Cgo build tests pass with LLVM 22. Vet retains the 12 existing fixture diagnostics.