Skip to content

Commit 6330e63

Browse files
authored
fix: struct-by-value argument passing on AMD64 (closes #33) (#39)
* fix: struct by-value arg passing on AMD64 Unix (System V ABI) Add StructType case to Execute() argument dispatch in call_unix.go. Previously structs fell through to default:, passing the pointer instead of the struct's bytes. The fix implements three size buckets: - <= 8 bytes: single eightbyte, SSE if all members are float/double, INTEGER otherwise; uses sized reads to avoid overread. - 9-16 bytes: two eightbytes, each classified independently via classifyEightbyte(); INTEGER wins over SSE per SysV ABI §3.2.3. - > 16 bytes: MEMORY class, bytes copied to stack in 8-byte chunks via addInt() with a sized read for the final partial chunk. Add isStructAllFloats() and classifyEightbyte() helper functions. * fix: correct classifyArgumentAMD64 for struct MEMORY class and per-eightbyte SSE classification Two bugs fixed in classifyArgumentAMD64: 1. Structs > 16 bytes are MEMORY class (SysV ABI §3.2.3) — passed on the stack by copying, consuming zero GP or SSE registers. The old code incorrectly set GPRCount = ceil(size/8), causing the CIF to reserve register slots for memory-class arguments. 2. For <= 16-byte structs the old code broke after finding the first float member and decremented GPRCount by 1 regardless of which eightbyte the member occupied. The fix classifies each eightbyte independently using classifyEightbyte(), applying the ABI merge rule (INTEGER wins over SSE) per eightbyte boundary. Move isStructAllFloats and classifyEightbyte helpers from call_unix.go into classification.go so they compile on all amd64 targets (including Windows). Update Struct24B_large test expectation to {0, 0}. * fix: struct by-value arg passing on AMD64 Windows (Win64 ABI) Add StructType case to Execute() in call_windows.go. Windows x64 ABI rule: structs whose size is exactly 1, 2, 4, or 8 bytes are passed by value in an integer register slot (the struct bytes are placed directly in RCX/RDX/R8/R9 or the corresponding stack slot). Structs of any other size are passed by reference — the caller provides a pointer to a caller-allocated copy of the struct. The previous default: case already passed the pointer for non-1/2/4/8 sizes, which was accidentally correct for those, but 1/2/4/8-byte structs were also being passed as pointers instead of values. * test: add unit tests for AMD64 struct argument passing helpers Add struct_args_test.go with 25 tests covering: - TestIsStructAllFloats: empty struct, pure float/double structs, mixed float+int (INTEGER wins), pointer-only — 8 cases - TestClassifyEightbyte: integer pair, float pair, mixed (INTEGER wins), split double/uint64 across eightbyte boundary, no field in range — 6 cases - TestStructValueRead{1,2,4,8}Byte: verify sized reads produce the correct uintptr values — 4 cases - TestStructValueReadSecondEightbyte: verify unsafe.Add offset for 16-byte struct second eightbyte reads correctly — 1 case - TestClassifyArgumentAMD64Structs: 7 struct classification cases including MEMORY class (> 16 bytes → 0 GPR + 0 SSE), INTEGER wins, pure SSE, and split SSE+INTEGER across eightbytes * fix: use double-indirection for unsafe.Pointer conversions (checkptr) Replace direct uintptr→unsafe.Pointer casts with the double-indirection pattern from accepted Go proposal #58625. This satisfies checkptr validation when running with -race, without disabling any safety checks. Affected: callback.go (4 conversions), implementation.go (1 conversion). Verified: CGO_ENABLED=1 go test -race passes on Linux amd64. * docs: update CHANGELOG, README, ARCHITECTURE for struct args and race detector - CHANGELOG: add Unreleased section with struct fix, CGO=1 support, checkptr fix - README: update feature table (struct pass/return, race detector, CI modes) - ARCHITECTURE: add Struct Argument Passing section (SysV, Win64, AAPCS64 rules) * fix: >16B struct MEMORY class must bypass registers (addStack) E2E tests revealed that >16B structs were placed in GP registers via addInt instead of directly on stack. Per SysV ABI §3.2.3, MEMORY class structs bypass registers entirely. Added addStack helper and e2e test suite with gcc-compiled test library (5 struct passing scenarios).
1 parent e664be6 commit 6330e63

12 files changed

Lines changed: 836 additions & 21 deletions

File tree

CHANGELOG.md

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
77

88
## [Unreleased]
99

10+
### Fixed
11+
- **AMD64: struct-by-value argument passing** — structs passed as arguments were sent as raw pointers instead of their bytes. Now correctly handles ≤8B (single eightbyte, INTEGER/SSE classification), 9-16B (two eightbytes classified independently), and >16B (MEMORY class, copied directly to stack bypassing registers). Follows System V AMD64 ABI §3.2.3. ([#33](https://github.com/go-webgpu/goffi/issues/33))
12+
- **AMD64 Windows: struct argument passing** — structs of exactly 1, 2, 4, or 8 bytes are now passed by value per Win64 ABI, not by pointer
13+
- **Race detector compatibility** — replaced direct `uintptr→unsafe.Pointer` casts with double-indirection pattern (Go proposal [#58625](https://github.com/golang/go/issues/58625)) in callback dispatch and return handling. `CGO_ENABLED=1 go test -race` now passes cleanly
14+
- **Classification: >16B structs**`classifyArgumentAMD64` now correctly returns zero register usage for MEMORY class structs (previously claimed GP registers)
15+
- **Classification: mixed eightbyte** — per-eightbyte SSE/INTEGER classification now walks all members with INTEGER-wins merge rule per System V ABI
16+
17+
### Added
18+
- `CGO_ENABLED=1` support ([#13](https://github.com/go-webgpu/goffi/issues/13), PR [#37](https://github.com/go-webgpu/goffi/pull/37) by [@jiyeyuran](https://github.com/jiyeyuran)) — goffi now builds and tests under both `CGO_ENABLED=0` (fakecgo) and `CGO_ENABLED=1` (real `runtime/cgo`). Enables race detector, coexistence with CGO libraries (gocv, database drivers, etc.), and resolves [#22](https://github.com/go-webgpu/goffi/issues/22) duplicate symbol conflict as alternative workaround
19+
- C-thread callback test — `TestCallback_FromCThread` verifies `NewCallback` works when invoked from a `pthread_create`-spawned C thread under both CGO modes
20+
- Unit tests for struct argument classification and value packing (25 test cases)
21+
- **End-to-end struct argument tests**`ffi/struct_e2e_test.go` compiles a C test library (`testdata/structtest.c`) via gcc at test time and validates 5 struct passing scenarios: ≤8B integer pair (issue #33 repro), ≤8B float pair (SSE), 16B two-eightbyte, >16B MEMORY class, and struct+scalar mixed arguments. Runs on Linux/macOS/FreeBSD where gcc is available; skipped gracefully elsewhere
22+
1023
## [0.5.0] - 2026-03-29
1124

1225
### Added

README.md

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,10 @@ ffi.CallFunction(cif, sym, unsafe.Pointer(&result), args)
3535
| **Cross-platform** | 7 targets | Windows, Linux, macOS, FreeBSD × AMD64 + ARM64 |
3636
| **Callbacks** | C→Go safe | `crosscall2` integration, works from any C thread |
3737
| **Type-safe** | Runtime validation | 5 typed error types with `errors.As()` support |
38-
| **Struct passing** | Full ABI | ≤8B (RAX), 9–16B (RAX+RDX), >16B (sret) |
38+
| **Struct pass/return** | Full ABI | Args: INTEGER/SSE classification. Returns: ≤8B (RAX), 9–16B (RAX+RDX), >16B (sret) |
3939
| **Context** | Timeouts | `CallFunctionContext(ctx, ...)` cancellation |
40-
| **Tested** | 89% coverage | CI on Linux, Windows, macOS |
40+
| **Race detector** | `-race` compatible | `CGO_ENABLED=1 go test -race` works cleanly |
41+
| **Tested** | 89% coverage | CI on Linux, Windows, macOS (CGO=0 and CGO=1) |
4142

4243
---
4344

docs/ARCHITECTURE.md

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,47 @@ TEXT syscallN(SB), NOSPLIT|NOFRAME, $0
132132

133133
---
134134

135+
## Struct Argument Passing
136+
137+
ABI rules for passing structs as arguments depend on size and platform:
138+
139+
### System V AMD64 (Linux, macOS, FreeBSD)
140+
141+
Per §3.2.3, each struct is classified by its eightbytes (8-byte chunks):
142+
143+
- **≤ 8 bytes**: single eightbyte. If all fields are float/double → SSE (XMM register). Otherwise → INTEGER (GP register). INTEGER wins over SSE within the same eightbyte (merge rule).
144+
- **9-16 bytes**: two eightbytes, each classified independently. First 8 bytes → GP or XMM. Remaining bytes → GP or XMM. Both classifications use the same INTEGER-wins merge rule.
145+
- **\> 16 bytes**: MEMORY class. Caller copies struct bytes onto the stack in 8-byte chunks.
146+
147+
Implementation in `internal/arch/amd64/call_unix.go`, helpers in `classification.go`:
148+
- `isStructAllFloats(t)` — returns true if all members are float/double
149+
- `classifyEightbyte(t, startOff, endOff)` — per-eightbyte SSE classification with merge rule
150+
151+
### Win64 (Windows AMD64)
152+
153+
- **Exactly 1, 2, 4, or 8 bytes**: passed as integer by value (same register slot)
154+
- **All other sizes**: passed by reference — caller passes a pointer
155+
156+
### AAPCS64 (ARM64)
157+
158+
- **≤ 16 bytes**: passed in GP registers (up to 2)
159+
- **HFA (Homogeneous Floating-point Aggregate)**: up to 4 same-type floats → D0-D3
160+
- **\> 16 bytes**: passed by reference
161+
162+
### End-to-End Testing
163+
164+
Struct argument passing is verified by `ffi/struct_e2e_test.go`, which compiles a C test library (`testdata/structtest.c`) via gcc at test time. Five scenarios are tested:
165+
166+
1. **≤8B integer pair** (`{int32, uint32}`) — INTEGER class, single GP register
167+
2. **≤8B float pair** (`{float, float}`) — SSE class, single XMM register
168+
3. **16B integer pair** (`{int64, int64}`) — two INTEGER eightbytes, two GP registers
169+
4. **24B triple** (`{int64, int64, int64}`) — MEMORY class, copied to stack
170+
5. **Struct + scalar** — mixed register allocation
171+
172+
Tests run on Linux, macOS, and FreeBSD where gcc is available; skipped gracefully on Windows.
173+
174+
---
175+
135176
## Struct Return Handling
136177
137178
ABI rules for returning structs depend on size:

ffi/callback.go

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -251,28 +251,24 @@ func callbackWrap(a *callbackArgs) {
251251
// 3. reflect.NewAt creates a proper typed pointer from the address
252252
if intIdx < numIntRegs {
253253
pos := numFloatRegs + intIdx
254-
//nolint:govet,gosec // G103: Converting uintptr from CPU register to pointer for FFI callback argument
255-
ptr := unsafe.Pointer(frame[pos])
254+
// Double-indirection: reinterpret uintptr bits as pointer without
255+
// triggering checkptr arithmetic check (go.dev/issue/58625).
256+
ptr := *(*unsafe.Pointer)(unsafe.Pointer(&frame[pos]))
256257
val = reflect.NewAt(argType.Elem(), ptr)
257258
intIdx++
258259
} else {
259-
//nolint:govet,gosec // G103: Converting uintptr from stack to pointer for FFI callback argument
260-
ptr := unsafe.Pointer(frame[stackIdx])
260+
ptr := *(*unsafe.Pointer)(unsafe.Pointer(&frame[stackIdx]))
261261
val = reflect.NewAt(argType.Elem(), ptr)
262262
stackIdx++
263263
}
264264

265265
case reflect.UnsafePointer:
266-
// UnsafePointer comes from integer registers.
267-
// Converting uintptr to unsafe.Pointer is necessary for FFI interop.
268266
if intIdx < numIntRegs {
269267
pos := numFloatRegs + intIdx
270-
//nolint:govet,gosec // G103: Converting uintptr from register to unsafe.Pointer for FFI
271-
val = reflect.ValueOf(unsafe.Pointer(frame[pos]))
268+
val = reflect.ValueOf(*(*unsafe.Pointer)(unsafe.Pointer(&frame[pos])))
272269
intIdx++
273270
} else {
274-
//nolint:govet,gosec // G103: Converting uintptr from stack to unsafe.Pointer for FFI
275-
val = reflect.ValueOf(unsafe.Pointer(frame[stackIdx]))
271+
val = reflect.ValueOf(*(*unsafe.Pointer)(unsafe.Pointer(&frame[stackIdx])))
276272
stackIdx++
277273
}
278274

ffi/struct_e2e_test.go

Lines changed: 278 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,278 @@
1+
// SPDX-License-Identifier: Apache-2.0
2+
// SPDX-FileCopyrightText: 2026 The Goffi Authors
3+
4+
//go:build (linux || darwin || freebsd) && amd64
5+
6+
package ffi
7+
8+
import (
9+
"os"
10+
"os/exec"
11+
"path/filepath"
12+
"runtime"
13+
"testing"
14+
"unsafe"
15+
16+
"github.com/go-webgpu/goffi/types"
17+
)
18+
19+
var structTestLib unsafe.Pointer
20+
21+
func TestMain(m *testing.M) {
22+
if err := buildStructTestLib(); err != nil {
23+
// If gcc not available, skip struct e2e tests gracefully.
24+
// Other tests still run.
25+
structTestLib = nil
26+
}
27+
code := m.Run()
28+
if structTestLib != nil {
29+
FreeLibrary(structTestLib)
30+
}
31+
os.Exit(code)
32+
}
33+
34+
func buildStructTestLib() error {
35+
srcPath := filepath.Join("testdata", "structtest.c")
36+
soPath := filepath.Join("testdata", "libstructtest.so")
37+
if runtime.GOOS == "darwin" {
38+
soPath = filepath.Join("testdata", "libstructtest.dylib")
39+
}
40+
41+
cc := os.Getenv("CC")
42+
if cc == "" {
43+
cc = "gcc"
44+
}
45+
cmd := exec.Command(cc, "-shared", "-fPIC", "-O2", "-o", soPath, srcPath)
46+
cmd.Stderr = os.Stderr
47+
if err := cmd.Run(); err != nil {
48+
return err
49+
}
50+
51+
absPath, err := filepath.Abs(soPath)
52+
if err != nil {
53+
return err
54+
}
55+
lib, err := LoadLibrary(absPath)
56+
if err != nil {
57+
return err
58+
}
59+
structTestLib = lib
60+
return nil
61+
}
62+
63+
func requireStructLib(t *testing.T) {
64+
t.Helper()
65+
if structTestLib == nil {
66+
t.Skip("structtest library not available (gcc required)")
67+
}
68+
}
69+
70+
// TestStructArg8B_IntegerPair tests issue #33: struct {int32, uint32} passed by value.
71+
func TestStructArg8B_IntegerPair(t *testing.T) {
72+
requireStructLib(t)
73+
74+
sym, err := GetSymbol(structTestLib, "take_struct_8")
75+
if err != nil {
76+
t.Fatal(err)
77+
}
78+
79+
structType := &types.TypeDescriptor{
80+
Kind: types.StructType,
81+
Size: 8,
82+
Alignment: 4,
83+
Members: []*types.TypeDescriptor{
84+
types.SInt32TypeDescriptor,
85+
types.UInt32TypeDescriptor,
86+
},
87+
}
88+
89+
var cif types.CallInterface
90+
if err := PrepareCallInterface(&cif, types.DefaultCall, types.SInt64TypeDescriptor,
91+
[]*types.TypeDescriptor{structType}); err != nil {
92+
t.Fatal(err)
93+
}
94+
95+
type Pair struct {
96+
A int32
97+
B uint32
98+
}
99+
s := Pair{A: 42, B: 19}
100+
args := []unsafe.Pointer{unsafe.Pointer(&s)}
101+
var result int64
102+
if err := CallFunction(&cif, sym, unsafe.Pointer(&result), args); err != nil {
103+
t.Fatal(err)
104+
}
105+
106+
expected := int64(42)*1000 + int64(19)
107+
if result != expected {
108+
t.Errorf("take_struct_8({42, 19}) = %d, want %d", result, expected)
109+
}
110+
}
111+
112+
// TestStructArg8B_FloatPair tests SSE classification: struct {float, float}.
113+
func TestStructArg8B_FloatPair(t *testing.T) {
114+
requireStructLib(t)
115+
116+
sym, err := GetSymbol(structTestLib, "take_struct_2floats")
117+
if err != nil {
118+
t.Fatal(err)
119+
}
120+
121+
structType := &types.TypeDescriptor{
122+
Kind: types.StructType,
123+
Size: 8,
124+
Alignment: 4,
125+
Members: []*types.TypeDescriptor{
126+
types.FloatTypeDescriptor,
127+
types.FloatTypeDescriptor,
128+
},
129+
}
130+
131+
var cif types.CallInterface
132+
if err := PrepareCallInterface(&cif, types.DefaultCall, types.FloatTypeDescriptor,
133+
[]*types.TypeDescriptor{structType}); err != nil {
134+
t.Fatal(err)
135+
}
136+
137+
type PairF32 struct {
138+
X float32
139+
Y float32
140+
}
141+
s := PairF32{X: 2.5, Y: 3.5}
142+
args := []unsafe.Pointer{unsafe.Pointer(&s)}
143+
var result float32
144+
if err := CallFunction(&cif, sym, unsafe.Pointer(&result), args); err != nil {
145+
t.Fatal(err)
146+
}
147+
148+
if result != 6.0 {
149+
t.Errorf("take_struct_2floats({2.5, 3.5}) = %f, want 6.0", result)
150+
}
151+
}
152+
153+
// TestStructArg16B tests two-eightbyte struct: {int64, int64}.
154+
func TestStructArg16B(t *testing.T) {
155+
requireStructLib(t)
156+
157+
sym, err := GetSymbol(structTestLib, "take_struct_16")
158+
if err != nil {
159+
t.Fatal(err)
160+
}
161+
162+
structType := &types.TypeDescriptor{
163+
Kind: types.StructType,
164+
Size: 16,
165+
Alignment: 8,
166+
Members: []*types.TypeDescriptor{
167+
types.SInt64TypeDescriptor,
168+
types.SInt64TypeDescriptor,
169+
},
170+
}
171+
172+
var cif types.CallInterface
173+
if err := PrepareCallInterface(&cif, types.DefaultCall, types.SInt64TypeDescriptor,
174+
[]*types.TypeDescriptor{structType}); err != nil {
175+
t.Fatal(err)
176+
}
177+
178+
type PairI64 struct {
179+
A int64
180+
B int64
181+
}
182+
s := PairI64{A: 1000000, B: 2000000}
183+
args := []unsafe.Pointer{unsafe.Pointer(&s)}
184+
var result int64
185+
if err := CallFunction(&cif, sym, unsafe.Pointer(&result), args); err != nil {
186+
t.Fatal(err)
187+
}
188+
189+
if result != 3000000 {
190+
t.Errorf("take_struct_16({1000000, 2000000}) = %d, want 3000000", result)
191+
}
192+
}
193+
194+
// TestStructArg24B_MemoryClass tests > 16B struct passed on stack (MEMORY class).
195+
func TestStructArg24B_MemoryClass(t *testing.T) {
196+
requireStructLib(t)
197+
198+
sym, err := GetSymbol(structTestLib, "take_struct_24")
199+
if err != nil {
200+
t.Fatal(err)
201+
}
202+
203+
structType := &types.TypeDescriptor{
204+
Kind: types.StructType,
205+
Size: 24,
206+
Alignment: 8,
207+
Members: []*types.TypeDescriptor{
208+
types.SInt64TypeDescriptor,
209+
types.SInt64TypeDescriptor,
210+
types.SInt64TypeDescriptor,
211+
},
212+
}
213+
214+
var cif types.CallInterface
215+
if err := PrepareCallInterface(&cif, types.DefaultCall, types.SInt64TypeDescriptor,
216+
[]*types.TypeDescriptor{structType}); err != nil {
217+
t.Fatal(err)
218+
}
219+
220+
type TripleI64 struct {
221+
A int64
222+
B int64
223+
C int64
224+
}
225+
s := TripleI64{A: 100, B: 200, C: 300}
226+
args := []unsafe.Pointer{unsafe.Pointer(&s)}
227+
var result int64
228+
if err := CallFunction(&cif, sym, unsafe.Pointer(&result), args); err != nil {
229+
t.Fatal(err)
230+
}
231+
232+
if result != 600 {
233+
t.Errorf("take_struct_24({100, 200, 300}) = %d, want 600", result)
234+
}
235+
}
236+
237+
// TestStructArgWithScalar tests struct + scalar argument (register allocation).
238+
func TestStructArgWithScalar(t *testing.T) {
239+
requireStructLib(t)
240+
241+
sym, err := GetSymbol(structTestLib, "take_struct_and_int")
242+
if err != nil {
243+
t.Fatal(err)
244+
}
245+
246+
structType := &types.TypeDescriptor{
247+
Kind: types.StructType,
248+
Size: 8,
249+
Alignment: 4,
250+
Members: []*types.TypeDescriptor{
251+
types.SInt32TypeDescriptor,
252+
types.UInt32TypeDescriptor,
253+
},
254+
}
255+
256+
var cif types.CallInterface
257+
if err := PrepareCallInterface(&cif, types.DefaultCall, types.SInt64TypeDescriptor,
258+
[]*types.TypeDescriptor{structType, types.SInt64TypeDescriptor}); err != nil {
259+
t.Fatal(err)
260+
}
261+
262+
type Pair struct {
263+
A int32
264+
B uint32
265+
}
266+
s := Pair{A: 10, B: 20}
267+
extra := int64(1000)
268+
args := []unsafe.Pointer{unsafe.Pointer(&s), unsafe.Pointer(&extra)}
269+
var result int64
270+
if err := CallFunction(&cif, sym, unsafe.Pointer(&result), args); err != nil {
271+
t.Fatal(err)
272+
}
273+
274+
expected := int64(10) + int64(20) + int64(1000)
275+
if result != expected {
276+
t.Errorf("take_struct_and_int({10, 20}, 1000) = %d, want %d", result, expected)
277+
}
278+
}

0 commit comments

Comments
 (0)