purego: extend struct argument support to Linux amd64/arm64 #361
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
| panic("purego: struct arguments are only supported on darwin amd64 & arm64") | ||
| // Struct arguments are supported on: | ||
| // - darwin amd64 & arm64 | ||
| // - linux amd64 & arm64 |
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
| panic("purego: struct return values only supported on darwin arm64 & amd64") | ||
| // Struct return values are supported on: | ||
| // - darwin amd64 & arm64 | ||
| // - linux amd64 & arm64 |
There was a problem hiding this comment.
My suggested function isStructSupportedForRegisterFunc should also be available here
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.
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
| return (uintptr_t)wrapper.ctx; | ||
| } | ||
|
|
||
| // Two pointer struct - tests register allocation for multiple pointer fields |
testdata/structtest/struct_test.c
Outdated
| return s.f0 + s.f1 + s.f2 + s.f3; | ||
| } | ||
|
|
||
| // Simple pointer wrapper struct - tests single pointer field |
struct_test.go
Outdated
| var AddPointers func(wrapper TwoPointers) uintptr | ||
| purego.RegisterLibFunc(&AddPointers, lib, "AddPointers") | ||
|
|
||
| // Use actual allocated memory to satisfy checkptr |
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
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.
hajimehoshi
left a comment
hajimehoshi
left a comment
There was a problem hiding this comment.
LGTM, thanks!
@TotallyGamerJet Please take a look
| purego.RegisterLibFunc(&Array4CharsFn, lib, "Array4Chars") | ||
| const expectedSum = 1 + 2 + 4 + 8 | ||
| if ret := Array4CharsFn(Array4Chars{a: [...]int8{1, 2, 4, 8}}); ret != expectedSum { | ||
| const expectedSum = 123 |
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). |
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.
|
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 |
4 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