Repository navigation
feat(c): add libc bindings with native ABI coverage - #45
MeteorsLiu wants to merge 9 commits into
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review — C-native shared types
Adds generated libc type bindings: Iovec (c), SchedParam/MachPortT/SemaphoreT (c/os), SigsetT (c/signal), plus the matching llcppg.pub entries.
ABI/memory-layout: verified correct on both targets. I independently checked each struct's size/alignment and cross-referenced the real C headers and the existing c/syscall/ztypes_* references:
| Type | linux/amd64 | darwin/arm64 | C ABI |
|---|---|---|---|
Iovec |
ptr + size = 16B | ptr + size = 16B | struct iovec 16B ✓ |
SchedParam |
int = 4B |
int + char[4] = 8B |
glibc 4B / macOS 8B ✓ |
SigsetT |
[16]Ulong = 128B |
uint32 = 4B |
glibc 1024-bit / __darwin_sigset_t ✓ |
MachPortT/SemaphoreT |
— | uint = 4B |
mach_port_t ✓ |
Build tags correctly gate each definition to the architecture whose ABI it models, and both targets cross-compile cleanly. No memory-safety or correctness issues — the undersized-sigset_t-OOB risk does not materialize.
The remaining notes are generated-code consistency / documentation nits (left inline). The highest-value one is the two iovec files using a non-standard // Code generated ... header; the rest are cosmetic. Since these are generated artifacts, the durable fix is in the llcppg generator / its post-processing rather than hand edits, so they don't regress on the next regeneration.
| @@ -0,0 +1,11 @@ | |||
| //go:build darwin && arm64 && !ios | |||
|
|
|||
| // Code generated from the libc binding; DO NOT EDIT. | |||
There was a problem hiding this comment.
Non-standard generated-code header. This file (and c/iovec_linux_amd64.go:3) use // Code generated from the libc binding; DO NOT EDIT., while every other file in this PR and across the repo uses the canonical // Code generated by llcppg; DO NOT EDIT. — these are the only two files with the different wording.
Both variants satisfy Go's ^// Code generated .* DO NOT EDIT\.$ detection, so this is not a functional bug. But any repo tooling/CI/regeneration script that keys on the exact by llcppg string would miss these two files, and the differing provenance string is confusing. Best fixed in the generator so it doesn't recur on regeneration.
| // Iovec describes one buffer for scatter/gather I/O. | ||
| type Iovec struct { | ||
| IovBase Pointer | ||
| IovLen Ulong |
There was a problem hiding this comment.
IovLen type differs across platforms (minor, no ABI impact). Here it is Ulong; the linux file (c/iovec_linux_amd64.go:10) uses SizeT. The C field iov_len is size_t on both platforms, so SizeT would be the faithful and cross-platform-consistent mapping.
This is cosmetic only: on darwin/arm64 and linux/amd64 both Ulong and SizeT are 64-bit, so the struct size/layout is identical (16B) and there is no runtime bug. Worth reconciling to SizeT on both so portable code against c.Iovec.IovLen sees one type regardless of target. Confirm it reflects actual llcppg output rather than a hand edit.
|
|
||
| type SigsetT = X__DarwinSigsetT | ||
|
|
||
| // [???] signal set |
There was a problem hiding this comment.
Generator placeholder leaked into a doc comment. // [???] signal set contains an unresolved [???] marker that will show up in godoc. The linux counterpart (c/signal/types_linux_amd64.go:13) has a clean comment: // A set of signals to be blocked, unblocked, or waited for.
Suggest emitting a real description (or dropping the placeholder). Note it also currently attaches to X__DarwinSigsetT, leaving the exported SigsetT (line 11) undocumented — unlike the linux file which documents SigsetT directly.
|
|
||
| import "github.com/goplus/lib/c" | ||
|
|
||
| // Canonical Mach typedefs from the macOS 14.0 SDK. |
There was a problem hiding this comment.
Minor doc nits. The declarations are correct (MachPortT = c.Uint matches mach_port_t→natural_t→unsigned int; SemaphoreT = MachPortT matches semaphore_t→mach_port_t). Two small things:
- The comment is phrased for multiple "typedefs" (plural) but, per Go doc-comment association, it attaches only to
MachPortT;SemaphoreTends up undocumented. - Pinning "macOS 14.0 SDK" is an arbitrary/unverifiable version claim for typedefs that are stable across SDKs — consider dropping the version.
|
|
||
| import "github.com/goplus/lib/c" | ||
|
|
||
| type SchedParam struct { |
There was a problem hiding this comment.
Missing doc comment (consistency). The exported SchedParam has no doc comment here, while the linux counterpart (c/os/sched_param_linux_amd64.go:9) carries // Data structure to describe a process' schedulability. The X__Opaque [4]c.Char padding itself is correct and required (matches macOS struct sched_param, 8B) — this is purely a docs-coverage inconsistency across the two platform files.
82b5401 to
e3a4dec
Compare
MeteorsLiu
changed the title
|
Merge order: #45 → #51 → #48 → #49 → #44 → #47 → #46 → #50 → #52. Each draft changes one folder only. This refresh regenerates native APIs without omitting functions merely to preserve handwritten entry points. Existing compatibility APIs remain available; colliding native names use the The combined prerequisite snapshot passes These PRs remain draft for review. Documented generator/ABI failures and previously approved |
MeteorsLiu
changed the title
MeteorsLiu
changed the title
MeteorsLiu
changed the title
MeteorsLiu
changed the title
MeteorsLiu
changed the title
MeteorsLiu
changed the title
There was a problem hiding this comment.
Review: native (link-mode) libc bindings
Reviewed the diff with code-quality, performance, security, and documentation-accuracy passes. This PR adds llcppg-generated native libc bindings for darwin/arm64, linux/amd64 and linux/arm64, moves LLGoPackage into platform-specific files, and adds a thorough ABI/symbol test harness.
Overall this looks solid. A few things stand out:
LLGoPackagecoverage is correct. After the move,LLGoPackageis defined exactly once for every target —darwin(stdio_darwin.go),windows(stdio_windows.go),linux/{amd64,arm64}/!android(the generatedlibc_linux_*.go), and the new catch-allpackage_default_other.go(!darwin && !windows && !(linux && (amd64 || arm64) && !android)). No double definition, no uncovered platform; removing the const fromstdio_default.gowas the right complement.- The test harness is well designed. The
symbol inventorysubtest cross-checks the generated source AST against the hand-maintained tables, so a newly generated symbol without a corresponding test fails loudly — good low-maintenance coverage.
The inline comments below cover one portability inconsistency worth addressing plus a few minor cleanups.
Minor (no inline anchor)
- Missing license header.
package_default_other.goand the four new hand-written test files (generated_native_llgo_test.go,libc_native_{linux_amd64,linux_arm64,darwin_arm64}_llgo_test.go) lack the// Copyright (c) 2026 The XGo Authors. Licensed under the Apache License 2.0.header that sibling files carry. They aren't marked as generated (DO NOT EDIT), so the generated-file exemption doesn't apply — worth adding for consistency. nativeCharsignedness divergence (FYI).libc_native_linux_arm64_llgo_test.goaliasesnativeChar = uint8while the package-widec.Char = int8(c.go:26, untagged) also applies on arm64. On ARMcharis unsigned, souint8is physically correct, but the harness papers over a representation mismatch betweenc.Charand the arm64 bindings. ABI size/class are unaffected; flagging only so the divergence is a conscious choice.
| @@ -1,3 +1,5 @@ | |||
| //go:build !(linux && arm64 && !android) | |||
There was a problem hiding this comment.
The exclusion only special-cases linux/arm64, but c.LongDouble/c.LongDoubleSize are defined solely by ldouble_float64.go and ldouble_x87.go, whose tags cover linux only on amd64, arm, mips, mipsle. So on other linux arches that otherwise compile (e.g. 386, riscv64, s390x — all have Long defined via ctypes_*.go), c.LongDouble is still undefined and a plain go test ./c fails to compile c_test.go exactly as it did before, since those arches aren't excluded here.
This fixes the one arch the PR adds bindings for while leaving the broader gap. Consider tying the constraint to the real LongDouble coverage (mirror the ldouble_*.go union) or moving TestLongDouble into its own file tagged identically to ldouble_*.go, and adding a one-line comment explaining the cross-file coupling.
| }}, | ||
| } | ||
|
|
||
| // Expected values were compiled from the original macOS 14.0 / glibc 2.24 C headers. |
There was a problem hiding this comment.
The macOS 14.0 / half looks like a copy-paste artifact from the darwin harness. This file is linux && amd64 only and its values are glibc-only (e.g. FNM_PATHNAME=1, LC_ALL=6, BUFSIZ=8192). Suggest referencing only the glibc 2.24 headers here.
| }}, | ||
| } | ||
|
|
||
| // Expected values were compiled from the original macOS 14.0 / glibc 2.24 C headers. |
There was a problem hiding this comment.
Same stale dual-attribution as the amd64 harness: this linux && arm64 file only contains glibc values, so the macOS 14.0 / reference is misleading — drop it.
| check func(*testing.T) | ||
| }{} | ||
|
|
||
| // Expected values were compiled from the original macOS 14.0 / glibc 2.24 C headers. |
There was a problem hiding this comment.
Inverse of the linux files: this darwin && arm64 file's values are macOS-only (e.g. FNM_PATHNAME=2, LC_ALL=0, BUFSIZ=1024), so the / glibc 2.24 attribution is inaccurate here. (The dual mention is fine in the shared libc_native_llgo_test.go, which compiles for all three targets.)
| if pointer, ok := receiver.(*ast.StarExpr); ok { | ||
| receiver, prefix = pointer.X, "*" | ||
| } | ||
| id := receiver.(*ast.Ident).Name |
There was a problem hiding this comment.
Minor/defensive: receiver.(*ast.Ident).Name is an unchecked type assertion. If the generator ever emits a non-*ast.Ident receiver (e.g. a generic/indexed receiver), this panics instead of failing cleanly. A comma-ok form with t.Fatalf on an unexpected shape would give a clearer diagnostic. Not a bug today — current bindings only emit simple identifier receivers.
There was a problem hiding this comment.
Review summary
Nicely structured change. It moves the Linux amd64/arm64 libc bindings to generated native (LLGoPackage = "link") files, relocates the "decl" fallback into the new package_default_other.go, and adds a thorough native ABI / symbol-inventory / runtime test suite.
Strong points verified during review:
- The
LLGoPackagebuild-constraint partition across all files is clean and exhaustive — exactly one definition per platform, no gaps or overlaps (including linux/386 and android/arm64 falling through to the newpackage_default_other.go). - The per-arch
nativeChar/nativeWide/nativeFilePositionaliases correctly model real ABI differences (unsignedcharandwchar_twidth on linux/arm64; glibcfpos_t16 vs darwin 8). - Header-value comments (
SEEK_SET==0,EOF==-1, glibc 2.24 / macOS 14 struct sizes) are accurate, and the shared runtime tests only use symbols present on all three native targets. - Dedicated security and performance passes found nothing of concern (bindings are thin
//go:linknamestubs; test buffers are correctly bounded).
A few observations below — none blocking.
Additional minor notes (no reliable inline anchor):
c/libc_native_llgo_test.go:73-74readslibc_<GOOS>_<GOARCH>.govia a relative path, relying ongo testrunning with the package dir as CWD. A one-line comment would make a future CWD-relatedt.Fatal(err)less puzzling.c/package_default_other.gohas no comment explaining why the fallback is declaration-only, unlike the explanatory note onstdio_windows.go. Pure nit.
| @@ -1,3 +1,5 @@ | |||
| //go:build !(linux && arm64 && !android) | |||
There was a problem hiding this comment.
This exclusion is needed because c.LongDouble / c.LongDoubleSize are undefined on linux/arm64: ldouble_x87.go is amd64-only and ldouble_float64.go's constraint covers arm (32-bit) but not linux && arm64. So the build tag works around a real platform gap rather than fixing it — on linux/arm64 c.LongDouble simply does not exist, even though the other two native targets (linux/amd64, darwin/arm64) have it, so any user code referencing it there won't compile. Note AArch64 long double is IEEE binary128 (16 bytes), so adding linux/arm64 to ldouble_float64.go (8 bytes) would be wrong — it needs its own 16-byte definition. Consider a brief comment here explaining the exclusion, and tracking proper linux/arm64 LongDouble support as follow-up.
| continue | ||
| } | ||
| if decl.Tok == token.CONST && name.Name == "LLGoPackage" { | ||
| if c.LLGoPackage != "link" { |
There was a problem hiding this comment.
This LLGoPackage == "link" assertion only runs when the parsed generated file actually declares an LLGoPackage const. The Linux generated files do (libc_linux_{amd64,arm64}.go set it to "link"), but libc_darwin_arm64.go does not — so on darwin this branch is a silent no-op, and darwin instead resolves LLGoPackage to "decl" from the unchanged stdio_darwin.go. Yet the darwin native tests still exercise real linked libc calls (Strtold, TestNativeLibcStdio, etc.), identical in spirit to the Linux ones that require "link". So the three native targets disagree on LLGoPackage while the test meant to guard it doesn't fire on darwin. Could you confirm darwin is intended to stay "decl"? If it should be "link" too, this is a latent linking inconsistency; if "decl" is correct for darwin, the check should assert a per-platform expected value instead of unconditionally "link".
| if pointer, ok := receiver.(*ast.StarExpr); ok { | ||
| receiver, prefix = pointer.X, "*" | ||
| } | ||
| id := receiver.(*ast.Ident).Name |
There was a problem hiding this comment.
receiver.(*ast.Ident).Name (and the *ast.StarExpr assertion above) assume every exported method receiver is Ident or *Ident. Any other receiver shape would panic the test rather than fail it cleanly. Fine for today's generated libc output, but a , ok check with a t.Fatalf would degrade gracefully if the generator ever emits a different receiver form.
d33ea8b to
99ddac9
Compare
Add generated libc bindings and native ABI checks for macOS arm64, Linux amd64 and Linux arm64 while preserving existing handwritten APIs.
The implementation includes:
c.Charandc.Printf, omit duplicate generatedNative*declarations, and defer the 36 macOS functions that require the generated C wrapper mechanism._Exit,_toupper,_tolower, XPGbasenameandstrerror_r, together with native types needed for ABI mappings.C.targets for the macOSSysNerr,SysErrlistandSuboptargnative variables, represent the externalsys_errlistarray as[0]*Char, and retain LinuxCfreethrough its nativefreealias.LLGoPackagedirectly under platform build tags. Linux amd64 and arm64 use"link"so generated Go union accessors are compiled; other targets retain their previous package mode. All 47 Go target pairs select exactly one declaration.Validation after rebasing onto
goplus/lib main(d83f71e1): the CI-pinned LLGodbce82abpassesllgo test -count=1 -v ./c, normally and with-tags=nogc, with 841 tests/subtests on Linux amd64 and 840 on Linux arm64. The nine-package libc integration build passes in all four Linux configurations. macOS arm64 CI passes 676 root-package tests/subtests, including native global storage checks. All four GitHub Actions jobs pass on this head. Every native binding has an explicitC./stdcall.target, compiler intrinsics retainllgo.*, and imported native functions remain bodyless declarations. Native C prototype and availability checks cover all 204 retained new C entries on macOS and all 214 on each Linux target.Generated with unmodified llcppg main
da227ff4, macOS SDK 14.0 and Bootlin stable glibc 2.24 headers. Previously approvedva_listand verified Linux long-double generation/ABI exclusions remain. Runtime results cover the current native environments; the SDK/sysroot versions provide the header baseline.The root package adds tested bindings without replacing existing handwritten signatures or publishing the deferred C wrappers and private implementation functions.