From b42f332e8f2842076fcf5b5bcbfcdc0b50abec2b Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Tue, 13 Jan 2026 20:34:54 -0800 Subject: [PATCH 1/2] Fix bug in UniqueRecGroups UniqueRecGroups compares the shapes of rec groups after considering how types will be written in the final binary for a particular feature set. Previously its interface dealt in arbitrary vectors of types, but it turns out that it could sometimes be subtly incorrect to pass vectors of types that were not already all in the same rec group. The problem is that the base case for the RecGroupShape utility that hashes rec group shapes is either hashing the index in the group for recursive references to types in the group or the type ID for non-recursive references. When the type vector contains types from different rec groups, non-recursive references across groups can inadvertently appear and be hashed as recursive references. Fix the problem for UniqueRecGroups by changing the interface to take RecGroups rather than arbitrary vectors of heap types. A regression test for this bug will be included in an upcoming PR. It cannot be included here because it depends on running a pass that does not exist yet. --- src/ir/type-updating.cpp | 10 +++--- src/passes/TypeSSA.cpp | 16 ++++----- src/wasm-type-shape.h | 11 +++--- src/wasm/wasm-type-shape.cpp | 68 ++++++++++++++++++------------------ src/wasm/wasm-type.cpp | 6 ++-- 5 files changed, 54 insertions(+), 57 deletions(-) diff --git a/src/ir/type-updating.cpp b/src/ir/type-updating.cpp index d402cd221cc..70b990464ed 100644 --- a/src/ir/type-updating.cpp +++ b/src/ir/type-updating.cpp @@ -41,9 +41,9 @@ GlobalTypeRewriter::GlobalTypeRewriter(Module& wasm) for (auto& [type, info] : typeInfo) { if (info.visibility == ModuleUtils::Visibility::Public) { auto group = type.getRecGroup(); - if (seenGroups.insert(type.getRecGroup()).second) { - std::vector groupTypes(group.begin(), group.end()); - publicGroups.insert(std::move(groupTypes)); + if (seenGroups.insert(group).second) { + [[maybe_unused]] RecGroup unique = publicGroups.insert(group); + assert(unique == group); } } } @@ -199,12 +199,12 @@ GlobalTypeRewriter::rebuildTypes(std::vector types) { } #endif // Ensure the new types are different from any public rec group. - const auto& newTypes = publicGroups.insert(*buildResults); + RecGroup newGroup = publicGroups.insert((*buildResults)[0].getRecGroup()); // Map the old types to the new ones. TypeMap oldToNewTypes; for (auto [type, index] : typeIndices) { - oldToNewTypes[type] = newTypes[index]; + oldToNewTypes[type] = newGroup[index]; } mapTypeNamesAndIndices(oldToNewTypes); return oldToNewTypes; diff --git a/src/passes/TypeSSA.cpp b/src/passes/TypeSSA.cpp index 095d6cec2bf..b387cc40b1c 100644 --- a/src/passes/TypeSSA.cpp +++ b/src/passes/TypeSSA.cpp @@ -63,8 +63,7 @@ namespace { // Ensure there are no conflicts between the newly built types and any existing // types. -std::vector ensureTypesAreInNewRecGroup(std::vector&& types, - Module& wasm) { +std::vector ensureRecGroupIsUnique(RecGroup group, Module& wasm) { std::unordered_set existing; for (auto type : ModuleUtils::collectHeapTypes(wasm)) { existing.insert(type.getRecGroup()); @@ -72,20 +71,19 @@ std::vector ensureTypesAreInNewRecGroup(std::vector&& types, UniqueRecGroups unique(wasm.features); for (auto group : existing) { - std::vector types(group.begin(), group.end()); // N.B. we use `insertOrGet` rather than `insert` because some passes (DAE, // BlockMerging) can create multiple types with the same shape, so we can't // assume all the rec groups are already unique. - unique.insertOrGet(std::move(types)); + unique.insertOrGet(group); } - auto num = types.size(); - std::vector uniqueTypes = unique.insert(std::move(types)); - if (uniqueTypes.size() != num) { + RecGroup uniqueGroup = unique.insert(group); + std::vector uniqueTypes(uniqueGroup.begin(), uniqueGroup.end()); + if (uniqueTypes.size() != group.size()) { // Remove the brand type, which we do not need to consider further. uniqueTypes.pop_back(); } - assert(uniqueTypes.size() == num); + assert(uniqueTypes.size() == group.size()); return uniqueTypes; } @@ -353,7 +351,7 @@ struct TypeSSA : public Pass { assert(newTypes.size() == num); // Make sure this is actually a new rec group. - newTypes = ensureTypesAreInNewRecGroup(std::move(newTypes), *module); + newTypes = ensureRecGroupIsUnique(newTypes[0].getRecGroup(), *module); // Success: we can apply the new types. diff --git a/src/wasm-type-shape.h b/src/wasm-type-shape.h index b3dda426803..eb6cd48b97f 100644 --- a/src/wasm-type-shape.h +++ b/src/wasm-type-shape.h @@ -164,20 +164,19 @@ struct BrandTypeIterator { struct UniqueRecGroups { using Groups = std::list>; Groups groups; - std::unordered_map shapes; + std::unordered_map shapes; FeatureSet features; UniqueRecGroups(FeatureSet features) : features(features) {} - // Insert a rec group. If it is already unique, return the original types. - // Otherwise rebuild the group to make it unique and return the rebuilt types, - // including the brand. - const std::vector& insert(std::vector group); + // Insert a rec group. If it is already unique, return the original group. + // Otherwise rebuild the group to make it unique and return the rebuilt group. + RecGroup insert(RecGroup group); // If the group is unique, insert it and return the types. Otherwise, return // the types that already have this shape. - const std::vector& insertOrGet(std::vector group); + RecGroup insertOrGet(RecGroup group); }; } // namespace wasm diff --git a/src/wasm/wasm-type-shape.cpp b/src/wasm/wasm-type-shape.cpp index 814959e7621..dcac43192f4 100644 --- a/src/wasm/wasm-type-shape.cpp +++ b/src/wasm/wasm-type-shape.cpp @@ -373,51 +373,51 @@ bool ComparableRecGroupShape::operator>(const RecGroupShape& other) const { return GT == compareComparable(*this, other); } -const std::vector& -UniqueRecGroups::insert(std::vector types) { - auto groupIt = groups.emplace(groups.end(), std::move(types)); - auto& group = *groupIt; - if (shapes.emplace(RecGroupShape(group, features), groupIt).second) { +RecGroup UniqueRecGroups::insert(RecGroup group) { + auto typesIt = groups.emplace(groups.end(), group.begin(), group.end()); + auto& types = *typesIt; + if (shapes.emplace(RecGroupShape(types, features), group).second) { // The types are already unique. return group; } // There is a conflict. Find a brand that makes the group unique. BrandTypeIterator brand; - group.push_back(*brand); - while (!shapes.emplace(RecGroupShape(group, features), groupIt).second) { - group.back() = *++brand; - } - // Rebuild the rec group to include the brand. Map the old types (excluding - // the brand) to their corresponding new types to preserve recursions within - // the group. - Index size = group.size(); - TypeBuilder builder(size); - std::unordered_map newTypes; - for (Index i = 0; i < size - 1; ++i) { - newTypes[group[i]] = builder[i]; - } - for (Index i = 0; i < size; ++i) { - builder[i].copy(group[i], [&](HeapType type) { - if (auto newType = newTypes.find(type); newType != newTypes.end()) { - return newType->second; - } - return type; - }); - } - builder.createRecGroup(0, size); - group = *builder.build(); + types.push_back(*brand); + Index size = types.size(); + do { + types.back() = *brand; + ++brand; + TypeBuilder builder(size); + // Map the old types (excluding the brand) to their corresponding new types + // to preserve recursions within the group. + std::unordered_map newTypes; + for (Index i = 0; i < size - 1; ++i) { + newTypes[group[i]] = builder[i]; + } + for (Index i = 0; i < size; ++i) { + builder[i].copy(types[i], [&](HeapType type) { + if (auto newType = newTypes.find(type); newType != newTypes.end()) { + return newType->second; + } + return type; + }); + } + builder.createRecGroup(0, size); + types = *builder.build(); + group = types[0].getRecGroup(); + } while (!shapes.emplace(RecGroupShape(types, features), group).second); + return group; } -const std::vector& -UniqueRecGroups::insertOrGet(std::vector types) { - auto groupIt = groups.emplace(groups.end(), std::move(types)); +RecGroup UniqueRecGroups::insertOrGet(RecGroup group) { + auto typesIt = groups.emplace(groups.end(), group.begin(), group.end()); auto [it, inserted] = - shapes.emplace(RecGroupShape(*groupIt, features), groupIt); + shapes.emplace(RecGroupShape(*typesIt, features), group); if (!inserted) { - groups.erase(groupIt); + groups.erase(typesIt); } - return *it->second; + return it->second; } } // namespace wasm diff --git a/src/wasm/wasm-type.cpp b/src/wasm/wasm-type.cpp index 80fd246c8ba..16bc1d0c380 100644 --- a/src/wasm/wasm-type.cpp +++ b/src/wasm/wasm-type.cpp @@ -2705,9 +2705,9 @@ TypeBuilder::BuildResult TypeBuilder::build() { // If we are building multiple groups, make sure there will be no conflicts // after disallowed features are taken into account. if (groupSize > 0 && groupSize != entryCount) { - auto expectedFirst = (*built)[0]; - auto& types = impl->unique.insertOrGet(*built); - if (types[0] != expectedFirst) { + auto group = (*built)[0].getRecGroup(); + auto uniqueGroup = impl->unique.insertOrGet(group); + if (group != uniqueGroup) { return {TypeBuilder::Error{ groupStart, TypeBuilder::ErrorReason::RecGroupCollision}}; } From 6deb4e6164cd46b64fe5887a049c8a85ebabac98 Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Wed, 14 Jan 2026 14:28:51 -0800 Subject: [PATCH 2/2] add suggested comment --- src/passes/TypeSSA.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/passes/TypeSSA.cpp b/src/passes/TypeSSA.cpp index b387cc40b1c..26252678fb2 100644 --- a/src/passes/TypeSSA.cpp +++ b/src/passes/TypeSSA.cpp @@ -61,8 +61,8 @@ namespace wasm { namespace { -// Ensure there are no conflicts between the newly built types and any existing -// types. +// Ensure there are no conflicts between the newly built types in the given +// RecGroup and any existing types. std::vector ensureRecGroupIsUnique(RecGroup group, Module& wasm) { std::unordered_set existing; for (auto type : ModuleUtils::collectHeapTypes(wasm)) {