From cb25670922785c62edfc060ebd9b9d926758f086 Mon Sep 17 00:00:00 2001 From: stevenfontanella Date: Fri, 12 Jun 2026 20:12:03 +0000 Subject: [PATCH 1/4] Fix type updating for indirect call effects --- src/ir/linear-execution.h | 4 +- src/ir/module-utils.h | 6 ++ src/ir/type-updating.cpp | 86 ++++++++++++----- src/passes/GlobalEffects.cpp | 16 ++-- src/passes/pass.cpp | 5 + src/wasm.h | 7 ++ src/wasm/wasm-type.cpp | 15 +-- test/gtest/CMakeLists.txt | 1 + test/gtest/type-updating.cpp | 180 +++++++++++++++++++++++++++++++++++ 9 files changed, 275 insertions(+), 45 deletions(-) create mode 100644 test/gtest/type-updating.cpp diff --git a/src/ir/linear-execution.h b/src/ir/linear-execution.h index 9e69405ff7c..9dc7fbf47f2 100644 --- a/src/ir/linear-execution.h +++ b/src/ir/linear-execution.h @@ -187,7 +187,7 @@ struct LinearExecutionWalker : public PostWalker { auto* effects = find_or_null(self->getModule()->indirectCallEffects, callRef->target->type.getHeapType()); - if (!effects) { + if (!effects || !*effects) { return false; } return !(*effects)->throws_; @@ -203,7 +203,7 @@ struct LinearExecutionWalker : public PostWalker { if (self->getModule()) { if (auto* effects = find_or_null( self->getModule()->indirectCallEffects, callIndirect->heapType); - effects) { + effects && *effects) { refutesThrowEffect = !(*effects)->throws_; } } diff --git a/src/ir/module-utils.h b/src/ir/module-utils.h index f0e1850155e..3f165d2325d 100644 --- a/src/ir/module-utils.h +++ b/src/ir/module-utils.h @@ -76,6 +76,12 @@ void renameFunction(Module& wasm, Name oldName, Name newName); // Convenient iteration over imported/non-imported module elements +inline void iterTypes(const Module& wasm, auto&& visitor) { + for (const auto& [type, _] : wasm.typeIndices) { + visitor(type); + } +} + inline void iterImportedMemories(const Module& wasm, auto&& visitor) { for (auto& import : wasm.memories) { if (import->imported()) { diff --git a/src/ir/type-updating.cpp b/src/ir/type-updating.cpp index ee7cf7d3e33..4bdf389bc00 100644 --- a/src/ir/type-updating.cpp +++ b/src/ir/type-updating.cpp @@ -26,6 +26,66 @@ namespace wasm { +namespace { + +std::unordered_map> +updateIndirectCallEffects(const Module& wasm, + const GlobalTypeRewriter::TypeMap& oldToNewTypes) { + std::unordered_map> + newTypeEffects; + + ModuleUtils::iterTypes(wasm, [&](HeapType oldType) { + HeapType newType; + { + auto it = oldToNewTypes.find(oldType); + if (it == oldToNewTypes.end()) { + // This type wasn't renamed. Copy over the existing entry if present. + if (const auto* oldEffects = + find_or_null(wasm.indirectCallEffects, oldType)) { + newTypeEffects[oldType] = *oldEffects; + } + return; + } + newType = it->second; + } + + const std::shared_ptr* oldEffects = + find_or_null(wasm.indirectCallEffects, oldType); + + if (!oldEffects) { + // We never knew the effects of this (either GlobalEffects never ran or a + // new type was created without copying over effects). Nothing to update. + return; + } + + auto [it, inserted] = newTypeEffects.emplace(newType, nullptr); + auto& newEffects = it->second; + + if (!inserted && !newEffects) { + // Effects of the new type were already unknown. Nothing to do. + return; + } + + if (!*oldEffects) { + // oldType is explicitly unknown. Set the new effects to unknown. + newEffects = nullptr; + return; + } + + if (inserted) { + newEffects = *oldEffects; + } else { + auto merged = std::make_shared(*newEffects); + merged->mergeIn(**oldEffects); + newEffects = std::move(merged); + } + }); + + return newTypeEffects; +} + +} // anonymous namespace + GlobalTypeRewriter::GlobalTypeRewriter(Module& wasm, WorldMode worldMode) : wasm(wasm), publicGroups(wasm.features) { // Find the heap types that are not publicly observable. Even in a closed @@ -329,31 +389,9 @@ void GlobalTypeRewriter::mapTypes(const TypeMap& oldToNewTypes) { // Update indirect call effects per type. // When A is rewritten to B, B inherits the effects of A and A loses its // effects. - std::unordered_map> - newTypeEffects; - - for (const auto& [oldType, newType] : oldToNewTypes) { - std::shared_ptr* oldEffects = - find_or_null(wasm.indirectCallEffects, oldType); - std::shared_ptr* targetEffects = - find_or_null(wasm.indirectCallEffects, newType); - - if (!targetEffects) { - // Nothing to update, we already know nothing and assume all effects. - continue; - } - - if (!oldEffects) { - targetEffects->reset(); - continue; - } - - auto merged = std::make_shared(**targetEffects); - merged->mergeIn(**oldEffects); - *targetEffects = std::move(merged); + if (!wasm.indirectCallEffects.empty()) { + wasm.indirectCallEffects = updateIndirectCallEffects(wasm, oldToNewTypes); } - - wasm.indirectCallEffects = std::move(newTypeEffects); } void GlobalTypeRewriter::mapTypeNamesAndIndices(const TypeMap& oldToNewTypes) { diff --git a/src/passes/GlobalEffects.cpp b/src/passes/GlobalEffects.cpp index efcc45eadd7..749b55722a5 100644 --- a/src/passes/GlobalEffects.cpp +++ b/src/passes/GlobalEffects.cpp @@ -321,9 +321,9 @@ void mergeMaybeEffects(std::shared_ptr& dest, dest->mergeIn(*src); } -// Propagate effects from callees to callers transitively -// e.g. if A -> B -> C (A calls B which calls C) -// Then B inherits effects from C and A inherits effects from both B and C. +// Propagate effects from callees to callers transitively and populate direct +// and indirect call effects. e.g. if A -> B -> C (A calls B which calls C) Then +// B inherits effects from C and A inherits effects from both B and C. // // Generate SCC for the call graph, then traverse it in reverse topological // order processing each callee before its callers. When traversing: @@ -335,7 +335,7 @@ void propagateEffects( const PassOptions& passOptions, std::map& funcInfos, std::unordered_map>& - typeEffects, + indirectCallEffects, const CallGraph& callGraph) { // We only care about Functions that are roots, not types. // A type would be a root if a function exists with that type, but no-one @@ -435,9 +435,9 @@ void propagateEffects( // Assign each function's effects to its CC effects. for (auto node : cc) { std::visit(overloaded{[&](HeapType type) { - if (ccEffects != UnknownEffects) { - typeEffects[type] = ccEffects; - } + // Assign the key even if ccEffects is nullptr. + // See the comment in Module::indirectCallEffects. + indirectCallEffects[type] = ccEffects; }, [&](Function* f) { f->effects = ccEffects; }}, node); @@ -455,6 +455,7 @@ struct GenerateGlobalEffects : public Pass { auto callGraph = buildCallGraph( *module, funcInfos, referencedFuncs, getPassOptions().worldMode); + module->indirectCallEffects.clear(); propagateEffects(*module, getPassOptions(), funcInfos, @@ -468,6 +469,7 @@ struct DiscardGlobalEffects : public Pass { for (auto& func : module->functions) { func->effects.reset(); } + module->indirectCallEffects.clear(); } }; diff --git a/src/passes/pass.cpp b/src/passes/pass.cpp index 672936f3444..31c5eb3052a 100644 --- a/src/passes/pass.cpp +++ b/src/passes/pass.cpp @@ -1079,6 +1079,11 @@ void PassRunner::handleAfterEffects(Pass* pass, Function* func) { // Binaryen IR is modified, so we may have work here. if (!func) { + if (pass->addsEffects()) { + // Indirect call effects are now under-approximating. Clear them to avoid + // incorrect optimizations. + wasm->indirectCallEffects.clear(); + } // If no function is provided, then this is not a function-parallel pass, // and it may have operated on any of the functions in theory, so run on // them all. diff --git a/src/wasm.h b/src/wasm.h index 046cd271d0c..8011b1bc525 100644 --- a/src/wasm.h +++ b/src/wasm.h @@ -2746,6 +2746,13 @@ class Module { // This data is only meaningful for indirect calls. If no indirect call // exists to a function, the data can be out of date (no effort is made to // clean up the data if e.g. all indirect calls to a function are removed). + // + // Null values are possible in the case that a types's effects are unknown. + // Note that this is different from a missing key in the map, which is + // important when rewriting types. Suppose we rewrite A -> B where B is a + // brand new type. Then B should inherit A's effects. OTOH if B is an existing + // type with *explicitly* unknown effects (present with nullptr), then B + // remains explicitly unknown. // TODO: Account for exactness here. std::unordered_map> indirectCallEffects; diff --git a/src/wasm/wasm-type.cpp b/src/wasm/wasm-type.cpp index ae4983daaff..4951892abac 100644 --- a/src/wasm/wasm-type.cpp +++ b/src/wasm/wasm-type.cpp @@ -2764,24 +2764,15 @@ std::unordered_set getIgnorablePublicTypes() { namespace HeapTypes { -HeapType getMutI8Array() { - static HeapType i8Array = Array(Field(Field::i8, Mutable)); - return i8Array; -} +HeapType getMutI8Array() { return Array(Field(Field::i8, Mutable)); } -HeapType getMutI16Array() { - static HeapType i16Array = Array(Field(Field::i16, Mutable)); - return i16Array; -} +HeapType getMutI16Array() { return Array(Field(Field::i16, Mutable)); } } // namespace HeapTypes namespace Types { -Type getI64Pair() { - static Type i64Pair({Type::i64, Type::i64}); - return i64Pair; -} +Type getI64Pair() { return Type({Type::i64, Type::i64}); } } // namespace Types diff --git a/test/gtest/CMakeLists.txt b/test/gtest/CMakeLists.txt index 4bd358032d7..94db64887d7 100644 --- a/test/gtest/CMakeLists.txt +++ b/test/gtest/CMakeLists.txt @@ -34,6 +34,7 @@ set(unittest_SOURCES suffix_tree.cpp topological-sort.cpp type-builder.cpp + type-updating.cpp wat-lexer.cpp validator.cpp ) diff --git a/test/gtest/type-updating.cpp b/test/gtest/type-updating.cpp new file mode 100644 index 00000000000..18699ab1cf4 --- /dev/null +++ b/test/gtest/type-updating.cpp @@ -0,0 +1,180 @@ +/* + * Copyright 2021 WebAssembly Community Group participants + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "gtest/gtest.h" + +#include "ir/effects.h" +#include "ir/type-updating.h" +#include "parser/wat-parser.h" +#include "support/result.h" +#include "wasm.h" + +using namespace wasm; +using namespace testing; + +namespace { + +class IndirectCallEffectsTest : public Test { +protected: + Module wasm; + PassOptions options; + + void SetUp() override { + auto moduleText = R"wasm( + (module + (rec + (type $A (func)) + (type $B (func)) + (type $C (func)) + (type $D (func)) + ) + ) + )wasm"; + + if (auto err = wasm::WATParser::parseModule(wasm, moduleText).getErr()) { + Fatal() << err->msg; + } + } + + std::unordered_map> + updateEffects( + const std::unordered_map>& oldEffects, + const std::unordered_map& typeMap) { + + std::unordered_map types; + for (const auto& [type, name] : wasm.typeNames) { + types[name.name] = type; + } + + for (const auto& [name, effects] : oldEffects) { + wasm.indirectCallEffects[types.at(name)] = effects; + } + + GlobalTypeRewriter::TypeMap map; + for (const auto& [oldName, newName] : typeMap) { + map[types.at(oldName)] = types.at(newName); + } + + GlobalTypeRewriter rewriter(wasm, WorldMode::Open); + rewriter.mapTypes(map); + + std::unordered_map> + result; + for (const auto& [type, effects] : wasm.indirectCallEffects) { + auto name = wasm.typeNames.at(type).name.toString(); + result[name] = effects; + } + return result; + } +}; + +} // anonymous namespace + +TEST_F(IndirectCallEffectsTest, SrcHasNoEffects) { + auto effectsB = std::make_shared(options, wasm); + effectsB->writesMemory = true; + + auto merged = + updateEffects(/*oldEffects=*/{{"B", effectsB}}, /*typeMap=*/{{"A", "B"}}); + + EXPECT_FALSE(merged.contains("A")); + ASSERT_TRUE(merged.contains("B")); + + // Pointer comparison + EXPECT_EQ(merged.at("B"), effectsB); +} + +TEST_F(IndirectCallEffectsTest, DestHasNoEffects) { + auto effectsA = std::make_shared(options, wasm); + effectsA->calls = true; + + auto merged = + updateEffects(/*oldEffects=*/{{"A", effectsA}}, /*typeMap=*/{{"A", "B"}}); + + EXPECT_FALSE(merged.contains("A")); + ASSERT_TRUE(merged.contains("B")); + + // Pointer comparison + EXPECT_EQ(merged.at("B"), effectsA); +} + +TEST_F(IndirectCallEffectsTest, BothHaveNoEffects) { + auto merged = updateEffects(/*oldEffects=*/{}, /*typeMap=*/{{"A", "B"}}); + + EXPECT_FALSE(merged.contains("A")); + EXPECT_FALSE(merged.contains("B")); +} + +TEST_F(IndirectCallEffectsTest, BothHaveEffects) { + auto effectsA = std::make_shared(options, wasm); + effectsA->calls = true; + auto effectsB = std::make_shared(options, wasm); + effectsB->writesMemory = true; + + auto merged = updateEffects(/*oldEffects=*/{{"A", effectsA}, {"B", effectsB}}, + /*typeMap=*/{{"A", "B"}}); + + EXPECT_FALSE(merged.contains("A")); + ASSERT_TRUE(merged.contains("B")); + EXPECT_TRUE(merged.at("B")->calls); + EXPECT_TRUE(merged.at("B")->writesMemory); +} + +TEST_F(IndirectCallEffectsTest, SrcHasNullptrEffects) { + auto effectsB = std::make_shared(options, wasm); + effectsB->writesMemory = true; + + auto merged = updateEffects(/*oldEffects=*/{{"A", nullptr}, {"B", effectsB}}, + /*typeMap=*/{{"A", "B"}}); + + EXPECT_FALSE(merged.contains("A")); + ASSERT_TRUE(merged.contains("B")); + EXPECT_EQ(merged.at("B"), nullptr); +} + +TEST_F(IndirectCallEffectsTest, DestHasNullptrEffects) { + auto effectsA = std::make_shared(options, wasm); + effectsA->calls = true; + + auto merged = updateEffects(/*oldEffects=*/{{"A", effectsA}, {"B", nullptr}}, + /*typeMap=*/{{"A", "B"}}); + + EXPECT_FALSE(merged.contains("A")); + ASSERT_TRUE(merged.contains("B")); + EXPECT_EQ(merged.at("B"), nullptr); +} + +TEST_F(IndirectCallEffectsTest, MergeThreeTypes) { + auto effectsA = std::make_shared(options, wasm); + effectsA->calls = true; + auto effectsB = std::make_shared(options, wasm); + effectsB->writesMemory = true; + auto effectsC = std::make_shared(options, wasm); + effectsC->throws_ = true; + + auto merged = updateEffects( + /*oldEffects=*/{{"A", effectsA}, {"B", effectsB}, {"C", effectsC}}, + /*typeMap=*/{{"A", "D"}, {"B", "D"}, {"C", "D"}}); + + EXPECT_FALSE(merged.contains("A")); + EXPECT_FALSE(merged.contains("B")); + EXPECT_FALSE(merged.contains("C")); + ASSERT_TRUE(merged.contains("D")); + EXPECT_TRUE(merged.at("D")->calls); + EXPECT_TRUE(merged.at("D")->writesMemory); + EXPECT_TRUE(merged.at("D")->throws_); +} From 642bd0ff20e96b800c62906d0dd62038c54aaf61 Mon Sep 17 00:00:00 2001 From: stevenfontanella Date: Tue, 30 Jun 2026 23:13:10 +0000 Subject: [PATCH 2/4] PR updates --- src/passes/GlobalEffects.cpp | 4 ++-- src/wasm.h | 13 ++++++++----- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/passes/GlobalEffects.cpp b/src/passes/GlobalEffects.cpp index 749b55722a5..47e16308ab6 100644 --- a/src/passes/GlobalEffects.cpp +++ b/src/passes/GlobalEffects.cpp @@ -322,8 +322,8 @@ void mergeMaybeEffects(std::shared_ptr& dest, } // Propagate effects from callees to callers transitively and populate direct -// and indirect call effects. e.g. if A -> B -> C (A calls B which calls C) Then -// B inherits effects from C and A inherits effects from both B and C. +// and indirect call effects. e.g. if A -> B -> C (A calls B which calls C), +// then B inherits effects from C and A inherits effects from both B and C. // // Generate SCC for the call graph, then traverse it in reverse topological // order processing each callee before its callers. When traversing: diff --git a/src/wasm.h b/src/wasm.h index 8011b1bc525..2ed576fc470 100644 --- a/src/wasm.h +++ b/src/wasm.h @@ -2730,6 +2730,9 @@ class Module { Name name; std::unordered_map typeNames; + + // The source binary's type indicies. Used in some cases for preserving + // ordering of types std::unordered_map typeIndices; // Potential effects for bodies of indirect calls to this type. Populated by @@ -2748,11 +2751,11 @@ class Module { // clean up the data if e.g. all indirect calls to a function are removed). // // Null values are possible in the case that a types's effects are unknown. - // Note that this is different from a missing key in the map, which is - // important when rewriting types. Suppose we rewrite A -> B where B is a - // brand new type. Then B should inherit A's effects. OTOH if B is an existing - // type with *explicitly* unknown effects (present with nullptr), then B - // remains explicitly unknown. + // A null value is functionally the same as the key not being present at all + // in the map, but we distinguish the two for correctness when rewriting + // types. Suppose we rewrite A -> B where B is a brand new type. Then B should + // inherit A's effects. OTOH if B is an existing type with *explicitly* + // unknown effects (present with nullptr), then B remains explicitly unknown. // TODO: Account for exactness here. std::unordered_map> indirectCallEffects; From fe3bda58c7d7197ded272b2d648f4ffacdc13f62 Mon Sep 17 00:00:00 2001 From: stevenfontanella Date: Mon, 6 Jul 2026 23:13:38 +0000 Subject: [PATCH 3/4] Try removing null state --- src/ir/linear-execution.h | 4 +-- src/ir/type-updating.cpp | 68 ++++++++++++++++-------------------- src/passes/GlobalEffects.cpp | 6 ++-- src/wasm.h | 12 +++---- test/gtest/type-updating.cpp | 43 ++++------------------- 5 files changed, 48 insertions(+), 85 deletions(-) diff --git a/src/ir/linear-execution.h b/src/ir/linear-execution.h index 9dc7fbf47f2..9e69405ff7c 100644 --- a/src/ir/linear-execution.h +++ b/src/ir/linear-execution.h @@ -187,7 +187,7 @@ struct LinearExecutionWalker : public PostWalker { auto* effects = find_or_null(self->getModule()->indirectCallEffects, callRef->target->type.getHeapType()); - if (!effects || !*effects) { + if (!effects) { return false; } return !(*effects)->throws_; @@ -203,7 +203,7 @@ struct LinearExecutionWalker : public PostWalker { if (self->getModule()) { if (auto* effects = find_or_null( self->getModule()->indirectCallEffects, callIndirect->heapType); - effects && *effects) { + effects) { refutesThrowEffect = !(*effects)->throws_; } } diff --git a/src/ir/type-updating.cpp b/src/ir/type-updating.cpp index 4bdf389bc00..9e1a1ffb736 100644 --- a/src/ir/type-updating.cpp +++ b/src/ir/type-updating.cpp @@ -30,56 +30,46 @@ namespace { std::unordered_map> updateIndirectCallEffects(const Module& wasm, + const std::vector& oldTypes, const GlobalTypeRewriter::TypeMap& oldToNewTypes) { std::unordered_map> newTypeEffects; - ModuleUtils::iterTypes(wasm, [&](HeapType oldType) { + std::unordered_set unknownNewTypes; + + for (HeapType oldType : oldTypes) { HeapType newType; { auto it = oldToNewTypes.find(oldType); if (it == oldToNewTypes.end()) { - // This type wasn't renamed. Copy over the existing entry if present. - if (const auto* oldEffects = - find_or_null(wasm.indirectCallEffects, oldType)) { - newTypeEffects[oldType] = *oldEffects; - } - return; + newType = oldType; + } else { + newType = it->second; } - newType = it->second; + } + + if (unknownNewTypes.count(newType)) { + continue; } const std::shared_ptr* oldEffects = find_or_null(wasm.indirectCallEffects, oldType); if (!oldEffects) { - // We never knew the effects of this (either GlobalEffects never ran or a - // new type was created without copying over effects). Nothing to update. - return; - } - - auto [it, inserted] = newTypeEffects.emplace(newType, nullptr); - auto& newEffects = it->second; - - if (!inserted && !newEffects) { - // Effects of the new type were already unknown. Nothing to do. - return; - } - - if (!*oldEffects) { - // oldType is explicitly unknown. Set the new effects to unknown. - newEffects = nullptr; - return; + // oldType is an existing type and has no entry, which means its + // effects are explicitly UNKNOWN. + unknownNewTypes.insert(newType); + newTypeEffects.erase(newType); + continue; } - if (inserted) { - newEffects = *oldEffects; - } else { - auto merged = std::make_shared(*newEffects); + auto [it, inserted] = newTypeEffects.emplace(newType, *oldEffects); + if (!inserted) { + auto merged = std::make_shared(*it->second); merged->mergeIn(**oldEffects); - newEffects = std::move(merged); + it->second = std::move(merged); } - }); + } return newTypeEffects; } @@ -272,6 +262,15 @@ GlobalTypeRewriter::rebuildTypes(std::vector types) { } void GlobalTypeRewriter::mapTypes(const TypeMap& oldToNewTypes) { + if (!wasm.indirectCallEffects.empty()) { + // Update indirect call effects per type. + // When A is rewritten to B, B inherits the effects of A and A loses its + // effects. + std::vector oldTypes = ModuleUtils::collectHeapTypes(wasm); + wasm.indirectCallEffects = + updateIndirectCallEffects(wasm, oldTypes, oldToNewTypes); + } + // Replace all the old types in the module with the new ones. struct CodeUpdater : public WalkerPass< @@ -385,13 +384,6 @@ void GlobalTypeRewriter::mapTypes(const TypeMap& oldToNewTypes) { for (auto& tag : wasm.tags) { tag->type = updater.getNew(tag->type); } - - // Update indirect call effects per type. - // When A is rewritten to B, B inherits the effects of A and A loses its - // effects. - if (!wasm.indirectCallEffects.empty()) { - wasm.indirectCallEffects = updateIndirectCallEffects(wasm, oldToNewTypes); - } } void GlobalTypeRewriter::mapTypeNamesAndIndices(const TypeMap& oldToNewTypes) { diff --git a/src/passes/GlobalEffects.cpp b/src/passes/GlobalEffects.cpp index 47e16308ab6..f3313700e8e 100644 --- a/src/passes/GlobalEffects.cpp +++ b/src/passes/GlobalEffects.cpp @@ -435,9 +435,9 @@ void propagateEffects( // Assign each function's effects to its CC effects. for (auto node : cc) { std::visit(overloaded{[&](HeapType type) { - // Assign the key even if ccEffects is nullptr. - // See the comment in Module::indirectCallEffects. - indirectCallEffects[type] = ccEffects; + if (ccEffects) { + indirectCallEffects[type] = ccEffects; + } }, [&](Function* f) { f->effects = ccEffects; }}, node); diff --git a/src/wasm.h b/src/wasm.h index 2ed576fc470..20814e5bb50 100644 --- a/src/wasm.h +++ b/src/wasm.h @@ -2750,12 +2750,12 @@ class Module { // exists to a function, the data can be out of date (no effort is made to // clean up the data if e.g. all indirect calls to a function are removed). // - // Null values are possible in the case that a types's effects are unknown. - // A null value is functionally the same as the key not being present at all - // in the map, but we distinguish the two for correctness when rewriting - // types. Suppose we rewrite A -> B where B is a brand new type. Then B should - // inherit A's effects. OTOH if B is an existing type with *explicitly* - // unknown effects (present with nullptr), then B remains explicitly unknown. + // A missing key in the map means that the type's effects are explicitly + // unknown. When rewriting types, we distinguish new types from existing + // types by checking whether they existed before the update. If a new type + // is created, it inherits the effects of the old types that map to it. If + // an existing type with unknown effects is mapped, the resulting type will + // also have explicitly unknown effects. // TODO: Account for exactness here. std::unordered_map> indirectCallEffects; diff --git a/test/gtest/type-updating.cpp b/test/gtest/type-updating.cpp index 18699ab1cf4..e9a52c5ba6c 100644 --- a/test/gtest/type-updating.cpp +++ b/test/gtest/type-updating.cpp @@ -41,6 +41,10 @@ class IndirectCallEffectsTest : public Test { (type $C (func)) (type $D (func)) ) + (func $f_a (type $A)) + (func $f_b (type $B)) + (func $f_c (type $C)) + (func $f_d (type $D)) ) )wasm"; @@ -92,10 +96,7 @@ TEST_F(IndirectCallEffectsTest, SrcHasNoEffects) { updateEffects(/*oldEffects=*/{{"B", effectsB}}, /*typeMap=*/{{"A", "B"}}); EXPECT_FALSE(merged.contains("A")); - ASSERT_TRUE(merged.contains("B")); - - // Pointer comparison - EXPECT_EQ(merged.at("B"), effectsB); + EXPECT_FALSE(merged.contains("B")); } TEST_F(IndirectCallEffectsTest, DestHasNoEffects) { @@ -106,10 +107,7 @@ TEST_F(IndirectCallEffectsTest, DestHasNoEffects) { updateEffects(/*oldEffects=*/{{"A", effectsA}}, /*typeMap=*/{{"A", "B"}}); EXPECT_FALSE(merged.contains("A")); - ASSERT_TRUE(merged.contains("B")); - - // Pointer comparison - EXPECT_EQ(merged.at("B"), effectsA); + EXPECT_FALSE(merged.contains("B")); } TEST_F(IndirectCallEffectsTest, BothHaveNoEffects) { @@ -134,30 +132,6 @@ TEST_F(IndirectCallEffectsTest, BothHaveEffects) { EXPECT_TRUE(merged.at("B")->writesMemory); } -TEST_F(IndirectCallEffectsTest, SrcHasNullptrEffects) { - auto effectsB = std::make_shared(options, wasm); - effectsB->writesMemory = true; - - auto merged = updateEffects(/*oldEffects=*/{{"A", nullptr}, {"B", effectsB}}, - /*typeMap=*/{{"A", "B"}}); - - EXPECT_FALSE(merged.contains("A")); - ASSERT_TRUE(merged.contains("B")); - EXPECT_EQ(merged.at("B"), nullptr); -} - -TEST_F(IndirectCallEffectsTest, DestHasNullptrEffects) { - auto effectsA = std::make_shared(options, wasm); - effectsA->calls = true; - - auto merged = updateEffects(/*oldEffects=*/{{"A", effectsA}, {"B", nullptr}}, - /*typeMap=*/{{"A", "B"}}); - - EXPECT_FALSE(merged.contains("A")); - ASSERT_TRUE(merged.contains("B")); - EXPECT_EQ(merged.at("B"), nullptr); -} - TEST_F(IndirectCallEffectsTest, MergeThreeTypes) { auto effectsA = std::make_shared(options, wasm); effectsA->calls = true; @@ -173,8 +147,5 @@ TEST_F(IndirectCallEffectsTest, MergeThreeTypes) { EXPECT_FALSE(merged.contains("A")); EXPECT_FALSE(merged.contains("B")); EXPECT_FALSE(merged.contains("C")); - ASSERT_TRUE(merged.contains("D")); - EXPECT_TRUE(merged.at("D")->calls); - EXPECT_TRUE(merged.at("D")->writesMemory); - EXPECT_TRUE(merged.at("D")->throws_); + EXPECT_FALSE(merged.contains("D")); } From c811b0dbe568e3ec1d017b2b3eff7bcaf85e92a8 Mon Sep 17 00:00:00 2001 From: stevenfontanella Date: Wed, 8 Jul 2026 20:10:47 +0000 Subject: [PATCH 4/4] More changes --- test/gtest/CMakeLists.txt | 2 +- test/gtest/matchers/effects.h | 49 ++++++++++++++++++ test/gtest/type-updating.cpp | 95 +++++++++++++++++++++++------------ third_party/CMakeLists.txt | 6 +++ 4 files changed, 119 insertions(+), 33 deletions(-) create mode 100644 test/gtest/matchers/effects.h diff --git a/test/gtest/CMakeLists.txt b/test/gtest/CMakeLists.txt index 51591e61893..18c9464dadf 100644 --- a/test/gtest/CMakeLists.txt +++ b/test/gtest/CMakeLists.txt @@ -58,5 +58,5 @@ if(BUILD_FUZZTEST) target_link_libraries(binaryen-unittests PRIVATE gmock) gtest_discover_tests(binaryen-unittests) else() - target_link_libraries(binaryen-unittests PRIVATE gtest gtest_main) + target_link_libraries(binaryen-unittests PRIVATE gtest gtest_main gmock) endif() diff --git a/test/gtest/matchers/effects.h b/test/gtest/matchers/effects.h new file mode 100644 index 00000000000..34d4544c18e --- /dev/null +++ b/test/gtest/matchers/effects.h @@ -0,0 +1,49 @@ +/* + * Copyright 2026 WebAssembly Community Group participants + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef WASM_TEST_GTEST_MATCHERS_EFFECTS_H +#define WASM_TEST_GTEST_MATCHERS_EFFECTS_H + +#include "gmock/gmock.h" + +namespace wasm { + +MATCHER(BranchesOut, "") { return arg->branchesOut; } +MATCHER(Calls, "") { return arg->calls; } +MATCHER(ReadsMemory, "") { return arg->readsMemory; } +MATCHER(WritesMemory, "") { return arg->writesMemory; } +MATCHER(ReadsSharedMemory, "") { return arg->readsSharedMemory; } +MATCHER(WritesSharedMemory, "") { return arg->writesSharedMemory; } +MATCHER(ReadsTable, "") { return arg->readsTable; } +MATCHER(WritesTable, "") { return arg->writesTable; } +MATCHER(ReadsMutableStruct, "") { return arg->readsMutableStruct; } +MATCHER(WritesStruct, "") { return arg->writesStruct; } +MATCHER(ReadsSharedMutableStruct, "") { return arg->readsSharedMutableStruct; } +MATCHER(WritesSharedStruct, "") { return arg->writesSharedStruct; } +MATCHER(ReadsMutableArray, "") { return arg->readsMutableArray; } +MATCHER(WritesArray, "") { return arg->writesArray; } +MATCHER(ReadsSharedMutableArray, "") { return arg->readsSharedMutableArray; } +MATCHER(WritesSharedArray, "") { return arg->writesSharedArray; } +MATCHER(Traps, "") { return arg->trap; } +MATCHER(ImplicitTraps, "") { return arg->implicitTrap; } +MATCHER(Throws, "") { return arg->throws_; } +MATCHER(DanglingPop, "") { return arg->danglingPop; } +MATCHER(MayNotReturn, "") { return arg->mayNotReturn; } +MATCHER(HasReturnCallThrow, "") { return arg->hasReturnCallThrow; } + +} // namespace wasm + +#endif // WASM_TEST_GTEST_MATCHERS_EFFECTS_H diff --git a/test/gtest/type-updating.cpp b/test/gtest/type-updating.cpp index e9a52c5ba6c..025217e9d3d 100644 --- a/test/gtest/type-updating.cpp +++ b/test/gtest/type-updating.cpp @@ -1,5 +1,5 @@ /* - * Copyright 2021 WebAssembly Community Group participants + * Copyright 2026 WebAssembly Community Group participants * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -14,10 +14,12 @@ * limitations under the License. */ +#include "gmock/gmock.h" #include "gtest/gtest.h" #include "ir/effects.h" #include "ir/type-updating.h" +#include "matchers/effects.h" #include "parser/wat-parser.h" #include "support/result.h" #include "wasm.h" @@ -59,9 +61,21 @@ class IndirectCallEffectsTest : public Test { std::shared_ptr>& oldEffects, const std::unordered_map& typeMap) { - std::unordered_map types; + std::unordered_map types; + std::unordered_map typeNames; for (const auto& [type, name] : wasm.typeNames) { - types[name.name] = type; + types[name.name.toString()] = type; + typeNames[type] = name.name.toString(); + } + + // Type that previously didn't exist in the module. + // This is a new type created by GlobalTypeRewriter. + { + TypeBuilder builder(1); + builder.setHeapType(0, Signature(Type::i32, Type::i32)); + HeapType newType = (*builder.build())[0]; + types["new type"] = newType; + typeNames[newType] = "new type"; } for (const auto& [name, effects] : oldEffects) { @@ -70,7 +84,8 @@ class IndirectCallEffectsTest : public Test { GlobalTypeRewriter::TypeMap map; for (const auto& [oldName, newName] : typeMap) { - map[types.at(oldName)] = types.at(newName); + HeapType oldType = types.at(oldName); + map[oldType] = types.at(newName); } GlobalTypeRewriter rewriter(wasm, WorldMode::Open); @@ -79,7 +94,7 @@ class IndirectCallEffectsTest : public Test { std::unordered_map> result; for (const auto& [type, effects] : wasm.indirectCallEffects) { - auto name = wasm.typeNames.at(type).name.toString(); + auto name = typeNames.at(type); result[name] = effects; } return result; @@ -88,33 +103,24 @@ class IndirectCallEffectsTest : public Test { } // anonymous namespace -TEST_F(IndirectCallEffectsTest, SrcHasNoEffects) { +TEST_F(IndirectCallEffectsTest, SrcHasUnknownEffects) { auto effectsB = std::make_shared(options, wasm); effectsB->writesMemory = true; auto merged = updateEffects(/*oldEffects=*/{{"B", effectsB}}, /*typeMap=*/{{"A", "B"}}); - EXPECT_FALSE(merged.contains("A")); - EXPECT_FALSE(merged.contains("B")); + EXPECT_THAT(merged, IsEmpty()); } -TEST_F(IndirectCallEffectsTest, DestHasNoEffects) { +TEST_F(IndirectCallEffectsTest, DestHasUnknownEffects) { auto effectsA = std::make_shared(options, wasm); effectsA->calls = true; auto merged = updateEffects(/*oldEffects=*/{{"A", effectsA}}, /*typeMap=*/{{"A", "B"}}); - EXPECT_FALSE(merged.contains("A")); - EXPECT_FALSE(merged.contains("B")); -} - -TEST_F(IndirectCallEffectsTest, BothHaveNoEffects) { - auto merged = updateEffects(/*oldEffects=*/{}, /*typeMap=*/{{"A", "B"}}); - - EXPECT_FALSE(merged.contains("A")); - EXPECT_FALSE(merged.contains("B")); + EXPECT_THAT(merged, IsEmpty()); } TEST_F(IndirectCallEffectsTest, BothHaveEffects) { @@ -126,26 +132,51 @@ TEST_F(IndirectCallEffectsTest, BothHaveEffects) { auto merged = updateEffects(/*oldEffects=*/{{"A", effectsA}, {"B", effectsB}}, /*typeMap=*/{{"A", "B"}}); - EXPECT_FALSE(merged.contains("A")); - ASSERT_TRUE(merged.contains("B")); - EXPECT_TRUE(merged.at("B")->calls); - EXPECT_TRUE(merged.at("B")->writesMemory); + EXPECT_THAT(merged, + UnorderedElementsAre(Pair("B", AllOf(Calls(), WritesMemory())))); +} + +TEST_F(IndirectCallEffectsTest, MapToNewType) { + auto effectsA = std::make_shared(options, wasm); + effectsA->calls = true; + + auto merged = updateEffects(/*oldEffects=*/{{"A", effectsA}}, + /*typeMap=*/{{"A", "new type"}}); + + // Pointer comparison + EXPECT_THAT(merged, UnorderedElementsAre(Pair("new type", effectsA))); } -TEST_F(IndirectCallEffectsTest, MergeThreeTypes) { +TEST_F(IndirectCallEffectsTest, MapToNewTypeNoEffects) { + auto merged = updateEffects(/*oldEffects=*/{}, + /*typeMap=*/{{"A", "new type"}}); + + EXPECT_THAT(merged, IsEmpty()); +} + +TEST_F(IndirectCallEffectsTest, MergeNewTypeAndExisting) { + auto effectsB = std::make_shared(options, wasm); + effectsB->writesMemory = true; + + auto merged = + updateEffects(/*oldEffects=*/{{"B", effectsB}}, + /*typeMap=*/{{"A", "new type"}, {"B", "new type"}}); + + // Type A already had unknown effects, so the new type remains unknown. + EXPECT_THAT(merged, IsEmpty()); +} + +TEST_F(IndirectCallEffectsTest, MergeNewTypeAndExistingWithEffects) { auto effectsA = std::make_shared(options, wasm); effectsA->calls = true; auto effectsB = std::make_shared(options, wasm); effectsB->writesMemory = true; - auto effectsC = std::make_shared(options, wasm); - effectsC->throws_ = true; - auto merged = updateEffects( - /*oldEffects=*/{{"A", effectsA}, {"B", effectsB}, {"C", effectsC}}, - /*typeMap=*/{{"A", "D"}, {"B", "D"}, {"C", "D"}}); + auto merged = + updateEffects(/*oldEffects=*/{{"A", effectsA}, {"B", effectsB}}, + /*typeMap=*/{{"A", "new type"}, {"B", "new type"}}); - EXPECT_FALSE(merged.contains("A")); - EXPECT_FALSE(merged.contains("B")); - EXPECT_FALSE(merged.contains("C")); - EXPECT_FALSE(merged.contains("D")); + EXPECT_THAT( + merged, + UnorderedElementsAre(Pair("new type", AllOf(Calls(), WritesMemory())))); } diff --git a/third_party/CMakeLists.txt b/third_party/CMakeLists.txt index 5c77330e4fa..d704d2c7d68 100644 --- a/third_party/CMakeLists.txt +++ b/third_party/CMakeLists.txt @@ -4,11 +4,17 @@ elseif(BUILD_TESTS) # fuzztest includes gtest, but if we're not building fuzztest, build gtest ourselves. add_library(gtest STATIC googletest/googletest/src/gtest-all.cc) add_library(gtest_main STATIC googletest/googletest/src/gtest_main.cc) + add_library(gmock STATIC googletest/googlemock/src/gmock-all.cc) + add_library(gmock_main STATIC googletest/googlemock/src/gmock_main.cc) target_compile_options(gtest PRIVATE "-fno-rtti") target_compile_options(gtest_main PRIVATE "-fno-rtti") + target_compile_options(gmock PRIVATE "-fno-rtti") + target_compile_options(gmock_main PRIVATE "-fno-rtti") include_directories( googletest/googletest googletest/googletest/include + googletest/googlemock + googletest/googlemock/include ) endif()