Fix type updating for indirect call effects

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_;
           }
         }

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()) {

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) {

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();
   }
 };
 

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.

src/wasm.h

@@ -2730,6 +2730,9 @@ class Module {
   Name name;
 
   std::unordered_map<HeapType, TypeNames> typeNames;
+
+  // The source binary's type indicies. Used in some cases for preserving
+  // ordering of types
   std::unordered_map<HeapType, Index> typeIndices;
 
   // Potential effects for bodies of indirect calls to this type. Populated by
@@ -2746,6 +2749,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.
+  // 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<HeapType, std::shared_ptr<const EffectAnalyzer>>
     indirectCallEffects;

src/wasm/wasm-type.cpp

@@ -2764,24 +2764,15 @@ std::unordered_set<HeapType> 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
 

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
 )