diff --git a/CHANGELOG.md b/CHANGELOG.md index b3f2bca..6adb724 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.5.6] - 2026-07-05 + +### Fixed +- **Critical: callback stack-move corruption** — when a C callback re-enters Go and grows the goroutine stack (`copystack`), `syscallArgs` on the goroutine stack would move but `syscallN` on g0 still held the old address, writing return values to freed memory. Fix: `syscallArgs` is now heap-allocated via `sync.Pool`, immune to `copystack`. Reverts the unsafe `//go:noescape` optimization from v0.5.4. Discovered and fixed by [@tie](https://github.com/tie) with `TestCallbackGrowStack` reproducer. ([PR #59](https://github.com/go-webgpu/goffi/pull/59)) +- **sret cleanup** — simplified >16B struct return path in `call_unix.go`, removed redundant `sretBuf` variable +- **Added `TestStructReturn24B`** — end-to-end test for >16B struct return via sret hidden pointer + ## [0.5.5] - 2026-06-15 ### Fixed diff --git a/README.md b/README.md index 8dffd11..850456b 100644 --- a/README.md +++ b/README.md @@ -174,11 +174,11 @@ variadic arg type slices. | Benchmark | Time | Allocations | |-----------|------|-------------| -| Empty function (`getpid`) | 88 ns | 0 allocs (Unix) / 2 allocs (Windows) | -| Integer argument (`abs`) | 114 ns | 0 allocs (Unix) / 3 allocs (Windows) | -| String processing (`strlen`) | 98 ns | 0 allocs (Unix) / 3 allocs (Windows) | +| Empty function (`getpid`) | 88 ns | 0 allocs (steady state) | +| Integer argument (`abs`) | 114 ns | 0 allocs (steady state) | +| String processing (`strlen`) | 98 ns | 0 allocs (steady state) | -Since v0.5.4, `//go:noescape` on `runtime_cgocall` keeps `syscallArgs` on the goroutine stack — true zero-allocation FFI on Unix platforms. Windows uses `syscall.SyscallN` (runtime-managed). +`syscallArgs` is heap-allocated via `sync.Pool` for callback safety (goroutine stack may move during C→Go callbacks). Pool reuse gives 0 allocs/op in steady state. At 60 FPS with ~50 FFI calls per frame, overhead is **5 µs per frame** — 0.03% of the 16.6 ms budget. Unmeasurable in profiling. diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 83d5438..ecdae33 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -73,7 +73,7 @@ ffi.PrepareCallInterface(cif, types.DefaultCall, 2. Marks goroutine as "in syscall" — allows GC to proceed 3. Calls our assembly wrapper -Since v0.5.4, the `runtime_cgocall` linkname declaration has `//go:noescape`, keeping `syscallArgs` on the goroutine stack (zero heap allocations). All ABI-boundary structs use `structs.HostLayout` (Go 1.23+) to guarantee C-compatible memory layout. +Since v0.5.6, `syscallArgs` is heap-allocated via `sync.Pool` — goroutine stacks may move during C→Go callbacks (`copystack`), and assembly on g0 holds the args pointer across the call. All ABI-boundary structs use `structs.HostLayout` (Go 1.23+) to guarantee C-compatible memory layout. 4. Restores Go stack on return We access it via `//go:linkname`: diff --git a/docs/PERFORMANCE.md b/docs/PERFORMANCE.md index df59637..ad773bf 100644 --- a/docs/PERFORMANCE.md +++ b/docs/PERFORMANCE.md @@ -37,7 +37,7 @@ - **Minimum FFI overhead**: ~88 ns (empty function) - **Typical overhead**: ~100-115 ns (with arguments) - **Overhead ratio**: ~400-500x vs direct Go call -- **Allocations**: 2-3 per call (runtime.cgocall internals). Since v0.5.4, `//go:noescape` on `runtime_cgocall` keeps `syscallArgs` on the goroutine stack, eliminating the primary per-call heap allocation on Unix platforms. +- **Allocations**: 0 in steady state. `syscallArgs` is heap-allocated via `sync.Pool` for callback safety (goroutine stack may move during C→Go callbacks). Pool reuse eliminates per-call allocations after warmup. ### 2. One-Time Costs diff --git a/ffi/callback_test.go b/ffi/callback_test.go index 454b3d1..79367f2 100644 --- a/ffi/callback_test.go +++ b/ffi/callback_test.go @@ -7,6 +7,8 @@ import ( "sync" "testing" "unsafe" + + "github.com/go-webgpu/goffi/types" ) const callbackFloatRegCount = 8 @@ -734,3 +736,64 @@ func BenchmarkCallbackFloat(b *testing.B) { callbackWrap(args) } } + +// growStack makes the calling goroutine's stack grow, and so move (copystack), by +// recursing with frames big enough to matter. It returns 0. The //go:noinline +// keeps every frame real so the recursion actually uses up stack. +// +//go:noinline +func growStack(n int) int { + if n == 0 { + return 0 + } + var pad [4096]byte + pad[0] = byte(n) + pad[len(pad)-1] = byte(n) + return int(pad[0]) - int(pad[len(pad)-1]) + growStack(n-1) +} + +// TestCallbackGrowStack guards against a callback from C into Go losing its +// argument or return value when the goroutine stack moves during the call. +// +// CallNFloat builds the argument and return block, and syscallN (on g0) writes +// the C return value back into it after the call. If that block sits on the +// goroutine stack and the callback grows the stack, the stack moves and +// syscallN's write lands at the old, pre-move address, so the return value is +// lost. +// +// The test makes this deterministic instead of relying on process-global warmup +// state. It runs on a fresh goroutine with a small starting stack and forces a +// large stack growth from inside the callback, right while syscallN is holding +// the pre-move address. growStack adds 0, so passing arg=42 must yield 42 back. +func TestCallbackGrowStack(t *testing.T) { + u := types.UInt64TypeDescriptor + var cif types.CallInterface + if err := PrepareCallInterface(&cif, types.DefaultCall, u, []*types.TypeDescriptor{u}); err != nil { + t.Fatal(err) + } + cb := NewCallback(func(arg uintptr) uintptr { + // About 2 MiB of stack growth, enough to force a copystack mid-call. + // growStack returns 0. + return arg + uintptr(growStack(512)) + }) + + done := make(chan uintptr, 1) + go func() { + arg, ret := uintptr(42), uintptr(0) + // cb is the trampoline's code address as a uintptr. Reinterpret its bits as + // an unsafe.Pointer rather than converting directly: a direct unsafe.Pointer(cb) + // trips vet's unsafeptr guard against conversions that hide a heap address from + // the GC, and the trampoline is code that is never GC-managed and never moved. + fn := *(*unsafe.Pointer)(unsafe.Pointer(&cb)) + if err := CallFunction(&cif, fn, unsafe.Pointer(&ret), []unsafe.Pointer{unsafe.Pointer(&arg)}); err != nil { + t.Error(err) + done <- ^uintptr(0) + return + } + done <- ret + }() + + if got := <-done; got != 42 { + t.Fatalf("callback returned %d, want 42 (argument or return value lost when the stack moved during the call)", got) + } +} diff --git a/ffi/ffi.go b/ffi/ffi.go index 7aa3a37..de488e1 100644 --- a/ffi/ffi.go +++ b/ffi/ffi.go @@ -248,6 +248,7 @@ func PrepareVariadicCallInterface( // - All argument pointers must remain valid during the call // - Return value buffer must be large enough for the result type // - Use runtime.KeepAlive() if needed to prevent premature GC of arguments +// - Use runtime.Pinner to pin pointers under a moving GC func CallFunctionContext( ctx context.Context, cif *types.CallInterface, diff --git a/ffi/struct_e2e_test.go b/ffi/struct_e2e_test.go index 7104edf..5fa885e 100644 --- a/ffi/struct_e2e_test.go +++ b/ffi/struct_e2e_test.go @@ -531,3 +531,47 @@ func TestCallbackStructArgWithScalar(t *testing.T) { t.Errorf("expected %#v %d, received %#v %d", expected, extra, receivedArg1, receivedArg2) } } + +// TestStructReturn24B exercises the sret path for a struct larger than 16 bytes. +// The caller provides a buffer in rvalue, goffi points the hidden return pointer +// (RDI on amd64, X8 on arm64) at it, and the callee writes the struct directly +// into that buffer. This covers the >16B struct return ABI, which the other +// struct-return tests (all <= 16 bytes) do not reach. +func TestStructReturn24B(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows struct returns use a separate ABI path (call_windows.go), not covered here") + } + requireStructLib(t) + + sym, err := GetSymbol(structTestLib, "return_struct_24") + if err != nil { + t.Fatal(err) + } + + tripleI64 := &types.TypeDescriptor{ + Kind: types.StructType, + Size: 24, + Alignment: 8, + Members: []*types.TypeDescriptor{ + types.SInt64TypeDescriptor, + types.SInt64TypeDescriptor, + types.SInt64TypeDescriptor, + }, + } + + var cif types.CallInterface + if err := PrepareCallInterface(&cif, types.DefaultCall, tripleI64, nil); err != nil { + t.Fatal(err) + } + + type TripleI64 struct{ A, B, C int64 } + var result TripleI64 + if err := CallFunction(&cif, sym, unsafe.Pointer(&result), nil); err != nil { + t.Fatal(err) + } + + want := TripleI64{11, 22, 33} + if result != want { + t.Fatalf("return_struct_24() = %+v, want %+v", result, want) + } +} diff --git a/ffi/testdata/structtest.c b/ffi/testdata/structtest.c index c3f022e..63511bf 100644 --- a/ffi/testdata/structtest.c +++ b/ffi/testdata/structtest.c @@ -86,6 +86,15 @@ struct return_pair_i64 return_struct_2ints(int64_t a, int64_t b) { return s; } +// 24-byte struct (> 16 bytes) returned by value through the sret ABI: the caller +// passes a hidden destination pointer (RDI on SysV AMD64, X8 on AAPCS64) and the +// callee writes the struct into it. The Go test calls this with a real rvalue +// buffer and checks the returned fields. triple_i64 is defined above. +struct triple_i64 return_struct_24(void) { + struct triple_i64 s = {.a = 11, .b = 22, .c = 33}; + return s; +} + // Variadic: sum N int64_t values. // Prototype: int64_t sum_variadic(int64_t count, ...) // nfixedargs = 1 (only 'count' is fixed). diff --git a/internal/arch/amd64/call_unix.go b/internal/arch/amd64/call_unix.go index 653072d..c9c1869 100644 --- a/internal/arch/amd64/call_unix.go +++ b/internal/arch/amd64/call_unix.go @@ -73,14 +73,9 @@ func (i *Implementation) Execute( // Detect sret: struct > 16 bytes requires hidden first argument in RDI. // The caller's rvalue buffer is passed as the first integer argument and // callee writes the return value directly into it. - sretBuf := unsafe.Pointer(nil) - if cif.ReturnType.Kind == types.StructType && cif.ReturnType.Size > 16 { - if rvalue != nil { - sretBuf = rvalue - } else { - sretBuf = unsafe.Pointer(&[128]byte{}) - } - addInt(uintptr(sretBuf)) + sret := cif.ReturnType.Kind == types.StructType && cif.ReturnType.Size > 16 + if sret { + addInt(uintptr(rvalue)) } // Map arguments to registers or stack @@ -222,10 +217,10 @@ func (i *Implementation) Execute( ret, r2, fret, fret2 := gosyscall.CallNFloat(uintptr(fn), gpr, sse, stackArgs, numStack) runtime.KeepAlive(avalue) - runtime.KeepAlive(sretBuf) + runtime.KeepAlive(rvalue) // If sret, the callee wrote directly into rvalue — no further copy needed. - if sretBuf != nil { + if sret { return nil } diff --git a/internal/arch/arm64/call_arm64.go b/internal/arch/arm64/call_arm64.go index 11c4277..5a0a795 100644 --- a/internal/arch/arm64/call_arm64.go +++ b/internal/arch/arm64/call_arm64.go @@ -197,7 +197,7 @@ func (i *Implementation) Execute( // Determine if we need to pass X8 for large struct return (sret) var r8 uintptr - if cif.Flags&types.ReturnViaPointer != 0 && rvalue != nil { + if cif.Flags&types.ReturnViaPointer != 0 { // For sret, pass rvalue pointer in X8 - callee writes directly to it r8 = uintptr(rvalue) } diff --git a/internal/syscall/syscall_arm64.go b/internal/syscall/syscall_arm64.go index 3ad96d5..c80cdcb 100644 --- a/internal/syscall/syscall_arm64.go +++ b/internal/syscall/syscall_arm64.go @@ -6,13 +6,18 @@ package syscall import ( "structs" + "sync" "unsafe" ) //go:linkname runtime_cgocall runtime.cgocall -//go:noescape func runtime_cgocall(fn uintptr, arg unsafe.Pointer) int32 +// syscallArgsPool recycles the per-call argument/return block. The block must +// live in non-moving memory (see callNFloat): pooling keeps it heap-resident and +// reused, so the move-safe path costs no per-call allocation in steady state. +var syscallArgsPool = sync.Pool{New: func() any { return new(syscallArgs) }} + // syscallArgs matches the layout expected by syscallN assembly. // AAPCS64 uses X0-X7 (8 GPRs) and D0-D7 (8 FPRs) for arguments. // Args 9+ (integer) or FP overflow are placed on the stack. @@ -63,7 +68,18 @@ func CallNFloat(fn uintptr, gpr [8]uintptr, fpr [8]uint64, stackArgs [7]uintptr, } func callNFloat(fn uintptr, gpr [8]uintptr, fpr [8]uint64, stackArgs [7]uintptr, numStack int, r8 uintptr) (r1 uintptr, r2 uintptr, fret [4]uint64) { - args := syscallArgs{ + // args holds both the call arguments and the C function's return values. + // syscallN runs on g0 and writes the return values back into args after the + // call returns. That call can run a callback that re-enters Go and grows this + // goroutine's stack, which moves it. If args lived on the stack it would move + // too, syscallN's write would land at the old address, and the first such call + // would lose its return value. So args has to live in non-moving memory: we + // take it from the heap-backed pool. The heap does not move on today's Go + // (non-compacting GC, confirmed through Go 1.27). If a compacting GC ever + // lands, add runtime.Pinner here. + args := syscallArgsPool.Get().(*syscallArgs) + defer syscallArgsPool.Put(args) + *args = syscallArgs{ fn: fn, a1: gpr[0], a2: gpr[1], a3: gpr[2], a4: gpr[3], a5: gpr[4], a6: gpr[5], a7: gpr[6], a8: gpr[7], @@ -87,7 +103,9 @@ func callNFloat(fn uintptr, gpr [8]uintptr, fpr [8]uint64, stackArgs [7]uintptr, r8: r8, // X8 for large struct returns } _ = numStack // informational; assembly always pushes all 7 stack slots - runtime_cgocall(syscallNABI0, unsafe.Pointer(&args)) + + runtime_cgocall(syscallNABI0, unsafe.Pointer(args)) + r1 = args.r1 r2 = args.r2 fret[0] = uint64(args.fr1) diff --git a/internal/syscall/syscall_unix_amd64.go b/internal/syscall/syscall_unix_amd64.go index c900326..5b9d9f7 100644 --- a/internal/syscall/syscall_unix_amd64.go +++ b/internal/syscall/syscall_unix_amd64.go @@ -6,13 +6,18 @@ package syscall import ( "structs" + "sync" "unsafe" ) //go:linkname runtime_cgocall runtime.cgocall -//go:noescape func runtime_cgocall(fn uintptr, arg unsafe.Pointer) int32 +// syscallArgsPool recycles the per-call argument/return block. The block must +// live in non-moving memory (see CallNFloat): pooling keeps it heap-resident and +// reused, so the move-safe path costs no per-call allocation in steady state. +var syscallArgsPool = sync.Pool{New: func() any { return new(syscallArgs) }} + // syscallArgs matches the layout expected by syscallN assembly. // Supports up to 15 total arguments (6 GP registers + 9 stack slots), // matching purego's syscall15Args layout. @@ -54,7 +59,18 @@ var syscallNABI0 uintptr // - f1: XMM0 float return value (bit pattern) // - f2: XMM1 second float return value — for {SSE, SSE} 9-16B struct returns (e.g. NSPoint) func CallNFloat(fn uintptr, gpr [6]uintptr, sse [8]float64, stackArgs [9]uintptr, numStack int) (r1 uintptr, r2 uintptr, f1 float64, f2 float64) { - args := syscallArgs{ + // args holds both the call arguments and the C function's return values. + // syscallN runs on g0 and writes the return values back into args after the + // call returns. That call can run a callback that re-enters Go and grows this + // goroutine's stack, which moves it. If args lived on the stack it would move + // too, syscallN's write would land at the old address, and the first such call + // would lose its return value. So args has to live in non-moving memory: we + // take it from the heap-backed pool. The heap does not move on today's Go + // (non-compacting GC, confirmed through Go 1.27). If a compacting GC ever + // lands, add runtime.Pinner here. + args := syscallArgsPool.Get().(*syscallArgs) + defer syscallArgsPool.Put(args) + *args = syscallArgs{ fn: fn, a1: gpr[0], a2: gpr[1], a3: gpr[2], a4: gpr[3], a5: gpr[4], a6: gpr[5], @@ -79,7 +95,9 @@ func CallNFloat(fn uintptr, gpr [6]uintptr, sse [8]float64, stackArgs [9]uintptr f8: *(*uintptr)(unsafe.Pointer(&sse[7])), } _ = numStack // numStack is informational; assembly always pushes all 9 slots - runtime_cgocall(syscallNABI0, unsafe.Pointer(&args)) + + runtime_cgocall(syscallNABI0, unsafe.Pointer(args)) + r1 = args.r1 r2 = args.r2 f1 = *(*float64)(unsafe.Pointer(&args.f1))