Fix conflicting generalized types in TypeSSA #8119
Open
+303
−172
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 { |
src/wasm-type-shape.h
Outdated
| // 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.
Member
Author
There was a problem hiding this comment.
This can still happen when custom descriptors are disabled and the input already includes rec groups that differ only in exactness. This is nonsensical, but our validator doesn't reject it, so the fuzzer can still generate it. I'll investigate rejecting these cases in the parsers.
GlobalTypeRewriter methods previously took an optional list of extra types to consider private and therefore eligible to be modified. Remove this parameter because it is not used.
kripken
approved these changes
Dec 16, 2025
Member
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
lgtm % question and the fuzz issue you mentioned
| ;; CHECK-NEXT: (type $A_1 (sub $A (shared (array (mut i32))))) | ||
|
|
||
| ;; CHECK: (type $4 (struct (field (mut i32)) (field (mut i32)) (field (mut f64)) (field (mut f64)) (field (mut i32)) (field (mut f64)) (field (mut f64)) (field (mut i32)) (field (mut i32)) (field (mut i32)) (field (mut i32)))) | ||
| ;; CHECK: (type $4 (struct)) |
Member
Author
There was a problem hiding this comment.
TypeSSA no longer encodes a hash as a brand type, but rather uses the brand iterator, which produces much smaller types.
Move BrandTypeIterator from MinimizeRecGroups to wasm-type-shape.h and use it in a new UniqueRecGroups utility that can rebuild types to be distinct from previously seen rec groups. Use UniqueRecGroups in GlobalTypeRewriter to ensure the newly built private types do not conflict with public types. Split off from #8119 because this can land sooner.
tlively
added a commit
that referenced
this pull request
Dec 16, 2025
Move BrandTypeIterator from MinimizeRecGroups to wasm-type-shape.h and use it in a new UniqueRecGroups utility that can rebuild types to be distinct from previously seen rec groups. Use UniqueRecGroups in GlobalTypeRewriter to ensure the newly built private types do not conflict with public types. Split off from #8119 because this can land sooner.
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.