Repository navigation
docs: record what callbacks accept per platform, and the arm64 aggreg… - #84
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
3e1e0d1 to
3f9133f
Compare
kolkov
left a comment
There was a problem hiding this comment.
Thanks — a page that tells an unimplemented case from a broken one is exactly what was missing, and the arm64 section is a good starting point for whoever implements AAPCS64 aggregates.
While verifying the table against the code I ran real C→Go callbacks (not the hand-built frames from callback_test.go), and a row is wrong. This is a pre-existing bug, not something your PR introduced, but the page would publish it as supported:
float32 in callbacks is broken on amd64 Unix (linux/amd64, glibc 2.35, current main):
int call_fi(int (*f)(float, float), float a, float b) { return f(a, b); }
int call_di(int (*f)(double, double), double a, double b) { return f(a, b); }
float call_if(float (*f)(long), long a) { return f(a); }
double call_id(double (*f)(long), long a) { return f(a); }float32 args: Go saw a=0 b=0 (want 2.5 4)
float32 return: C got 0 (want 6)
float64 args: Go saw a=2.5 b=4 ok
float64 return: C got 6 ok
Why:
- Args: the C caller puts a
floatin the low 32 bits of XMMn as single precision.callbackWrapreads the 64-bit slot as afloat64and converts it withfloat32(getFloat()), which turns the bit pattern into garbage. It should bemath.Float32frombits(uint32(bits)).TestCallback_Float32Argsdoesn't catch this because it fills the slots withfloat64bit patterns, i.e. it mirrors the bug rather than the ABI. - Returns:
callbackDispatcheronly moves the result intoAX(R0on arm64). Nothing writes XMM0 / D0.float64returns currently come out right, apparently because X0 still happens to hold the value fromret.Float()when the dispatcher returns. That's luck, not a contract.float32returns come out wrong because the low half of afloat64bit pattern isn't thefloat32. - arm64 has the same code shape (
FSTPDsaves D0–D7, the arm64 dispatcher returns onlyR0, andfloat32args go throughfloat32(f64)). I couldn't run arm64 here, so for arm64 this comes from reading the code, not from a test run.
We'll track the fix separately (#93). For this PR, could you:
- Change the
float32 / float64column so it says what's actually true today. Something like "float64 args: yes; float64 returns: correct today, but not guaranteed by the dispatcher; float32 args and returns: broken, see #93". Then update it when the fix lands. - Add two caveats to the amd64 struct row, since "yes" overstates it. First, register exhaustion isn't handled: SysV passes a 9–16-byte aggregate in memory if its eightbytes don't all fit in the remaining registers. gcc confirms it: a
struct { long x, y; }after fivelongs goes on the stack and R9 stays unused.callbackWrapwould still take the first eightbyte from R9. Second,classifyEightbytedoesn't recurse into nested structs (its own comment says so). - Optional: the Apple note applies to scalars too, not only aggregates. Apple's ABI doc says "Function arguments may consume slots on the stack that are not multiples of 8 bytes" (a
chartakes one byte), while the arm64callbackWrapreads every stack argument as an 8-byte slot. Going by the code, a callback with more than 8 integer arguments narrower than 8 bytes is misread on darwin/arm64 today. I haven't run that on a Mac.
The callback_arm64.go comment pointing at the page is a nice touch. Happy to approve once the table matches reality.
…ate gap
NewCallback answers an unsupported signature with a panic, and two of the
three limits behind those panics are undocumented, so they read as bugs:
panic: ffi: unsupported callback argument type: struct
panic: ffi: Windows callbacks require uintptr-sized return type, got int32
docs/CALLBACK_ABI.md puts the three rows in one table -- amd64 Unix takes
struct-by-value arguments, arm64 does not, Windows takes uintptr-sized
arguments and one uintptr-sized result and nothing else -- and separates the
limit that is permanent from the one that is not. Windows is the platform's
own contract: NewCallback delegates to syscall.NewCallback, which accepts
nothing more without cgo, and purego documents the same restriction. arm64 is
unfinished work.
For that one the page carries what an implementer needs rather than a wish:
where amd64 does the equivalent (callbackWrap's eightbyte classification in
ffi/callback.go), the four AAPCS64 cases to cover, the two that are easy to
get wrong -- a four-float32 HFA occupying S0-S3, i.e. the low halves of four V
registers the frame stores as 64-bit slots, and register exhaustion sending
the whole aggregate to the stack with no backfill -- and Apple's naturally
aligned stack packing, which means any implementation has to be checked
against both a Linux and an Apple toolchain. It also points at
ffi/callback_struct_args_test.go as the model for testing classification with
a hand-built frame, no C toolchain and no real foreign call needed.
Callers get the workaround in one line: take a pointer instead of a value.
The panic site in ffi/callback_arm64.go now points at the page, so the message
someone actually sees leads to the explanation.
isStructAllFloats is gone; the amd64 callback path now takes its register and stack assignment from the call path's System V allocator. Link the page from README and ARCHITECTURE.md and record it in the CHANGELOG.
3f9133f to
11156ba
Compare
kolkov
left a comment
There was a problem hiding this comment.
Everything I asked for here has since been fixed on main rather than in the table: float32 arguments and float results in callbacks (#93), System V register exhaustion and nested structs, and Apple's natural-size stack packing. So the table is now right as you wrote it.
I rebased the branch onto main. Your Android CI commit was already there through #85, so the rebase dropped it. I also added one commit on top: the page referred to isStructAllFloats, which no longer exists, so I fixed the name; it also links the page from the README and ARCHITECTURE.md and adds a CHANGELOG entry with your name. Thanks for writing this up, and especially the arm64 section: it is the spec for whoever implements AAPCS64 struct callbacks.
…ate gap
NewCallback answers an unsupported signature with a panic, and two of the three limits behind those panics are undocumented, so they read as bugs:
docs/CALLBACK_ABI.md puts the three rows in one table -- amd64 Unix takes struct-by-value arguments, arm64 does not, Windows takes uintptr-sized arguments and one uintptr-sized result and nothing else -- and separates the limit that is permanent from the one that is not. Windows is the platform's own contract: NewCallback delegates to syscall.NewCallback, which accepts nothing more without cgo, and purego documents the same restriction. arm64 is unfinished work.
For that one the page carries what an implementer needs rather than a wish: where amd64 does the equivalent (callbackWrap's eightbyte classification in ffi/callback.go), the four AAPCS64 cases to cover, the two that are easy to get wrong -- a four-float32 HFA occupying S0-S3, i.e. the low halves of four V registers the frame stores as 64-bit slots, and register exhaustion sending the whole aggregate to the stack with no backfill -- and Apple's naturally aligned stack packing, which means any implementation has to be checked against both a Linux and an Apple toolchain. It also points at ffi/callback_struct_args_test.go as the model for testing classification with a hand-built frame, no C toolchain and no real foreign call needed.
Callers get the workaround in one line: take a pointer instead of a value.
The panic site in ffi/callback_arm64.go now points at the page, so the message someone actually sees leads to the explanation.