wasm2js: Fix switch lowering, don't fall through after the hoisted parts (#2301)

The switch lowering will "hoist" blocks of code into the JS switch when it can. If it can hoist some but not others, it must not fall through into those others (while it can fall through the hoisted ones - they began as nested blocks with falling-through between them). To fix this, after the hoisted ones issue a break out of the switch (which now contains all the hoisted code, so breaking out of it gets to the code right after the hoisted ones).

fixes #2300

8 files changed, 137 insertions, 38 deletions

diff --git a/src/wasm2js.h b/src/wasm2js.h
index 25c51f490..624847f7d 100644
--- a/src/wasm2js.h
+++ b/src/wasm2js.h
@@ -1013,43 +1013,6 @@ Ref Wasm2JSBuilder::processFunctionBody(Module* m,
     Expression* defaultBody = nullptr; // default must be last in asm.js
 
     Ref visitSwitch(Switch* curr) {
-#if 0
-      // Simple switch emitting. This is valid but may lead to block nesting of a size
-      // that JS engines can't handle.
-      Ref ret = ValueBuilder::makeBlock();
-      Ref condition = visit(curr->condition, EXPRESSION_RESULT);
-      Ref theSwitch =
-        ValueBuilder::makeSwitch(makeAsmCoercion(condition, ASM_INT));
-      ret[1]->push_back(theSwitch);
-      // First, group the switch targets.
-      std::map<Name, std::vector<Index>> targetIndexes;
-      for (size_t i = 0; i < curr->targets.size(); i++) {
-        targetIndexes[curr->targets[i]].push_back(i);
-      }
-      // Emit group by group.
-      for (auto& pair : targetIndexes) {
-        auto target = pair.first;
-        auto& indexes = pair.second;
-        if (target != curr->default_) {
-          for (auto i : indexes) {
-            ValueBuilder::appendCaseToSwitch(theSwitch,
-                                             ValueBuilder::makeNum(i));
-          }
-          ValueBuilder::appendCodeToSwitch(
-            theSwitch, blockify(makeBreakOrContinue(target)), false);
-        } else {
-          // For the group going to the same place as the default, we can just
-          // emit the default itself, which we do at the end.
-        }
-      }
-      // TODO: if the group the default is in is not the largest, we can turn
-      // the largest into
-      //       the default by using a local and a check on the range
-      ValueBuilder::appendDefaultToSwitch(theSwitch);
-      ValueBuilder::appendCodeToSwitch(
-        theSwitch, blockify(makeBreakOrContinue(curr->default_)), false);
-      return ret;
-#else
       // Even without optimizations, we work hard here to emit minimal and
       // especially minimally-nested code, since otherwise we may get block
       // nesting of a size that JS engines can't handle.
@@ -1064,6 +1027,7 @@ Ref Wasm2JSBuilder::processFunctionBody(Module* m,
       // Emit first any hoisted groups.
       auto& hoistedCases = switchProcessor.hoistedSwitchCases[curr];
       std::set<Name> emittedTargets;
+      bool hoistedEndsWithUnreachable = false;
       for (auto& case_ : hoistedCases) {
         auto target = case_.target;
         auto& code = case_.code;
@@ -1080,8 +1044,23 @@ Ref Wasm2JSBuilder::processFunctionBody(Module* m,
         for (auto* c : code) {
           ValueBuilder::appendCodeToSwitch(
             theSwitch, blockify(visit(c, NO_RESULT)), false);
+          hoistedEndsWithUnreachable = c->type == unreachable;
         }
       }
+      // After the hoisted cases, if any remain we must make sure not to
+      // fall through into them. If no code was hoisted, this is unnecessary,
+      // and if the hoisted code ended with an unreachable it also is not
+      // necessary.
+      bool stoppedFurtherFallthrough = false;
+      auto stopFurtherFallthrough = [&]() {
+        if (!stoppedFurtherFallthrough && !hoistedCases.empty() &&
+            !hoistedEndsWithUnreachable) {
+          stoppedFurtherFallthrough = true;
+          ValueBuilder::appendCodeToSwitch(
+            theSwitch, blockify(ValueBuilder::makeBreak(IString())), false);
+        }
+      };
+
       // Emit any remaining groups by just emitting branches to their code,
       // which will appear outside the switch.
       for (auto& pair : targetIndexes) {
@@ -1090,6 +1069,7 @@ Ref Wasm2JSBuilder::processFunctionBody(Module* m,
         if (emittedTargets.count(target)) {
           continue;
         }
+        stopFurtherFallthrough();
         if (target != curr->default_) {
           for (auto i : indexes) {
             ValueBuilder::appendCaseToSwitch(theSwitch,
@@ -1106,12 +1086,12 @@ Ref Wasm2JSBuilder::processFunctionBody(Module* m,
       // the largest into
       //       the default by using a local and a check on the range
       if (!emittedTargets.count(curr->default_)) {
+        stopFurtherFallthrough();
         ValueBuilder::appendDefaultToSwitch(theSwitch);
         ValueBuilder::appendCodeToSwitch(
           theSwitch, blockify(makeBreakOrContinue(curr->default_)), false);
       }
       return theSwitch;
-#endif
     }
 
     Ref visitCall(Call* curr) {
diff --git a/test/wasm2js/br_table_hoisting.2asm.js b/test/wasm2js/br_table_hoisting.2asm.js
index e1d743cfe..8d5e45440 100644
--- a/test/wasm2js/br_table_hoisting.2asm.js
+++ b/test/wasm2js/br_table_hoisting.2asm.js
@@ -104,6 +104,7 @@ function asmFunc(global, env, buffer) {
        }
        zed(-1 | 0);
        zed(-2 | 0);
+       break;
       case 0:
        break a;
       case 1:
diff --git a/test/wasm2js/br_table_hoisting.2asm.js.opt b/test/wasm2js/br_table_hoisting.2asm.js.opt
index 44c9d0417..f28214760 100644
--- a/test/wasm2js/br_table_hoisting.2asm.js.opt
+++ b/test/wasm2js/br_table_hoisting.2asm.js.opt
@@ -100,6 +100,7 @@ function asmFunc(global, env, buffer) {
        }
        zed(-1);
        zed(-2);
+       break;
       case 0:
        break a;
       case 1:
@@ -140,6 +141,7 @@ function asmFunc(global, env, buffer) {
        }
        zed(-1);
        zed(-2);
+       break;
       case 0:
        break a;
       case 1:
diff --git a/test/wasm2js/br_table_temp.2asm.js.opt b/test/wasm2js/br_table_temp.2asm.js.opt
index bc29f2c3a..eba815912 100644
--- a/test/wasm2js/br_table_temp.2asm.js.opt
+++ b/test/wasm2js/br_table_temp.2asm.js.opt
@@ -12571,6 +12571,7 @@ function asmFunc(global, env, buffer) {
     $2 = 18;
    case 1:
     $1 = $2 + 1 | 0;
+    break;
    default:
     break block;
    };
@@ -12589,6 +12590,7 @@ function asmFunc(global, env, buffer) {
     $2 = 16;
    case 1:
     $1 = $2 + 1 | 0;
+    break;
    case 0:
     break block;
    };
@@ -12607,6 +12609,7 @@ function asmFunc(global, env, buffer) {
     $2 = 16;
    case 1:
     $1 = $2 + 1 | 0;
+    break;
    default:
     break block;
    };
diff --git a/test/wasm2js/excess_fallthrough.2asm.js b/test/wasm2js/excess_fallthrough.2asm.js
new file mode 100644
index 000000000..e5c4bcd1a
--- /dev/null
+++ b/test/wasm2js/excess_fallthrough.2asm.js
@@ -0,0 +1,55 @@
+
+function asmFunc(global, env, buffer) {
+ var HEAP8 = new global.Int8Array(buffer);
+ var HEAP16 = new global.Int16Array(buffer);
+ var HEAP32 = new global.Int32Array(buffer);
+ var HEAPU8 = new global.Uint8Array(buffer);
+ var HEAPU16 = new global.Uint16Array(buffer);
+ var HEAPU32 = new global.Uint32Array(buffer);
+ var HEAPF32 = new global.Float32Array(buffer);
+ var HEAPF64 = new global.Float64Array(buffer);
+ var Math_imul = global.Math.imul;
+ var Math_fround = global.Math.fround;
+ var Math_abs = global.Math.abs;
+ var Math_clz32 = global.Math.clz32;
+ var Math_min = global.Math.min;
+ var Math_max = global.Math.max;
+ var Math_floor = global.Math.floor;
+ var Math_ceil = global.Math.ceil;
+ var Math_sqrt = global.Math.sqrt;
+ var abort = env.abort;
+ var nan = global.NaN;
+ var infinity = global.Infinity;
+ function bar() {
+  
+ }
+ 
+ function foo($0) {
+  $0 = $0 | 0;
+  label$4 : while (1) {
+   label$5 : {
+    bar();
+    block : {
+     switch (123 | 0) {
+     case 0:
+      bar();
+      break;
+     default:
+      break label$5;
+     };
+    }
+    return;
+   }
+   abort();
+  };
+ }
+ 
+ var FUNCTION_TABLE = [];
+ return {
+  "foo": foo
+ };
+}
+
+var memasmFunc = new ArrayBuffer(65536);
+var retasmFunc = asmFunc({Math,Int8Array,Uint8Array,Int16Array,Uint16Array,Int32Array,Uint32Array,Float32Array,Float64Array,NaN,Infinity}, {abort:function() { throw new Error('abort'); }},memasmFunc);
+export var foo = retasmFunc.foo;
diff --git a/test/wasm2js/excess_fallthrough.2asm.js.opt b/test/wasm2js/excess_fallthrough.2asm.js.opt
new file mode 100644
index 000000000..a805107be
--- /dev/null
+++ b/test/wasm2js/excess_fallthrough.2asm.js.opt
@@ -0,0 +1,36 @@
+
+function asmFunc(global, env, buffer) {
+ var HEAP8 = new global.Int8Array(buffer);
+ var HEAP16 = new global.Int16Array(buffer);
+ var HEAP32 = new global.Int32Array(buffer);
+ var HEAPU8 = new global.Uint8Array(buffer);
+ var HEAPU16 = new global.Uint16Array(buffer);
+ var HEAPU32 = new global.Uint32Array(buffer);
+ var HEAPF32 = new global.Float32Array(buffer);
+ var HEAPF64 = new global.Float64Array(buffer);
+ var Math_imul = global.Math.imul;
+ var Math_fround = global.Math.fround;
+ var Math_abs = global.Math.abs;
+ var Math_clz32 = global.Math.clz32;
+ var Math_min = global.Math.min;
+ var Math_max = global.Math.max;
+ var Math_floor = global.Math.floor;
+ var Math_ceil = global.Math.ceil;
+ var Math_sqrt = global.Math.sqrt;
+ var abort = env.abort;
+ var nan = global.NaN;
+ var infinity = global.Infinity;
+ function foo($0) {
+  $0 = $0 | 0;
+  abort();
+ }
+ 
+ var FUNCTION_TABLE = [];
+ return {
+  "foo": foo
+ };
+}
+
+var memasmFunc = new ArrayBuffer(65536);
+var retasmFunc = asmFunc({Math,Int8Array,Uint8Array,Int16Array,Uint16Array,Int32Array,Uint32Array,Float32Array,Float64Array,NaN,Infinity}, {abort:function() { throw new Error('abort'); }},memasmFunc);
+export var foo = retasmFunc.foo;
diff --git a/test/wasm2js/excess_fallthrough.wast b/test/wasm2js/excess_fallthrough.wast
new file mode 100644
index 000000000..a31ba88f3
--- /dev/null
+++ b/test/wasm2js/excess_fallthrough.wast
@@ -0,0 +1,21 @@
+(module
+ (export "foo" (func $foo))
+ (func $bar)
+ (func $foo (param $0 i32)
+  (loop $label$4
+   (block $label$5
+    (call $bar)
+    (block
+     (block $label$7
+      (br_table $label$7 $label$5
+       (i32.const 123)
+      )
+     )
+     (call $bar)
+    )
+    (return)
+   )
+   (unreachable)
+  )
+ )
+)
diff --git a/test/wasm2js/switch.2asm.js b/test/wasm2js/switch.2asm.js
index b7583298e..3c2572315 100644
--- a/test/wasm2js/switch.2asm.js
+++ b/test/wasm2js/switch.2asm.js
@@ -46,6 +46,7 @@ function asmFunc(global, env, buffer) {
      j = 101;
     default:
      j = 102;
+     break;
     case 7:
      break $7;
     };