purego: respect field offsets in amd64 struct packing - #515
Conversation
tryPlaceRegister packed fields back-to-back and overwrote pending
small fields when a 64-bit field followed (val = instead of |=),
dropping the first eightbyte and shifting all later arguments by one
slot. Small fields also ignored padding, misplacing e.g. the int32 at
offset 4 in {int8; int32}. Track each field's in-memory offset: flush
the pending eightbyte on crossing, place wide fields directly, and
realign the bit cursor for small fields. The recursive place() also
clobbered the outer eightbyte index with the inner tail position,
misclassifying later sibling fields of nested structs (e.g.
StructInStruct lost B and C); save and restore the cursor around
recursion.
|
If this is a bug fix, file an issue first |
Cover the struct argument shapes fixed in tryPlaceRegister: a small field followed by a wide field crossing the eightbyte boundary, a small field after padding, a struct between scalar arguments, and a nested struct followed by a sibling field. These identity round trips fail on main, where the first eightbyte was dropped and fields were packed back-to-back, and pass with the offset-aware packing.
|
Done — I've filed #520 for this bug and updated the PR description to reference it. This PR now also adds regression tests that reproduce the broken packing described there: identity round trips for {int8; int64}, {int8; int32}, a struct passed between scalar arguments, and a padded nested struct followed by a sibling field. They fail on main and pass with this change. PTAL. |
placeRegistersArm64 marked the eightbyte as flushed when a field's
alignment pushed the bit cursor past the register boundary, but the
field itself was then accumulated into val and never emitted, so the
last register of a struct was lost (struct { struct { int8 a; int32 b;
}; int8 c } dropped c on linux/arm64). Keep flushed false so the final
flush still emits the trailing value.
Found by the NestedSmallTail regression test added for the amd64 field
offset fix.
|
The linux/arm64 CI failure was a pre-existing bug that the new NestedSmallTail regression test exposed: in placeRegistersArm64, when a field's alignment pushed the bit cursor past a register boundary, the pending eightbyte was flushed but the flush was marked as final, so the field accumulated afterwards (the trailing int8) was never placed into a register and was lost. This commit leaves that intermediate flush non-final so the final flush still emits the trailing value. TestRegisterFunc_structArgs now passes on both linux/amd64 and linux/arm64. |
hajimehoshi
left a comment
hajimehoshi
left a comment
There was a problem hiding this comment.
Found one amd64 argument-packing regression, reproduced with real C calls against both the PR head and its base.
Reviewed by Codex (OpenAI), on behalf of @hajimehoshi.
…rsion
tryPlaceRegister restored curEight to the outer field's eightbyte when a
nested struct or array recursion returned, but the accumulator is shared
across recursion levels. When the nested value spans eightbytes its tail
stays pending in the last one it touched; restoring the outer index
flushed that tail prematurely and the flushed flag then suppressed the
flush of the following sibling field, dropping it from the register
arguments (struct { A struct{X,Y,Z int32}; B int32 } and
struct { A [3]int32; B int32 } passed six instead of ten to a C sum).
Leave curEight pointing at the pending accumulator and add regression
coverage for both layouts.
|
Addressed the review: tryPlaceRegister no longer restores the pending eightbyte index around struct/array recursion. The accumulator is shared across recursion levels, so keeping the outer field's index after recursion made it inconsistent whenever a nested struct spanned eightbytes — the tail was flushed prematurely and the flushed flag then dropped the following sibling. Both flagged layouts (struct { A struct { X, Y, Z int32 }; B int32 } and struct { A [3]int32; B int32 }) now keep curEight associated with the pending accumulator, and I added regression coverage for them using C sum functions over the four fields initialized to 1, 2, 3, 4. The tests return 6 on the previous head and 10 with this fix; verified on linux/amd64 and linux/arm64, including the callback variants. PTAL. |
hajimehoshi
left a comment
hajimehoshi
left a comment
There was a problem hiding this comment.
Confirmed that the previous nested-struct/array regression is fixed and the added regression tests pass. One related padding case remains, described inline.
Reviewed by Codex (OpenAI), on behalf of @hajimehoshi.
| // after flushing we can place it directly without shifting. | ||
| needFresh := shift != 0 && fieldOff/8 != curEight | ||
| if needFresh { | ||
| flushIfNeeded() |
There was a problem hiding this comment.
[P2] Reset flushed after an intermediate boundary flush
For struct { A struct { X int32; Y int8 }; B int8 }, the inner struct has trailing padding, so B starts at offset 8 while the first eightbyte is still pending. This call emits that eightbyte and sets flushed = true. The Int8 case then accumulates B without clearing the flag, so the final flushIfNeeded() skips it.
Confirmed with a real C function summing the fields initialized to 1, 2, and 3: the latest head returns 3 instead of 6 on macOS amd64 under Rosetta (CGO_ENABLED=0). Setting flushed = false immediately after this intermediate flush makes the reproducer and TestRegisterFunc_structArgs pass. Please reset the flag before accumulating the next field and add coverage for this trailing-padding layout.
This layout also fails on the PR base, so this is a remaining gap in the padding fix, not a new regression.
2 participants
What issue is this addressing?
Closes #520
What type of issue is this addressing?
bug
What this PR does | solves
tryPlaceRegister packed fields back-to-back and overwrote pending small fields when a 64-bit field followed (val = instead of |=), dropping the first eightbyte and shifting all later arguments by one slot. Small fields also ignored padding, misplacing e.g. the int32 at offset 4 in {int8; int32}. Track each field's in-memory offset: flush the pending eightbyte on crossing, place wide fields directly, and realign the bit cursor for small fields. The recursive place() also clobbered the outer eightbyte index with the inner tail position, misclassifying later sibling fields of nested structs (e.g. StructInStruct lost B and C); save and restore the cursor around recursion.