[Nominal Fuzzing] Fix SignatureRefining by updating types fully at once (#4665)

Optionally avoid updating types in TypeUpdating::updateParamTypes(). That update is incomplete if the function signature is also changing, which is the case in SignatureRefining (but not DeadArgumentElimination). "Incomplete" means that we updated the local.get type, but the function signature does not match yet. That incomplete state can hit an internal error in GlobalTypeRewriter::updateSignatures where it updates types. To avoid that, do the entire full update only there (in GlobalTypeRewriter::updateSignatures).
author: Alon Zakai <azakai@google.com> 2022-05-23 12:44:52 -0700
committer: GitHub <noreply@github.com> 2022-05-23 19:44:52 +0000
commit: a78d0e44cbcc72682ab9c45dec76d4b1c52588c9 (patch)
tree: 8362d3ca459b5c048a7e539eb63e5738f5c18b06 /src
parent: fa3ffd0c2697fde7705495b52b139f7939f97925 (diff)
download: binaryen-a78d0e44cbcc72682ab9c45dec76d4b1c52588c9.tar.gz
binaryen-a78d0e44cbcc72682ab9c45dec76d4b1c52588c9.tar.bz2
binaryen-a78d0e44cbcc72682ab9c45dec76d4b1c52588c9.zip
3 files changed, 66 insertions, 20 deletions
diff --git a/src/ir/type-updating.cpp b/src/ir/type-updating.cpp
index ff154dd2f..b9bbf7a03 100644
--- a/src/ir/type-updating.cpp
+++ b/src/ir/type-updating.cpp
@@ -137,6 +137,26 @@ void GlobalTypeRewriter::update() {
     }
 
     void visitExpression(Expression* curr) {
+      // local.get and local.tee are special in that their type is tied to the
+      // type of the local in the function, which is tied to the signature. That
+      // means we must update it based on the signature, and not on the old type
+      // in the local.
+      //
+      // We have already updated function signatures by the time we get here,
+      // which means we can just apply the current local type that we see (there
+      // is no need to call getNew(), which we already did on the function's
+      // signature itself).
+      if (auto* get = curr->dynCast<LocalGet>()) {
+        curr->type = getFunction()->getLocalType(get->index);
+        return;
+      } else if (auto* tee = curr->dynCast<LocalSet>()) {
+        // Rule out a local.set and unreachable code.
+        if (tee->type != Type::none && tee->type != Type::unreachable) {
+          curr->type = getFunction()->getLocalType(tee->index);
+        }
+        return;
+      }
+
       // Update the type to the new one.
       curr->type = getNew(curr->type);
 
@@ -172,6 +192,16 @@ void GlobalTypeRewriter::update() {
 
   CodeUpdater updater(oldToNewTypes);
   PassRunner runner(&wasm);
+
+  // Update functions first, so that we see the updated types for locals (which
+  // can change if the function signature changes).
+  for (auto& func : wasm.functions) {
+    func->type = updater.getNew(func->type);
+    for (auto& var : func->vars) {
+      var = updater.getNew(var);
+    }
+  }
+
   updater.run(&runner, &wasm);
   updater.walkModuleCode(&wasm);
 
@@ -185,12 +215,6 @@ void GlobalTypeRewriter::update() {
   for (auto& global : wasm.globals) {
     global->type = updater.getNew(global->type);
   }
-  for (auto& func : wasm.functions) {
-    func->type = updater.getNew(func->type);
-    for (auto& var : func->vars) {
-      var = updater.getNew(var);
-    }
-  }
   for (auto& tag : wasm.tags) {
     tag->sig = updater.getNew(tag->sig);
   }
@@ -323,7 +347,8 @@ Expression* fixLocalGet(LocalGet* get, Module& wasm) {
 
 void updateParamTypes(Function* func,
                       const std::vector<Type>& newParamTypes,
-                      Module& wasm) {
+                      Module& wasm,
+                      LocalUpdatingMode localUpdating) {
   // Before making this update, we must be careful if the param was "reused",
   // specifically, if it is assigned a less-specific type in the body then
   // we'd get a validation error when we refine it. To handle that, if a less-
@@ -369,7 +394,11 @@ void updateParamTypes(Function* func,
       if (iter != paramFixups.end()) {
         auto fixup = iter->second;
         contents.push_back(builder.makeLocalSet(
-          fixup, builder.makeLocalGet(index, newParamTypes[index])));
+          fixup,
+          builder.makeLocalGet(index,
+                               localUpdating == Update
+                                 ? newParamTypes[index]
+                                 : func->getLocalType(index))));
       }
     }
     contents.push_back(func->body);
@@ -391,17 +420,19 @@ void updateParamTypes(Function* func,
   }
 
   // Update local.get/local.tee operations that use the modified param type.
-  for (auto* get : gets.list) {
-    auto index = get->index;
-    if (func->isParam(index)) {
-      get->type = newParamTypes[index];
+  if (localUpdating == Update) {
+    for (auto* get : gets.list) {
+      auto index = get->index;
+      if (func->isParam(index)) {
+        get->type = newParamTypes[index];
+      }
     }
-  }
-  for (auto* set : sets.list) {
-    auto index = set->index;
-    if (func->isParam(index) && set->isTee()) {
-      set->type = newParamTypes[index];
-      set->finalize();
+    for (auto* set : sets.list) {
+      auto index = set->index;
+      if (func->isParam(index) && set->isTee()) {
+        set->type = newParamTypes[index];
+        set->finalize();
+      }
     }
   }
 
diff --git a/src/ir/type-updating.h b/src/ir/type-updating.h
index d1c4af6bc..f6df449ae 100644
--- a/src/ir/type-updating.h
+++ b/src/ir/type-updating.h
@@ -415,9 +415,16 @@ Expression* fixLocalGet(LocalGet* get, Module& wasm);
 // function does are to update the types of local.get/tee operations,
 // refinalize, etc., basically all operations necessary to ensure validation
 // with the new types.
+//
+// While doing so, we can either update or not update the types of local.get and
+// local.tee operations. (We do not update them here if we'll be doing an update
+// later in the caller, which is the case if we are rewriting function types).
+enum LocalUpdatingMode { Update, DoNotUpdate };
+
 void updateParamTypes(Function* func,
                       const std::vector<Type>& newParamTypes,
-                      Module& wasm);
+                      Module& wasm,
+                      LocalUpdatingMode localUpdating = Update);
 
 } // namespace TypeUpdating
 
diff --git a/src/passes/SignatureRefining.cpp b/src/passes/SignatureRefining.cpp
index 2c2146c3f..bde1c4fb9 100644
--- a/src/passes/SignatureRefining.cpp
+++ b/src/passes/SignatureRefining.cpp
@@ -243,7 +243,15 @@ struct SignatureRefining : public Pass {
           for (auto param : iter->second.params) {
             newParamsTypes.push_back(param);
           }
-          TypeUpdating::updateParamTypes(func, newParamsTypes, wasm);
+          // Do not update local.get/local.tee here, as we will do so in
+          // GlobalTypeRewriter::updateSignatures, below. (Doing an update here
+          // would leave the IR in an inconsistent state of a partial update;
+          // instead, do the full update at the end.)
+          TypeUpdating::updateParamTypes(
+            func,
+            newParamsTypes,
+            wasm,
+            TypeUpdating::LocalUpdatingMode::DoNotUpdate);
         }
       }
     };
author	Alon Zakai <azakai@google.com>	2022-05-23 12:44:52 -0700
committer	GitHub <noreply@github.com>	2022-05-23 19:44:52 +0000
commit	a78d0e44cbcc72682ab9c45dec76d4b1c52588c9 (patch)
tree	8362d3ca459b5c048a7e539eb63e5738f5c18b06 /src
parent	fa3ffd0c2697fde7705495b52b139f7939f97925 (diff)
download	binaryen-a78d0e44cbcc72682ab9c45dec76d4b1c52588c9.tar.gz binaryen-a78d0e44cbcc72682ab9c45dec76d4b1c52588c9.tar.bz2 binaryen-a78d0e44cbcc72682ab9c45dec76d4b1c52588c9.zip