purego: extend struct argument support to Linux amd64/arm64#361
purego: extend struct argument support to Linux amd64/arm64#361hajimehoshi merged 15 commits intoebitengine:mainfrom
Conversation
|
Write an issue number this PR works on. |
I believe this closes #236 |
|
Thanks. Note that, as #236 is a new feature rather than a bug fix, I'm not going to backport this fix to the stable branch. |
func.go
Outdated
There was a problem hiding this comment.
I think this comment is redundant. Create a utility function like isStructSupportedForRegisterFunc and use it.
func.go
Outdated
There was a problem hiding this comment.
My suggested function isStructSupportedForRegisterFunc should also be available here
tmc
force-pushed
the
linux-struct-support
branch
from
29b5a8f to
3c62bd1
Compare
|
rebased and I believe have addressed outstanding comments |
testdata/structtest/struct_test.c
Outdated
testdata/structtest/struct_test.c
Outdated
struct_test.go
Outdated
There was a problem hiding this comment.
as in go tool compile -dcheckptr which go test -race sets:
go tool compile -d help
There was a problem hiding this comment.
but I think this can be simplified, 1m.
There was a problem hiding this comment.
hm yeah actually this causes go vet failures like this:
./struct_test.go:510:48: possible misuse of unsafe.Pointer
./struct_test.go:525:37: possible misuse of unsafe.Pointer
./struct_test.go:525:59: possible misuse of unsafe.Pointer
hajimehoshi
left a comment
hajimehoshi
left a comment
There was a problem hiding this comment.
LGTM, thanks!
@TotallyGamerJet Please take a look
struct_test.go
Outdated
There was a problem hiding this comment.
Optional:
const expectedSum = 10 + 20 + 30 + 63
|
Are there any limitations like nested structs, float32 struct members, or the order of the struct members? |
I don't think so -- certainly welcome including more edge cases in tests. |
|
@tmc I created a new testcase which fails: I also noticed that |
|
Thanks, I'll get that incorporated and addressed (at least the bool case). |
|
string handling is a bit more involved and I think that should be done in a follow-on commit but I have bool support in now. |
|
note: I have string support in https://github.com/tmc/purego/tree/add-string-field-support -- will open a PR for it after this goes in |
|
I wonder, is there any likelihood of this being merged soon? It was approved over 3 months ago. I appreciate that there have been lots of other great PRs being reviewed and merged lately, and that this one might not be a priority. But I wonder whether it has been forgotten? |
|
@tmc doesn't this require rebasing? @TotallyGamerJet ping |
Expands struct argument and return value support from Darwin-only to both Darwin and Linux on amd64 and arm64 architectures. This change enables purego to handle struct marshalling on additional platforms while maintaining the existing safety checks for unsupported architectures. Updates platform validation logic to check both GOOS and GOARCH, ensuring struct support is only enabled on tested platform combinations. The build constraints for struct tests are updated accordingly to include Linux. Adds test coverage for pointer wrapper structs to validate proper handling of single and multiple pointer fields within structs, ensuring correct register allocation and marshalling across the expanded platform support.
Addresses platform-specific differences in struct argument tests between Darwin and Linux by using consistent data types and avoiding sign extension issues. Changes the C struct field from int8_t to char for better cross-platform consistency, and updates test values to use all positive numbers to avoid sign extension differences between platforms. Also simplifies the build constraint logic for better readability. These changes ensure struct tests pass reliably on both Darwin and Linux while maintaining the same test coverage.
Refactors RegisterFunc to use a shared isStructSupportedForRegisterFunc helper function instead of duplicating the platform validation logic for both struct arguments and return values. This improves code maintainability while preserving the same validation behavior.
Changes the Array4Chars struct field from char to signed char to ensure consistent signedness across platforms. The signedness of plain char is implementation-defined and can vary between compilers and platforms, potentially causing test inconsistencies. Using signed char makes the signedness explicit and ensures consistent behavior in struct marshalling tests across Darwin and Linux platforms.
Removes descriptive comments from PointerWrapper and TwoPointers struct tests in both Go and C code as the test purpose is clear from context and variable names.
Replaces dynamic memory allocation with constant uintptr values in PointerWrapper and TwoPointers tests. This eliminates the need for runtime.KeepAlive calls and makes the tests more deterministic by using fixed pointer values instead of heap-allocated objects.
Uses stack-allocated variables instead of uintptr constants to fix go vet warnings about misuse of unsafe.Pointer. The new approach is simpler than heap allocation (no runtime.KeepAlive needed) while still being correct.
Extends struct field type support to include boolean fields by adding reflect.Bool to the list of supported types in checkStructFieldsSupported. Adds comprehensive test coverage for boolean fields in struct returns, including Mixed5 (with pointer and boolean), SmallBool (small struct with boolean), and LargeBool (large struct with boolean) test cases. The C implementations validate proper marshalling of boolean values across different struct layouts and sizes.
Make the expectedSum derivation clearer by expressing it as the sum of its component values rather than the precomputed result.
Rebasing, will push shortly. |
tmc
force-pushed
the
linux-struct-support
branch
from
a82375f to
a9cb6fe
Compare
The arm64 implementation was already fixed but loong64 was missed.
|
noticed reflect.UnsafePointer was missed on loong64, fixed. |
|
@tmc It will fail |
…t/float fields From JupiterRider feedback on PR ebitengine#361. Passes on darwin/arm64 but reported to fail on Linux.
Thanks, confirmed and looking into this. |
placeStack was decomposing struct fields into separate stack slots, which broke the x86-64 ABI for structs with mixed-type fields sharing an eightbyte (e.g. int32 + float32 at adjacent offsets). The ABI requires structs passed on the stack to preserve their in-memory layout. Copy raw eightbyte chunks instead. Fixes IdentityMixed5 (ptr, int32, float32, int32) on linux/amd64 where the float32 bits were misplaced into the adjacent int32 field.
5 participants
What issue is this addressing?
Extends struct argument support to Linux amd64 and arm64 platforms.
What type of issue is this addressing?
enhancement
What this PR does | solves
This PR removes the Darwin-only restriction on struct arguments and enables struct-by-value support on Linux for both amd64 and arm64 architectures.
Closes #236, #412