Fix conflicting generalized types in TypeSSA #8119
Open
+213
−182
Conversation
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
Make enough constructors of Type and Field constexpr that we can make the fieldOptions array in BrandTypeIterator constexpr as well. This lets us remove logic for initializing this array at runtime.
TypeSSA already had logic to detect and resolve inadvertent conflicts between its newly constructed types and existing types. However, this logic did not take into account the changes that the binary writer can make when writing types, so it was still possible to construct a situation where TypeSSA would produce types that would start conflicting after binary writing. Fix the problem by adding a new UniqueRecGroups utility to wasm-type-shape.h. This utility uses the existing RecGroupShape utility, which is aware of how the binary writer will modify types, to detect conflicts. It uses the BrandTypeIterator, moved to wasm-type-shape.h from MinimizeRecGroups.cpp, to create new types to differentiate rec groups.
kripken
reviewed
Dec 15, 2025
| } | ||
| }; | ||
|
|
||
| struct UniqueRecGroups { |
| // the group will be rebuilt with a brand at the end to make it unique. | ||
| // Returns the rebuilt types (including the brand) or the original types if no | ||
| // brand was necessary. | ||
| const std::vector<HeapType>& get(std::vector<HeapType> group); |
Member
There was a problem hiding this comment.
This is called "get", but it adds a rec group - perhaps "add"?
I was confused by this API in the code, too, calls to get() seemed to have an effect that I couldn't figure out without reading this header.
Member
Author
There was a problem hiding this comment.
Yep, you're right. I'll go with insert.
tlively
commented
Dec 15, 2025
| for (auto group : existing) { | ||
| std::vector<HeapType> types(group.begin(), group.end()); | ||
| [[maybe_unused]] auto uniqueTypes = unique.get(std::move(types)); | ||
| assert(uniqueTypes.size() == group.size() && "unexpected collision"); |
Member
Author
There was a problem hiding this comment.
The fuzzer found a way to make this fail after about 80k iterations, so I'll investigate and fix that as well.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
TypeSSA already had logic to detect and resolve inadvertent conflicts
between its newly constructed types and existing types. However, this
logic did not take into account the changes that the binary writer can
make when writing types, so it was still possible to construct a
situation where TypeSSA would produce types that would start conflicting
after binary writing.
Fix the problem by adding a new UniqueRecGroups utility to
wasm-type-shape.h. This utility uses the existing RecGroupShape utility,
which is aware of how the binary writer will modify types, to detect
conflicts. It uses the BrandTypeIterator, moved to wasm-type-shape.h
from MinimizeRecGroups.cpp, to create new types to differentiate rec
groups.