Validate finalization (#1014)

* validate that types are properly finalized, when in pass-debug mode (BINARYEN_PASS_DEBUG env var): check after each pass is run that the type of each node is equal to the proper type (when finalizing it, i.e., fully recomputing the type).

* fix many fuzz bugs found by that.

* in particular, fix dce bugs with type changes not being fully updated during code removal. add a new TypeUpdater helper class that lets a pass update types efficiently, by the helper tracking deps between blocks and branches etc., and updating/propagating type changes only as necessary.

Author: kripken

check.py

@@ -125,16 +125,29 @@
           cmd += ['--wasm-only']
         wasm = os.path.join(options.binaryen_test, wasm)
         print '..', asm, wasm
-        actual = run_command(cmd)
 
-        # verify output
-        if not os.path.exists(wasm):
-          fail_with_error('output .wast file %s does not exist' % wasm)
-        expected = open(wasm, 'rb').read()
-        if actual != expected:
-          fail(actual, expected)
-
-        binary_format_check(wasm, verify_final_result=False)
+        def do_asm2wasm_test():
+          actual = run_command(cmd)
+
+          # verify output
+          if not os.path.exists(wasm):
+            fail_with_error('output .wast file %s does not exist' % wasm)
+          expected = open(wasm, 'rb').read()
+          if actual != expected:
+            fail(actual, expected)
+
+          binary_format_check(wasm, verify_final_result=False)
+
+        # test both normally and with pass debug (so each inter-pass state is validated)
+        old_pass_debug = os.environ.get('BINARYEN_PASS_DEBUG')
+        try:
+          os.environ['BINARYEN_PASS_DEBUG'] = '1'
+          do_asm2wasm_test()
+          del os.environ['BINARYEN_PASS_DEBUG']
+          do_asm2wasm_test()
+        finally:
+          if old_pass_debug is not None:
+            os.environ['BINARYEN_PASS_DEBUG'] = old_pass_debug
 
         # verify in wasm
         if options.interpreter:
@@ -280,6 +293,11 @@ def run_spec_test(wast):
       cmd = cmd + (extra.get(os.path.basename(wast)) or [])
       return run_command(cmd, stderr=subprocess.PIPE)
 
+    def run_opt_test(wast):
+      # check optimization validation
+      cmd = WASM_OPT + [wast, '-O']
+      run_command(cmd)
+
     def check_expected(actual, expected):
       if expected and os.path.exists(expected):
         expected = open(expected).read()
@@ -335,6 +353,7 @@ def fix(x):
         split_num += 1
         with open('split.wast', 'w') as o: o.write(module + '\n' + '\n'.join(asserts))
         run_spec_test('split.wast') # before binary stuff - just check it's still ok split out
+        run_opt_test('split.wast') # also that our optimizer doesn't break on it
         result_wast = binary_format_check('split.wast', verify_final_result=False)
         # add the asserts, and verify that the test still passes
         open(result_wast, 'a').write('\n' + '\n'.join(asserts))

src/asm2wasm.h

@@ -35,7 +35,6 @@
 #include "wasm-builder.h"
 #include "wasm-emscripten.h"
 #include "wasm-printing.h"
-#include "wasm-validator.h"
 #include "wasm-module-building.h"
 
 namespace wasm {
@@ -1339,6 +1338,12 @@ void Asm2WasmBuilder::processAsm(Ref ast) {
         add->left = parent->builder.makeConst(Literal((int32_t)parent->functionTableStarts[tableName]));
       }
     }
+
+    void visitFunction(Function* curr) {
+      // changing call types requires we percolate types, and drop stuff.
+      // we do this in this pass so that we don't look broken between passes
+      AutoDrop().walkFunctionInModule(curr, getModule());
+    }
   };
 
   // apply debug info, reducing intrinsic calls into annotations on the ast nodes
@@ -1400,9 +1405,9 @@ void Asm2WasmBuilder::processAsm(Ref ast) {
     passRunner.setDebug(true);
     passRunner.setValidateGlobally(false);
   }
+  // finalizeCalls also does autoDrop, which is crucial for the non-optimizing case,
+  // so that the output of the first pass is valid
   passRunner.add<FinalizeCalls>(this);
-  passRunner.add<ReFinalize>(); // FinalizeCalls changes call types, need to percolate
-  passRunner.add<AutoDrop>(); // FinalizeCalls may cause us to require additional drops
   if (legalizeJavaScriptFFI) {
     passRunner.add("legalize-js-interface");
   }
@@ -1551,8 +1556,6 @@ void Asm2WasmBuilder::processAsm(Ref ast) {
     body->finalize();
     func->body = body;
   }
