From 8a6964014cc1198acda11646c4b2f79406c5fec2 Mon Sep 17 00:00:00 2001
From: Alon Zakai <alonzakai@gmail.com>
Date: Fri, 17 Nov 2017 21:36:40 -0800
Subject: [PATCH] Fix a code-folding fuzz bug (#1282)

* fix a code-folding bug where when merging function-level tails, we moved code out of where it could reach a break target - we must not move code if it has a break target not enclosed in itself. the EffectAnalyzer already had the functionality for that, move the code around a little there to make that clearer too
---
 src/ir/effects.h              |  3 ++
 src/passes/CodeFolding.cpp    | 14 ++++++--
 test/passes/code-folding.txt  | 57 ++++++++++++++++++++++++++++++++
 test/passes/code-folding.wast | 62 +++++++++++++++++++++++++++++++++++
 4 files changed, 134 insertions(+), 2 deletions(-)

diff --git a/src/ir/effects.h b/src/ir/effects.h
index 98911d451eb..b6eafae27b3 100644
--- a/src/ir/effects.h
+++ b/src/ir/effects.h
@@ -62,6 +62,9 @@ struct EffectAnalyzer : public PostWalker<EffectAnalyzer> {
   bool hasSideEffects() { return hasGlobalSideEffects() || localsWritten.size() > 0 || branches || implicitTrap; }
   bool hasAnything() { return branches || calls || accessesLocal() || readsMemory || writesMemory || accessesGlobal() || implicitTrap || isAtomic; }
 
+  // check if we break to anything external from ourselves
+  bool hasExternalBreakTargets() { return !breakNames.empty(); }
+
   // checks if these effects would invalidate another set (e.g., if we write, we invalidate someone that reads, they can't be moved past us)
   bool invalidates(EffectAnalyzer& other) {
     if (branches || other.branches
diff --git a/src/passes/CodeFolding.cpp b/src/passes/CodeFolding.cpp
index 415cf38de23..afcd3515ca4 100644
--- a/src/passes/CodeFolding.cpp
+++ b/src/passes/CodeFolding.cpp
@@ -61,6 +61,7 @@
 #include "wasm-builder.h"
 #include "ir/utils.h"
 #include "ir/branch-utils.h"
+#include "ir/effects.h"
 #include "ir/label-utils.h"
 
 namespace wasm {
@@ -474,9 +475,18 @@ struct CodeFolding : public WalkerPass<ControlFlowWalker<CodeFolding>> {
     };
     // let's see if we can merge deeper than num, to num + 1
     auto next = tails;
-    // remove tails that are too short
+    // remove tails that are too short, or that we hit an item we can't handle
     next.erase(std::remove_if(next.begin(), next.end(), [&](Tail& tail) {
-      return effectiveSize(tail) < num + 1;
+      if (effectiveSize(tail) < num + 1) return true;
+      auto* newItem = getItem(tail, num);
+      // ignore tails that break to outside blocks. we want to move code to
+      // the very outermost position, so such code cannot be moved
+      // TODO: this should not be a problem in *non*-terminating tails,
+      //       but double-verify that
+      if (EffectAnalyzer(getPassOptions(), newItem).hasExternalBreakTargets()) {
+        return true;
+      }
+      return false;
     }), next.end());
     // if we have enough to investigate, do so
     if (next.size() >= 2) {
diff --git a/test/passes/code-folding.txt b/test/passes/code-folding.txt
index 031f93b91fd..e11999ac857 100644
--- a/test/passes/code-folding.txt
+++ b/test/passes/code-folding.txt
@@ -49,4 +49,61 @@
    (f32.const 0)
   )
  )
+ (func $break-target-outside-of-return-merged-code (; 4 ;) (type $1)
+  (block $label$A
+   (if
+    (unreachable)
+    (block $block
+     (block $block0
+      (block $label$B
+       (if
+        (unreachable)
+        (br_table $label$A $label$B
+         (unreachable)
+        )
+       )
+      )
+      (return)
+     )
+    )
+    (block $block2
+     (block $label$C
+      (if
+       (unreachable)
+       (br_table $label$A $label$C
+        (unreachable)
+       )
+      )
+     )
+     (return)
+    )
+   )
+  )
+ )
+ (func $break-target-inside-all-good (; 5 ;) (type $1)
+  (block $folding-inner0
+   (block $label$A
+    (if
+     (unreachable)
+     (block $block
+      (block $block4
+       (br $folding-inner0)
+      )
+     )
+     (block $block6
+      (br $folding-inner0)
+     )
+    )
+   )
+  )
+  (block $label$B
+   (if
+    (unreachable)
+    (br_table $label$B $label$B
+     (unreachable)
+    )
+   )
+  )
+  (return)
+ )
 )
diff --git a/test/passes/code-folding.wast b/test/passes/code-folding.wast
index 32a32b28b2c..2064f2db6a5 100644
--- a/test/passes/code-folding.wast
+++ b/test/passes/code-folding.wast
@@ -52,5 +52,67 @@
    )
   )
  )
+ (func $break-target-outside-of-return-merged-code
+  (block $label$A
+   (if
+    (unreachable)
+    (block
+     (block
+      (block $label$B
+       (if
+        (unreachable)
+        (br_table $label$A $label$B
+         (unreachable)
+        )
+       )
+      )
+      (return)
+     )
+    )
+    (block
+     (block $label$C
+      (if
+       (unreachable)
+       (br_table $label$A $label$C ;; this all looks mergeable, but $label$A is outside
+        (unreachable)
+       )
+      )
+     )
+     (return)
+    )
+   )
+  )
+ )
+ (func $break-target-inside-all-good
+  (block $label$A
+   (if
+    (unreachable)
+    (block
+     (block
+      (block $label$B
+       (if
+        (unreachable)
+        (br_table $label$B $label$B
+         (unreachable)
+        )
+       )
+      )
+      (return)
+     )
+    )
+    (block
+     (block $label$C
+      (if
+       (unreachable)
+       (br_table $label$C $label$C ;; this all looks mergeable, and is, B ~~ C
+        (unreachable)
+       )
+      )
+     )
+     (return)
+    )
+   )
+  )
+ )
 )