Repository navigation
feat(c/os): restore common native OS APIs - #47
MeteorsLiu wants to merge 6 commits into
Conversation
b3e821f to
89cef74
Compare
|
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 |
There was a problem hiding this comment.
Review: native libc OS bindings
Reviewed the full diff across four dimensions (correctness/quality, performance, security, documentation). This PR adds llcppg-generated libc bindings for linux/amd64 and darwin/arm64 (plus shared Iovec, signal, sched_param, and dirent types).
Overall: looks solid. go build and go vet ./c/os/ pass on linux/amd64 (darwin/arm64 can't be cross-compiled here due to the cgo toolchain, so darwin was reasoned about statically). No duplicate- or missing-symbol conflicts between the generated files and the existing hand-written os.go / os_darwin.go / os_other.go on either platform — the shared ModeT/UidT/GidT/OffT/DevT/PidT types resolve per-platform and the generated files only reference them.
ABI layouts verified against glibc / macOS SDK and found correct:
Termios— linux[32]CcT+c.Uintflags; darwin[20]CcT+c.Ulongflags.SigsetT— linux[16]c.Ulong(128B); darwinuint32.Iovec,Statvfs,Rusage,siginfo_t,SchedParam,GlobT,Passwd/Group— layouts and integer widths match their targets.- Spot-checked ~50
linkname/llgo:linkmappings; Go→C symbol names and platform-divergent signatures are correct.
No security concerns and no meaningful performance concerns (these are pure FFI declarations).
The findings below are all low-severity / nits on generated output — none block the PR. Several belong upstream in the llcppg generator rather than being hand-edited in these DO NOT EDIT files.
Not inline-anchored
- Merged
.pubmanifests can't express per-platform type ownership.c/signal/llcppg.publistsX__SigvalT/SiginfoT/Sigval(which exist only in the linux variant) andSigsetT, but omits the darwin-only intermediate aliasesX__Uint32TandX__DarwinSigsetTexported bytypes_darwin_arm64.go. Similarlyc/os/llcppg.pubmixes linux-only (__syscall_slong_t, …) and darwin-only (mach_port_t,_filesec, …) typedefs in one file. This is generator metadata (it doesn't affect the Go build, which passes), but worth confirming the single merged manifest is intentional vs. splitting per platform. - Missing Darwin
fcntl/openflag constants. The orphaned comments flagged inline atlibc_darwin_arm64.goindicateO_NONBLOCK(O_NDELAY) andF_GETFL/F_SETFLwere dropped from the Darwin bindings during generation. Worth confirming the omission is intentional.
Findings without inline locations
c/iovec_linux_amd64.go:10: Nit:iov_lenissize_ton both platforms, but it's typedSizeThere (linux) andUlonginiovec_darwin_arm64.go. Both are 8-byte unsigned on LP64 so the ABI is correct, but using two alias names for the same semantic C type is a needless cross-platform inconsistency — prefer one (SizeT) on both for maintainability.c/signal/types_darwin_arm64.go:13: Nit: The// [???] signal setplaceholder is Apple's own[???]conformance marker faithfully extracted fromsys/_types.h, so it's not wrong — but it reads as noise/an incomplete doc. If the generator can be configured to drop or translate these[???]markers, that would read cleaner on regeneration (same marker also appears atlibc_darwin_arm64.go// [???] microseconds).
|
|
||
| import "github.com/goplus/lib/c" | ||
|
|
||
| // Canonical Mach typedefs from the macOS 14.0 SDK. |
There was a problem hiding this comment.
Nit (low): Pinning a specific SDK version — macOS 14.0 SDK — in a DO NOT EDIT generated file is a staleness trap: mach_port_t/semaphore_t both resolve to natural_t (unsigned int) and have been ABI-stable across many macOS releases, so the version isn't what's actually version-sensitive, and the text won't update if the file is regenerated against a newer SDK. Consider dropping the version (or deriving it from the generator) so the comment can't drift.
| const ( | ||
| // no delay | ||
| // set append mode | ||
| O_APPEND = 8 |
There was a problem hiding this comment.
Nit (low): Orphaned/misattributed comments. // no delay and // set append mode are stacked above O_APPEND = 8; // no delay actually belongs to an omitted O_NONBLOCK/O_NDELAY = 4 constant, so it now reads as if it describes O_APPEND. Besides the misleading comment, this signals O_NONBLOCK is missing from the Darwin bindings. Generator artifact — fix belongs upstream in llcppg rather than hand-editing this file.
| // get file status flags | ||
| // set file status flags | ||
| // get SIGIO/SIGURG proc/pgrp | ||
| F_GETOWN = 5 |
There was a problem hiding this comment.
Nit (low): Same orphaned-comment pattern — // get file status flags and // set file status flags sit above F_GETOWN = 5 but belong to omitted F_GETFL = 3 / F_SETFL = 4 constants. The comment is misleading and it signals F_GETFL/F_SETFL are missing from the Darwin bindings. Generator-level; worth confirming the omission is intentional.
Restore common native OS APIs from unified libc generation.
The implementation includes:
Close,Closedir,Dup,Dup2,Fcntl, andSyscall.nogcon all three targets.Existing C symbol coverage and the shared root C API remain unchanged by the name cleanup.