From cb25670922785c62edfc060ebd9b9d926758f086 Mon Sep 17 00:00:00 2001 From: stevenfontanella Date: Fri, 12 Jun 2026 20:12:03 +0000 Subject: [PATCH 1/2] 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/2] 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;