-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[mono][llvm] Fix alignment of local vars #106313
Conversation
klass->min_align specifies the alignment required by the class fields. For example a class/struct containing an int64 will require a minimum alignment of 8 byte for its storage, while a a class/struct containing an int8 will require a 1 byte alignment. `build_named_alloca` is used to allocate storage for a local var of a certain type so using `klass->min_align` is incorrect. We should use `mono_type_size` instead, as this is used throughout the runtime for this purpose. A var of object type should have the alignment `sizeof (gpointer)` since it is a pointer, completely unrelated to the class field layout.
/azp run runtime-llvm |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-llvm |
Azure Pipelines successfully started running 1 pipeline(s). |
This will probably fix #106200 |
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.
nice @BrzVlad!
Please consider back-port to Net8. I guess #106200 is real also there. |
@pavelsavara Do we have a strong suspicion that we have crashes there that are fixed by this ? While this seems relatively low risk to backport I'm not 100% convinced. |
I'm pretty confident that pointers which are misaligned on wasm shadow stack are invisible (unpinned) to GC. I'm not confident that there are AOT applications in the wild, which hit this combo of GC move & bad alignment. We have many AOT crashes on Net9 CI pipeline. |
All of the CI issues are gone now AFAIK, meaning this has large positive impact. @BrzVlad is this low risk enough to be backported to Net8 LTS ? |
@pavelsavara I think this would be low risk enough to backport, however I feel like we should have some solid justification. Are there any GC crashes still happening regularly on specific test suites on .NET8 CI that we've seen fixed on .NET9 following this PR ? If we don't have such specific crashes I would rather not backport. |
klass->min_align specifies the alignment required by the class fields. For example a class/struct containing an int64 will require a minimum alignment of 8 byte for its storage, while a a class/struct containing an int8 will require a 1 byte alignment.
build_named_alloca
is used to allocate storage for a local var of a certain type so usingklass->min_align
is incorrect. We should usemono_type_size
instead, as this is used throughout the runtime for this purpose. A var of object type should have the alignmentsizeof (gpointer)
since it is a pointer, completely unrelated to the class field layout.Should fix #105251