purego: return 64-bit callback results on 386 - #519
Conversation
The 386 trampoline allocated only one result word and returned EAX alone, so int64/uint64 callbacks lost their upper half (cdecl wants EDX:EAX) and the args copy overlapped result[1..]. Widen the callbackArgs area to the full Go struct, shift the args copy by 12 bytes, and return both halves.
|
Can we have tests? |
Add callback tests that return int64 and uint64 values whose upper word is significant, which covers the EDX:EAX half of the 386 trampoline, and one with stack arguments so the copied arguments are checked against the result words. While at it, write the last copied argument to 300(SP) instead of clobbering 288(SP); the shift of the argument area left that word behind.
|
Tests added in callback_test.go:
I verified them with While doing that I noticed that the last copied argument was still written to 288(SP) instead of 300(SP) after the argument area was shifted, so argument slot 63 was never written and slot 60 was overwritten. That is fixed as well, along with the frame layout comments. |
|
Fix the test failure |
RegisterFunc rejects a function with more stack arguments than maxArgs - numOfIntegerRegisters() allows, and on ppc64le that leaves only 7 stack slots out of maxArgs = 15. The 20 int arguments of TestNewCallbackInt64ResultWithStackArgs overflowed this and panicked with "purego: too many stack arguments", aborting the whole test binary on Linux ppc64le. Passing 12 arguments still puts every argument on the stack on 386 and still spills onto the stack on arm and on the 64-bit architectures, so the shifted argument copy stays covered on all platforms.
|
The failure was on Linux ppc64le: The test now passes 12 arguments. That is within every supported architecture's limit and still puts all the arguments on the stack on 386, and still spills onto the stack on arm (8), s390x (7) and the other 64-bit architectures (4), so the shifted argument copy remains covered. Verified locally:
|
hajimehoshi
left a comment
hajimehoshi
left a comment
There was a problem hiding this comment.
LGTM (but TBH I don't fully understand the code...)
@TotallyGamerJet PTAL
2 participants
What issue is this addressing?
Closes #524
What type of issue is this addressing?
bug
What this PR does | solves
The 386 trampoline allocated only one result word and returned EAX alone, so int64/uint64 callbacks lost their upper half (cdecl wants EDX:EAX) and the args copy overlapped result[1..]. Widen the callbackArgs area to the full Go struct, shift the args copy by 12 bytes, and return both halves.