-
-  assert(WasmValidator().validate(wasm));
 }
 
 Function* Asm2WasmBuilder::processFunction(Ref ast) {
@@ -2489,7 +2492,6 @@ Function* Asm2WasmBuilder::processFunction(Ref ast) {
         }
 
         top->list.push_back(br);
-        top->finalize();
 
         for (unsigned i = 0; i < cases->size(); i++) {
           Ref curr = cases[i];
@@ -2564,7 +2566,6 @@ Function* Asm2WasmBuilder::processFunction(Ref ast) {
           top->name = name;
           next->list.push_back(top);
           next->list.push_back(case_);
-          next->finalize();
           top = next;
           nameMapper.popLabelName(name);
         }
@@ -2580,7 +2581,6 @@ Function* Asm2WasmBuilder::processFunction(Ref ast) {
         first->ifFalse = builder.makeBreak(br->default_);
 
         brHolder->list.push_back(chain);
-        brHolder->finalize();
       }
 
       breakStack.pop_back();

src/ast/ExpressionManipulator.cpp

@@ -147,6 +147,7 @@ void ExpressionManipulator::spliceIntoBlock(Block* block, Index index, Expressio
     }
     list[index] = add;
   }
+  block->finalize(block->type);
 }
 
 } // namespace wasm

src/ast/block-utils.h

@@ -0,0 +1,66 @@
+/*
+ * Copyright 2017 WebAssembly Community Group participants
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef wasm_ast_block_h
+#define wasm_ast_block_h
+
+#include "literal.h"
+#include "wasm.h"
+#include "ast_utils.h"
+
+namespace wasm {
+
+namespace BlockUtils {
+  // if a block has just one element, it can often be replaced
+  // with that content
+  template<typename T>
+  inline Expression* simplifyToContents(Block* block, T* parent, bool allowTypeChange = false) {
+    auto& list = block->list;
+    if (list.size() == 1 && !BreakSeeker::has(list[0], block->name)) {
+      // just one element. try to replace the block
+      auto* singleton = list[0];
+      auto sideEffects = EffectAnalyzer(parent->getPassOptions(), singleton).hasSideEffects();
+      if (!sideEffects && !isConcreteWasmType(singleton->type)) {
+        // no side effects, and singleton is not returning a value, so we can throw away
+        // the block and its contents, basically
+        return Builder(*parent->getModule()).replaceWithIdenticalType(block);
+      } else if (block->type == singleton->type || allowTypeChange) {
+        return singleton;
+      } else {
+        // (side effects +) type change, must be block with declared value but inside is unreachable
+        // (if both concrete, must match, and since no name on block, we can't be
+        // branched to, so if singleton is unreachable, so is the block)
+        assert(isConcreteWasmType(block->type) && singleton->type == unreachable);
+        // we could replace with unreachable, but would need to update all
+        // the parent's types
+      }
+    } else if (list.size() == 0) {
+      ExpressionManipulator::nop(block);
+    }
+    return block;
+  }
+
+  // similar, but when we allow the type to change while doing so
+  template<typename T>
+  inline Expression* simplifyToContentsWithPossibleTypeChange(Block* block, T* parent) {
+    return simplifyToContents(block, parent, true);
+  }
+};
+
+} // namespace wasm
+
+#endif // wasm_ast_block_h
+

src/ast/manipulation.h

@@ -0,0 +1,69 @@
+/*
+ * Copyright 2017 WebAssembly Community Group participants
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef wasm_ast_manipulation_h
+#define wasm_ast_manipulation_h
+
+#include "wasm.h"
+
+namespace wasm {
+
+struct ExpressionManipulator {
+  // Re-use a node's memory. This helps avoid allocation when optimizing.
+  template<typename InputType, typename OutputType>
+  static OutputType* convert(InputType *input) {
+    static_assert(sizeof(OutputType) <= sizeof(InputType),
+                  "Can only convert to a smaller size Expression node");
+    input->~InputType(); // arena-allocaed, so no destructor, but avoid UB.
+    OutputType* output = (OutputType*)(input);
+    new (output) OutputType;
+    return output;
+  }
+
+  // Convenience method for nop, which is a common conversion
+  template<typename InputType>
+  static Nop* nop(InputType* target) {
+    return convert<InputType, Nop>(target);
+  }
+
+  // Convert a node that allocates
+  template<typename InputType, typename OutputType>
+  static OutputType* convert(InputType *input, MixedArena& allocator) {
+    assert(sizeof(OutputType) <= sizeof(InputType));
+    input->~InputType(); // arena-allocaed, so no destructor, but avoid UB.
+    OutputType* output = (OutputType*)(input);
+    new (output) OutputType(allocator);
+    return output;
+  }
+
+  using CustomCopier = std::function<Expression*(Expression*)>;
+  static Expression* flexibleCopy(Expression* original, Module& wasm, CustomCopier custom);
+
+  static Expression* copy(Expression* original, Module& wasm) {
+    auto copy = [](Expression* curr) {
+        return nullptr;
+    };
+    return flexibleCopy(original, wasm, copy);
+  }
+
+  // Splice an item into the middle of a block's list
+  static void spliceIntoBlock(Block* block, Index index, Expression* add);
+};
+
+} // wasm
+
+#endif // wams_ast_manipulation_h
+