BranchHinting fuzzing: Do not remove effects when deinstrumenting (#8613)

The BranchHinting fuzzer broke after #8608, but it wasn't that PR's fault -
just that we now generate a different pattern of blocks that BranchHinting's
deinstrumentation did not handle yet. The problem was that the block with
the condition that we replace now has effects, so we can't always remove it
wholesale.

Author: kripken

src/passes/InstrumentBranchHints.cpp

@@ -96,6 +96,7 @@
 //
 
 #include "ir/drop.h"
+#include "ir/effects.h"
 #include "ir/eh-utils.h"
 #include "ir/find_all.h"
 #include "ir/local-graph.h"
@@ -272,6 +273,8 @@ struct InstrumentationProcessor : public WalkerPass<PostWalker<Sub>> {
     // The condition before the instrumentation (a pointer to it, so we can
     // replace it).
     Expression** originalCondition;
+    // The local that the original condition is stored in temporarily.
+    Index tempLocal;
     // The call to the logging that the instrumentation added.
     Call* call;
   };
@@ -330,7 +333,7 @@ struct InstrumentationProcessor : public WalkerPass<PostWalker<Sub>> {
       return {};
     }
     // Great, this is indeed a prior instrumentation.
-    return Instrumentation{&set->value, call};
+    return Instrumentation{&set->value, set->index, call};
   }
 };
 
@@ -374,7 +377,27 @@ struct DeInstrumentBranchHints
       // IR, and the original condition is still used in another place, until
       // we remove the logging calls; since we will remove the calls anyhow, we
       // just need some valid IR there).
-      std::swap(curr->condition, *info->originalCondition);
+      //
+      // Check for dangerous effects in the condition we are about to replace,
+      // to avoid a situation where the condition looks like this:
+      //
+      //  (set $temp (original condition))
+      //   ..effects..
+      //  (local.get $temp)
+      //
+      // We cannot replace all this with the original condition, as it would
+      // remove the effects. (Even in that case we will remove the actual call
+      // to log the branch hint, below, so this just prevents some cleanup that
+      // is normally safe - the cleanup is mainly useful to allow inspection of
+      // testcases for debugging.)
+      EffectAnalyzer effects(getPassOptions(), *getModule(), curr->condition);
+      // The only condition we allow is a write to the temp local from the
+      // instrumentation, which getInstrumentation() verified has no other uses
+      // than us.
+      effects.localsWritten.erase(info->tempLocal);
+      if (!effects.hasUnremovableSideEffects()) {
+        std::swap(curr->condition, *info->originalCondition);
+      }
     }
   }
 
@@ -403,6 +426,21 @@ struct DeInstrumentBranchHints
       }
     }
   }
+
+  void doWalkModule(Module* module) {
+    auto logBranchImport = getLogBranchImport(module);
+    if (!logBranchImport) {
+      Fatal()
+        << "No branch hint logging import found. Was this code instrumented?";
+    }
+
+    // Mark the log-branch import as having no side effects - we are removing it
+    // entirely here, and its effect should not stop us when we compute effects.
+    module->getFunction(logBranchImport)->effects =
+      std::make_shared<EffectAnalyzer>(getPassOptions(), *module);
+
+    InstrumentationProcessor<DeInstrumentBranchHints>::doWalkModule(module);
+  }
 };
 
 } // anonymous namespace

test/lit/passes/deinstrument-branch-hints.wast

@@ -7,6 +7,8 @@
 
   ;; CHECK:      (type $1 (func (param i32 i32 i32)))
 
+  ;; CHECK:      (type $2 (func (result i32)))
+
   ;; CHECK:      (import "fuzzing-support" "log-branch" (func $log (type $1) (param i32 i32 i32)))
   (import "fuzzing-support" "log-branch" (func $log (param i32 i32 i32)))
 
@@ -164,4 +166,57 @@
       )
     )
   )
+
+  ;; CHECK:      (func $if-unreachable (type $2) (result i32)
+  ;; CHECK-NEXT:  (local $0 i32)
+  ;; CHECK-NEXT:  (block $block (result i32)
+  ;; CHECK-NEXT:   (br_if $block
+  ;; CHECK-NEXT:    (i32.const 0)
+  ;; CHECK-NEXT:    (block (result i32)
+  ;; CHECK-NEXT:     (block
+  ;; CHECK-NEXT:      (local.set $0
+  ;; CHECK-NEXT:       (i32.const 42)
+  ;; CHECK-NEXT:      )
+  ;; CHECK-NEXT:      (if
+  ;; CHECK-NEXT:       (i32.const 1)
+  ;; CHECK-NEXT:       (then
+  ;; CHECK-NEXT:        (unreachable)
+  ;; CHECK-NEXT:       )
+  ;; CHECK-NEXT:      )
+  ;; CHECK-NEXT:     )
+  ;; CHECK-NEXT:     (nop)
+  ;; CHECK-NEXT:     (local.get $0)
+  ;; CHECK-NEXT:    )
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT: )
+  (func $if-unreachable (result i32)
+    (local $0 i32)
+    ;; The unreachable here must be executed. Normally we replace the br_if's
+    ;; entire condition, but here we only remove the call to $log.
+    (block $block (result i32)
+      (br_if $block
+        (i32.const 0)
+        (block (result i32)
+          (block
+            (local.set $0
+              (i32.const 42)
+            )
+            (if
+              (i32.const 1)
+              (then
+                (unreachable)
+              )
+            )
+          )
+          (call $log
+            (i32.const 0)
+            (i32.const 0)
+            (local.get $0)
+          )
+          (local.get $0)
+        )
+      )
+    )
+  )
 )