Conversation
|
If this is a bug fix, file an issue first |
|
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. |
|
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.
|
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.
|
Addressed the latest review: the boundary-crossing flush in tryPlaceRegister left the accumulator marked as flushed, so a small field accumulated afterwards (the trailing int8 in struct { A struct { X int32; Y int8 }; B int8 }) was skipped by the final flushIfNeeded and never emitted. The flag is now reset after that intermediate flush. The new SumNestedPadTail regression test also exposed the same layout failing on linux/arm64: placeRegistersArm64 ignored a nested composite's trailing padding and packed the following sibling back-to-back after the last inner field instead of at its in-memory offset. It now tracks the memory offset backing the pending register and realigns the cursor to the composite's end after recursion. Verified on linux/amd64 and linux/arm64 (qemu), direct and callback paths; the test returns 3 before the fix and 6 after. PTAL. |
|
If we do this we need to update the documentation to say it is handled
|
|
Addressed the latest feedback. The comments are shortened: the eightbyte-crossing note, the boundary-flush note and the recursion note in I also followed up on @TotallyGamerJet's point about the documentation. The The branch remains mergeable with main. Locally: PTAL. |
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.
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.
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.
…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.
On amd64 the eightbyte-crossing flush in tryPlaceRegister left the
accumulator marked as flushed, so a small field accumulated afterwards
(the trailing int8 in struct { A struct{X int32; Y int8}; B int8 }) was
skipped by the final flushIfNeeded and lost from the register
arguments; a C function summing 1, 2, 3 returned 3. Reset the flag
after that intermediate flush so the fresh accumulator still reaches
the flush at the end of the iteration or of place().
The same layout failed on linux/arm64 for a related reason:
placeRegistersArm64 positions fields with per-field alignment
arithmetic and the recursion ignored a nested composite's trailing
padding, so the sibling was packed back-to-back after the last inner
field instead of starting at its own in-memory offset. Track the
memory offset that bit 0 of the pending register corresponds to and
realign the cursor to the composite's end after recursion, emitting
the pending register whenever the padding carries past the slot.
Add SumNestedPadTail regression coverage; verified on linux/amd64 and
linux/arm64 (qemu) for both the direct and callback paths.
RegisterFunc still said that purego could not align struct fields and that callers had to add the padding themselves, which the offset-aware packing made untrue. Document that fields are placed at their in-memory offsets and that only the Go struct declaration has to mirror the C one, and cover a struct that relies on the padding Go inserts. Shorten the comments added with the packing fixes while here.
b9c9e88 to
e276b85
Compare
3 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.