Ignore unreachable code in wasm binaries (#1122)

Ignoring unreachable code in wasm binaries lets us avoid corner cases with unstructured code in wasm binaries that is a poor fit for Binaryen's structured IR.

Author: kripken

README.md

@@ -36,7 +36,10 @@ The differences between Binaryen IR and WebAssembly are:
 
  * Binaryen IR [is an AST](https://github.com/WebAssembly/binaryen/issues/663), for convenience of optimization. This differs from the WebAssembly binary format which is a stack machine.
  * WebAssembly limits block/if/loop types to none and the concrete value types (i32, i64, f32, f64). Binaryen IR has an unreachable type, and it allows block/if/loop to take it, allowing [local transforms that don't need to know the global context](https://github.com/WebAssembly/binaryen/issues/903).
- * Binaryen IR's text format requires the names of blocks and loops to be unique. This differs from the WebAssembly s-expression format which allows duplicate names (and depends on scoping to disambiguate).
+ * Binaryen IR requires the names of blocks and loops to be unique. (Reading wast files with duplicate names is supported, by disambiguating them).
+ * Binaryen IR has only one node with a list: blocks. WebAssembly on the other hand allows lists in loops and ifs (Binaryen would represent those with additional blocks as necessary). The motivation here is that many passes need special code for iterating on lists, so having a single IR node with a list simplifies things.
+ * Binaryen's text format allows only s-expressions. WebAssembly's official text format is a stack machine with s-expression extensions. Binaryen can't read stack machine code, but it can read a wast if it contains only s-expressions.
+ * Binaryen ignores unreachable code when reading WebAssembly binaries. That means that if you read a wasm file with unreachable code, that code will be discarded as if it were optimized out (often this is what you want anyhow, and optimized programs have no unreachable code anyway, but if you write an unoptimized file and then read it, it may look different). The reason for this behavior is that unreachable code in WebAssembly has corner cases that are tricky to handle in Binaryen IR (it can be very unstructured, and Binaryen IR is more structured than WebAssembly as noted earlier). Note that Binaryen does support unreachable code in wast text files, since as we saw Binaryen only supports s-expressions there, which are structured.
 
 As a result, you might notice that round-trip conversions (wasm => Binaryen IR => wasm) change code a little in some corner cases.
 

src/wasm-binary.h

@@ -855,9 +855,15 @@ class WasmBinaryBuilder {
 
   std::vector<Expression*> expressionStack;
 
+  bool definitelyUnreachable; // set when we know code is definitely unreachable. this helps parse
+                              // stacky wasm code, which can be unsuitable for our IR when unreachable
+
   BinaryConsts::ASTNodes lastSeparator = BinaryConsts::End;
 
+  // process a block-type scope, until an end or else marker, or the end of the function
   void processExpressions();
+  void skipUnreachableCode();
+
   Expression* popExpression();
   Expression* popNonVoidExpression();
 

src/wasm/wasm-binary.cpp

@@ -1807,28 +1807,81 @@ void WasmBinaryBuilder::readGlobals() {
   }
 }
 
-void WasmBinaryBuilder::processExpressions() { // until an end or else marker, or the end of the function
+void WasmBinaryBuilder::processExpressions() {
+  if (debug) std::cerr << "== processExpressions" << std::endl;
+  definitelyUnreachable = false;
   while (1) {
     Expression* curr;
     auto ret = readExpression(curr);
     if (!curr) {
       lastSeparator = ret;
+      if (debug) std::cerr << "== processExpressions finished" << std::endl;
+      return;
+    }
+    expressionStack.push_back(curr);
+    if (curr->type == unreachable) {
+      // once we see something unreachable, we don't want to add anything else
+      // to the stack, as it could be stacky code that is non-representable in
+      // our AST. but we do need to skip it
+      // if there is nothing else here, just stop. otherwise, go into unreachable
+      // mode. peek to see what to do
+      if (pos == endOfFunction) {
+        throw ParseException("Reached function end without seeing End opcode");
+      }
+      auto peek = input[pos];
+      if (peek == BinaryConsts::End || peek == BinaryConsts::Else) {
+        if (debug) std::cerr << "== processExpressions finished with unreachable" << std::endl;
+        lastSeparator = BinaryConsts::ASTNodes(peek);
+        pos++;
+        return;
+      } else {
+        skipUnreachableCode();
+        return;
+      }
+    }
+  }
+}
+
+void WasmBinaryBuilder::skipUnreachableCode() {
+  if (debug) std::cerr << "== skipUnreachableCode" << std::endl;
+  // preserve the stack, and restore it. it contains the instruction that made us
+  // unreachable, and we can ignore anything after it. things after it may pop,
+  // we want to undo that
+  auto savedStack = expressionStack;
+  // clear the stack. nothing should be popped from there anyhow, just stuff
+  // can be pushed and then popped. Popping past the top of the stack will
+  // result in uneachables being returned
+  expressionStack.clear();
+  while (1) {
+    // set the definitelyUnreachable flag each time, as sub-blocks may set and unset it
+    definitelyUnreachable = true;
+    Expression* curr;
+    auto ret = readExpression(curr);
+    if (!curr) {
+      if (debug) std::cerr << "== skipUnreachableCode finished" << std::endl;
+      lastSeparator = ret;
+      definitelyUnreachable = false;
+      expressionStack = savedStack;
       return;
     }
     expressionStack.push_back(curr);
   }
 }
 
 Expression* WasmBinaryBuilder::popExpression() {
+  if (debug) std::cerr << "== popExpression" << std::endl;
   if (expressionStack.empty()) {
-    throw ParseException("attempted pop from empty stack at " + std::to_string(pos));
+    if (definitelyUnreachable) {
+      // in unreachable code, trying to pop past the polymorphic stack
+      // area results in receiving unreachables
+      if (debug) std::cerr << "== popping unreachable from polymorphic stack" << std::endl;
+      return allocator.alloc<Unreachable>();
+    }
+    throw ParseException("attempted pop from empty stack / beyond block start boundary at " + std::to_string(pos));
   }
+  // the stack is not empty, and we would not be going out of the current block
   auto ret = expressionStack.back();
-  // to simulate the wasm polymorphic stack mode, leave a final
-  // unreachable, don't empty the stack in that case
-  if (!(expressionStack.size() == 1 && ret->type == unreachable)) {
-    expressionStack.pop_back();
-  }
+  expressionStack.pop_back();
   return ret;
 }
 
@@ -2222,7 +2275,6 @@ void WasmBinaryBuilder::visitBreak(Break *curr, uint8_t code) {
 void WasmBinaryBuilder::visitSwitch(Switch *curr) {
   if (debug) std::cerr << "zz node: Switch" << std::endl;
   curr->condition = popNonVoidExpression();
-
   auto numTargets = getU32LEB();
   if (debug) std::cerr << "targets: "<< numTargets<<std::endl;
   for (size_t i = 0; i < numTargets; i++) {

test/example/c-api-unused-mem.txt

@@ -67,19 +67,15 @@
     )
     (block $label$2
      (br $label$1)
-     (unreachable)
     )
-    (unreachable)
    )
    (block $label$3
     (block $label$4
      (block $label$5
      )
      (block $label$6
       (br $label$4)
-      (unreachable)
      )
-     (unreachable)
     )
     (block $label$7
      (block $label$8
@@ -88,18 +84,11 @@
        (get_local $var$0)
       )
       (return)
-      (unreachable)
      )
      (unreachable)
-     (unreachable)
     )
-    (unreachable)
-    (unreachable)
    )
-   (unreachable)
-   (unreachable)
   )
-  (unreachable)
  )
  (func $__wasm_start (type $1)
   (block $label$0

test/fib-dbg.wasm.fromBinary

@@ -72,9 +72,7 @@
    (return
     (get_local $var$1)
    )
-   (unreachable)
   )
-  (unreachable)
  )
  (func $stackSave (type $2) (result i32)
   (return
@@ -214,10 +212,7 @@
    (return
     (get_local $var$4)
    )
-   (unreachable)
-   (unreachable)
   )
-  (unreachable)
  )
  (func $runPostSets (type $4)
   (local $var$0 i32)

test/min.wast.fromBinary

@@ -46,7 +46,6 @@
    (br $label$0
     (i32.const 2)
    )
-   (i32.const 0)
   )
  )
  (func $f1 (type $3) (param $var$0 i32) (param $var$1 i32) (param $var$2 i32) (result i32)

test/min.wast.fromBinary.noDebugInfo

@@ -46,7 +46,6 @@
    (br $label$0
     (i32.const 2)
    )
-   (i32.const 0)
   )
  )
  (func $3 (type $3) (param $var$0 i32) (param $var$1 i32) (param $var$2 i32) (result i32)

test/passes/flatten-control-flow.bin.txt

@@ -119,51 +119,7 @@
   (local $var$8 f64)
   (block $label$0
    (nop)
-   (block
-    (block
-     (unreachable)
-    )
-   )
-   (drop
-    (f32.neg
-     (get_local $var$1)
-    )
-   )
-   (drop
-    (f64.neg
-     (get_local $var$2)
-    )
-   )
-   (drop
-    (i32.eqz
-     (get_local $var$3)
-    )
-   )
-   (drop
-    (i32.eqz
-     (get_local $var$4)
-    )
-   )
-   (drop
-    (f32.neg
-     (get_local $var$7)
-    )
-   )
-   (drop
-    (i64.eqz
-     (get_local $var$5)
-    )
-   )
-   (drop
-    (i64.eqz
-     (get_local $var$6)
-    )
-   )
-   (drop
-    (f64.neg
-     (get_local $var$8)
-    )
-   )
+   (unreachable)
   )
  )
  (func $9 (type $9) (param $var$0 i64) (param $var$1 f32) (param $var$2 f64) (param $var$3 i32) (param $var$4 i32) (result f64)

test/polymorphic_stack.wast

@@ -86,6 +86,21 @@
       )
     )
   )
+  (func $unreachable-in-block-but-code-before (param $0 i32) (result i32)
+   (if
+    (get_local $0)
+    (return
+     (i32.const 127)
+    )
+   )
+   (block $label$0 (result i32)
+    (br_if $label$0
+     (return
+      (i32.const -32)
+     )
+    )
+   )
+  )
   (func $br_table_unreachable_to_also_unreachable (result i32)
     (block $a (result i32)
       (block $b

test/polymorphic_stack.wast.from-wast

@@ -90,6 +90,21 @@
    )
   )
  )
+ (func $unreachable-in-block-but-code-before (type $FUNCSIG$ii) (param $0 i32) (result i32)
+  (if
+   (get_local $0)
+   (return
+    (i32.const 127)
+   )
+  )
+  (block $label$0 (result i32)
+   (br_if $label$0
+    (return
+     (i32.const -32)
+    )
+   )
+  )
+ )
  (func $br_table_unreachable_to_also_unreachable (type $1) (result i32)
   (block $a (result i32)
    (block $b