Fix type updating for indirect call effects

stevenfontanella · stevenfontanella · commit 7b92d58156c0 · 2026-06-30T20:36:03.000Z
diff --git a/src/ir/linear-execution.h b/src/ir/linear-execution.h
@@ -187,7 +187,7 @@ struct LinearExecutionWalker : public PostWalker<SubType, VisitorType> {
 
           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<SubType, VisitorType> {
         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
@@ -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
@@ -26,6 +26,66 @@
 
 namespace wasm {
 
+namespace {
+
+std::unordered_map<HeapType, std::shared_ptr<const EffectAnalyzer>>
+updateIndirectCallEffects(const Module& wasm,
+                          const GlobalTypeRewriter::TypeMap& oldToNewTypes) {
+  std::unordered_map<HeapType, std::shared_ptr<const EffectAnalyzer>>
+    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<const EffectAnalyzer>* 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<EffectAnalyzer>(*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<HeapType, std::shared_ptr<const EffectAnalyzer>>
-    newTypeEffects;
-
-  for (const auto& [oldType, newType] : oldToNewTypes) {
-    std::shared_ptr<const EffectAnalyzer>* oldEffects =
-      find_or_null(wasm.indirectCallEffects, oldType);
-    std::shared_ptr<const EffectAnalyzer>* 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<EffectAnalyzer>(**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
@@ -321,9 +321,9 @@ void mergeMaybeEffects(std::shared_ptr<EffectAnalyzer>& 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<Function*, FuncInfo>& funcInfos,
   std::unordered_map<HeapType, std::shared_ptr<const EffectAnalyzer>>&
-    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
@@ -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
@@ -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<HeapType, std::shared_ptr<const EffectAnalyzer>>
     indirectCallEffects;
diff --git 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
@@ -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<std::string, std::shared_ptr<const EffectAnalyzer>>
+  updateEffects(
+    const std::unordered_map<std::string,
+                             std::shared_ptr<const EffectAnalyzer>>& oldEffects,
+    const std::unordered_map<std::string, std::string>& typeMap) {
+
+    std::unordered_map<Name, HeapType> 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<std::string, std::shared_ptr<const EffectAnalyzer>>
+      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<EffectAnalyzer>(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<EffectAnalyzer>(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<EffectAnalyzer>(options, wasm);
+  effectsA->calls = true;
+  auto effectsB = std::make_shared<EffectAnalyzer>(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<EffectAnalyzer>(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<EffectAnalyzer>(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<EffectAnalyzer>(options, wasm);
+  effectsA->calls = true;
+  auto effectsB = std::make_shared<EffectAnalyzer>(options, wasm);
+  effectsB->writesMemory = true;
+  auto effectsC = std::make_shared<EffectAnalyzer>(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_);
+}

Original file line number	Diff line number	Diff line change
`@@ -187,7 +187,7 @@ struct LinearExecutionWalker : public PostWalker<SubType, VisitorType> {`
`187`	`187`
`188`	`188`	`auto* effects = find_or_null(self->getModule()->indirectCallEffects,`
`189`	`189`	`callRef->target->type.getHeapType());`
`190`		`- if (!effects) {`
	`190`	`+ if (!effects \|\| !*effects) {`
`191`	`191`	`return false;`
`192`	`192`	`}`
`193`	`193`	`return !(*effects)->throws_;`
`@@ -203,7 +203,7 @@ struct LinearExecutionWalker : public PostWalker<SubType, VisitorType> {`
`203`	`203`	`if (self->getModule()) {`
`204`	`204`	`if (auto* effects = find_or_null(`
`205`	`205`	`self->getModule()->indirectCallEffects, callIndirect->heapType);`
`206`		`- effects) {`
	`206`	`+ effects && *effects) {`
`207`	`207`	`refutesThrowEffect = !(*effects)->throws_;`
`208`	`208`	`}`
`209`	`209`	`}`
Original file line number	Diff line number	Diff line change
`@@ -34,6 +34,7 @@ set(unittest_SOURCES`
`34`	`34`	`suffix_tree.cpp`
`35`	`35`	`topological-sort.cpp`
`36`	`36`	`type-builder.cpp`
	`37`	`+ type-updating.cpp`
`37`	`38`	`wat-lexer.cpp`
`38`	`39`	`validator.cpp`
`39`	`40`	`)`