wasm: correctly implement union trivial aggregates - #162806
Merged
Merged
Conversation
A union with two scalar fields, even if they overlap exactly, is not a trivial aggregate
folkertdev
added
O-wasm
rustbot
added
S-waiting-on-review
Member
|
@bors r+ Thanks! |
Contributor
rust-bors
Bot
added
S-waiting-on-bors
Contributor
|
Sounds like this fixes #121408, thanks :) As I found out in that issue, there's also an ABI mismatch for repr(Int) enums with ZST fields, which I don't see represented in the tests added here. I wouldn't be surprised if this change also fixes that case, but might be worth adding a test for as follow-up? |
rust-bors Bot
pushed a commit
that referenced
this pull request
Sep 15, 2026
Rollup merge of #162806 - folkertdev:wasm-union-abi, r=alexcrichton wasm: correctly implement `union` trivial aggregates A union with two scalar fields, even if they overlap exactly, is not a trivial aggregate, based on the implementation in `clang`: https://github.com/llvm/llvm-project/blob/e78970f80f1041de8948cb0b665a9aaae682f6c6/clang/lib/CodeGen/ABIInfoImpl.cpp#L338-L402 It never explicitly considers unions, but essentially views them as records. The `clang` implementation is a plausible interpretation of the specification (though I'd like it to be a bit clearer): https://github.com/WebAssembly/tool-conventions/blob/83e5d715d0c18aee51e2d5ae9434f22d67b6e905/BasicCABI.md#function-arguments-and-return-values > [2] Any struct or union that recursively (including through nested structs, unions, and arrays) contains just a single scalar value and is not specified to have greater than natural alignment. We previously got this wrong. I also don't think this was really tested very well, so I added a bunch of test for various cases. This change moves away from using the `homogeneous_aggregate` method, which is convenient because it is a bit broken and now we don't have to consider wasm when we fix that. I've validated this against `clang` with abi-cafe. r? alexcrichton
Zalathar
added a commit
to Zalathar/rust
that referenced
this pull request
Sep 16, 2026
…excrichton wasm: fix ABI for enums with integer layout and ZST fields fixes rust-lang#121408 follow-up to rust-lang#162806 Clang is doing something that is rather annoying for us to emulate: like with unions it looks at the structure of the type, not the representation. But in rust an `enum` that is represented as an integer is not considered an `aggregate` at all, yet may still need to be passed as one if it has any (even ZST) fields. r? alexcrichton cc @hanna-kruppe
Zalathar
added a commit
to Zalathar/rust
that referenced
this pull request
Sep 16, 2026
…excrichton wasm: fix ABI for enums with integer layout and ZST fields fixes rust-lang#121408 follow-up to rust-lang#162806 Clang is doing something that is rather annoying for us to emulate: like with unions it looks at the structure of the type, not the representation. But in rust an `enum` that is represented as an integer is not considered an `aggregate` at all, yet may still need to be passed as one if it has any (even ZST) fields. r? alexcrichton cc @hanna-kruppe
rust-bors Bot
pushed a commit
that referenced
this pull request
Sep 16, 2026
Rollup merge of #162826 - folkertdev:wasm-int-enum-abi, r=alexcrichton wasm: fix ABI for enums with integer layout and ZST fields fixes #121408 follow-up to #162806 Clang is doing something that is rather annoying for us to emulate: like with unions it looks at the structure of the type, not the representation. But in rust an `enum` that is represented as an integer is not considered an `aggregate` at all, yet may still need to be passed as one if it has any (even ZST) fields. r? alexcrichton cc @hanna-kruppe
pull Bot
pushed a commit
to LeeeeeeM/miri
that referenced
this pull request
Sep 17, 2026
wasm: fix ABI for enums with integer layout and ZST fields fixes rust-lang/rust#121408 follow-up to rust-lang/rust#162806 Clang is doing something that is rather annoying for us to emulate: like with unions it looks at the structure of the type, not the representation. But in rust an `enum` that is represented as an integer is not considered an `aggregate` at all, yet may still need to be passed as one if it has any (even ZST) fields. r? alexcrichton cc @hanna-kruppe
This was referenced Sep 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A union with two scalar fields, even if they overlap exactly, is not a trivial aggregate, based on the implementation in
clang:https://github.com/llvm/llvm-project/blob/e78970f80f1041de8948cb0b665a9aaae682f6c6/clang/lib/CodeGen/ABIInfoImpl.cpp#L338-L402
It never explicitly considers unions, but essentially views them as records. The
clangimplementation is a plausible interpretation of the specification (though I'd like it to be a bit clearer):https://github.com/WebAssembly/tool-conventions/blob/83e5d715d0c18aee51e2d5ae9434f22d67b6e905/BasicCABI.md#function-arguments-and-return-values
We previously got this wrong. I also don't think this was really tested very well, so I added a bunch of test for various cases.
This change moves away from using the
homogeneous_aggregatemethod, which is convenient because it is a bit broken and now we don't have to consider wasm when we fix that.I've validated this against
clangwith abi-cafe.r? alexcrichton