-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Fix ABI for Int128 #73601
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix ABI for Int128 #73601
Conversation
|
|
||
| struct StructWithInt128 | ||
| { | ||
| int64_t messUpPadding; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use int8_t to better validate 32bit platforms that expect 4 byte alignment instead of 8/16 ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it doesn't matter too much. The way alignment works in the various architectures we support/are likely to support, there won't be a meaningful difference. However, this is altogether messy enough that we may simply remove the ability to pass Int128 across the p/invoke boundary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noting that while blocking this is fine for .NET 7 given the short timeframe and that its currently busted. This won't be viable longer term. Int128 is an ABI primitive and much like Vector128<T> needs to eventually be supported.
- In particular: 1. The alignment requirement imposed by of a non-primitive, non-enum valuetype field is the alignment of that field 2. The alignment requirement imposed by a primitive is the pointer size of the target platform, unless running on Arm32, in which case if the primitive or enum is 8 bytes in size, the alignment requirement is 8. - The previous implementation produced an alignment of pointer size, unless running on Arm32 and one of the fields had an alignment requirement of 8 (in which case the alignment requirement computed for the structure would be 8) In addition, add a test which verifies that the instance field layout test types are actually producing R2R compatible results at runtime. - This test shows that we have some issues around explicit layout, so I was forced to disable that portion of the test for now. Fixes dotnet#65281
|
Closing, as this PR isn't really ready for checkin any time soon. |
Fixes #72206
(This is WIP as this first PR run is intended only to find out what problems actually exist on our various systems, following that, I will actually write the fix.)