Merge pull request #1095 from WebAssembly/fuzz-3

More fuzz fixes

Author: kripken

src/asm2wasm.h

@@ -1397,7 +1397,12 @@ void Asm2WasmBuilder::processAsm(Ref ast) {
     void visitCallIndirect(CallIndirect* curr) {
       // we already call into target = something + offset, where offset is a callImport with the name of the table. replace that with the table offset
       // note that for an ftCall or mftCall, we have no asm.js mask, so have nothing to do here
-      auto* add = curr->target->dynCast<Binary>();
+      auto* target = curr->target;
+      // might be a block with a fallthrough
+      if (auto* block = target->dynCast<Block>()) {
+        target = block->list.back();
+      }
+      auto* add = target->dynCast<Binary>();
       if (!add) return;
       if (add->right->is<CallImport>()) {
         auto* offset = add->right->cast<CallImport>();

src/ast/effects.h

@@ -39,7 +39,7 @@ struct EffectAnalyzer : public PostWalker<EffectAnalyzer> {
     if (breakNames.size() > 0) branches = true;
   }
 
-  bool branches = false; // branches out of this expression
+  bool branches = false; // branches out of this expression, returns, infinite loops, etc
   bool calls = false;
   std::set<Index> localsRead;
   std::set<Index> localsWritten;
@@ -138,6 +138,18 @@ struct EffectAnalyzer : public PostWalker<EffectAnalyzer> {
   }
   void visitLoop(Loop* curr) {
     if (curr->name.is()) breakNames.erase(curr->name); // these were internal breaks
+    // if the loop is unreachable, then there is branching control flow:
+    //  (1) if the body is unreachable because of a (return), uncaught (br) etc., then we
+    //      already noted branching, so it is ok to mark it again  (if we have *caught*
+    //      (br)s, then they did not lead to the loop body being unreachable).
+    //      (same logic applies to blocks)
+    //  (2) if the loop is unreachable because it only has branches up to the loop
+    //      top, but no way to get out, then it is an infinite loop, and we consider
+    //      that a branching side effect (note how the same logic does not apply to
+    //      blocks).
+    if (curr->type == unreachable) {
+      branches = true;
+    }
   }
 
   void visitCall(Call *curr) { calls = true; }
@@ -182,6 +194,7 @@ struct EffectAnalyzer : public PostWalker<EffectAnalyzer> {
         case TruncUFloat64ToInt32:
         case TruncUFloat64ToInt64: {
           implicitTrap = true;
+          break;
         }
         default: {}
       }
@@ -199,6 +212,7 @@ struct EffectAnalyzer : public PostWalker<EffectAnalyzer> {
         case RemSInt64:
         case RemUInt64: {
           implicitTrap = true;
+          break;
         }
         default: {}
       }

src/ast/properties.h

@@ -60,11 +60,13 @@ struct Properties {
     if (auto* outer = curr->dynCast<Binary>()) {
       if (outer->op == ShrSInt32) {
         if (auto* outerConst = outer->right->dynCast<Const>()) {
-          if (auto* inner = outer->left->dynCast<Binary>()) {
-            if (inner->op == ShlInt32) {
-              if (auto* innerConst = inner->right->dynCast<Const>()) {
-                if (outerConst->value == innerConst->value) {
-                  return inner->left;
+          if (outerConst->value.geti32() != 0) {
+            if (auto* inner = outer->left->dynCast<Binary>()) {
+              if (inner->op == ShlInt32) {
+                if (auto* innerConst = inner->right->dynCast<Const>()) {
+                  if (outerConst->value == innerConst->value) {
+                    return inner->left;
+                  }
                 }
               }
             }
@@ -87,11 +89,13 @@ struct Properties {
     if (auto* outer = curr->dynCast<Binary>()) {
       if (outer->op == ShrSInt32) {
         if (auto* outerConst = outer->right->dynCast<Const>()) {
-          if (auto* inner = outer->left->dynCast<Binary>()) {
-            if (inner->op == ShlInt32) {
-              if (auto* innerConst = inner->right->dynCast<Const>()) {
-                if (outerConst->value.leU(innerConst->value).geti32()) {
-                  return inner->left;
+          if (outerConst->value.geti32() != 0) {
+            if (auto* inner = outer->left->dynCast<Binary>()) {
+              if (inner->op == ShlInt32) {
+                if (auto* innerConst = inner->right->dynCast<Const>()) {
+                  if (outerConst->value.leU(innerConst->value).geti32()) {
+                    return inner->left;
+                  }
                 }
               }
             }

src/passes/CoalesceLocals.cpp

@@ -196,7 +196,9 @@ struct CoalesceLocals : public WalkerPass<CFGWalker<CoalesceLocals, Visitor<Coal
     if (auto* get = set->value->dynCast<GetLocal>()) return get;
     if (auto* iff = set->value->dynCast<If>()) {
       if (auto* get = iff->ifTrue->dynCast<GetLocal>()) return get;
-      if (auto* get = iff->ifFalse->dynCast<GetLocal>()) return get;
+      if (iff->ifFalse) {
+        if (auto* get = iff->ifFalse->dynCast<GetLocal>()) return get;
+      }
     }
     return nullptr;
   }
@@ -595,11 +597,13 @@ void CoalesceLocals::pickIndices(std::vector<Index>& indices) {
 // Remove a copy from a set of an if, where one if arm is a get of the same set
 static void removeIfCopy(Expression** origin, SetLocal* set, If* iff, Expression*& copy, Expression*& other, Module* module) {
   // replace the origin with the if, and sink the set into the other non-copying arm
+  bool tee = set->isTee();
   *origin = iff;
   set->value = other;
   set->finalize();
   other = set;
-  if (!isConcreteWasmType(set->type)) {
+  // if this is not a tee, then we can get rid of the copy in that arm
+  if (!tee) {
     // we don't need the copy at all
     copy = nullptr;
     if (!iff->ifTrue) {

src/passes/MergeBlocks.cpp

@@ -188,6 +188,15 @@ static void optimizeBlock(Block* curr, Module* module) {
       }
       if (!child) continue;
       if (child->name.is()) continue; // named blocks can have breaks to them (and certainly do, if we ran RemoveUnusedNames and RemoveUnusedBrs)
+      if (child->type == unreachable) {
+        // an unreachable block can have a concrete final element (which is never reached)
+        if (!child->list.empty()) {
+          if (isConcreteWasmType(child->list.back()->type)) {
+            // just remove it
+            child->list.pop_back();
+          }
+        }
+      }
       ExpressionList merged(module->allocator);
       for (size_t j = 0; j < i; j++) {
         merged.push_back(curr->list[j]);
@@ -279,11 +288,14 @@ struct MergeBlocks : public WalkerPass<PostWalker<MergeBlocks>> {
   }
 
   void visitSelect(Select* curr) {
+    // TODO: for now, just stop when we see any side effect. instead, we could
+    //       check effects carefully for reordering
     Block* outer = nullptr;
-    outer = optimize(curr, curr->ifTrue, outer);
     if (EffectAnalyzer(getPassOptions(), curr->ifTrue).hasSideEffects()) return;
-    outer = optimize(curr, curr->ifFalse, outer);
+    outer = optimize(curr, curr->ifTrue, outer);
     if (EffectAnalyzer(getPassOptions(), curr->ifFalse).hasSideEffects()) return;
+    outer = optimize(curr, curr->ifFalse, outer);
+    if (EffectAnalyzer(getPassOptions(), curr->condition).hasSideEffects()) return;
             optimize(curr, curr->condition, outer);
   }
 
@@ -299,11 +311,13 @@ struct MergeBlocks : public WalkerPass<PostWalker<MergeBlocks>> {
   }
 
   template<typename T>
-  void handleCall(T* curr, Block* outer = nullptr) {
+  void handleCall(T* curr) {
+    Block* outer = nullptr;
     for (Index i = 0; i < curr->operands.size(); i++) {
-      outer = optimize(curr, curr->operands[i], outer);
       if (EffectAnalyzer(getPassOptions(), curr->operands[i]).hasSideEffects()) return;
+      outer = optimize(curr, curr->operands[i], outer);
     }
+    return;
   }
 
   void visitCall(Call* curr) {
@@ -315,9 +329,13 @@ struct MergeBlocks : public WalkerPass<PostWalker<MergeBlocks>> {
   }
 
   void visitCallIndirect(CallIndirect* curr) {
-    auto* outer = optimize(curr, curr->target);
+    Block* outer = nullptr;
+    for (Index i = 0; i < curr->operands.size(); i++) {
+      if (EffectAnalyzer(getPassOptions(), curr->operands[i]).hasSideEffects()) return;
+      outer = optimize(curr, curr->operands[i], outer);
+    }
     if (EffectAnalyzer(getPassOptions(), curr->target).hasSideEffects()) return;
-    handleCall(curr, outer);
+    optimize(curr, curr->target, outer);
   }
 };
 

src/passes/OptimizeInstructions.cpp

@@ -517,6 +517,12 @@ struct OptimizeInstructions : public WalkerPass<PostWalker<OptimizeInstructions,
             }
           }
         }
+        // some operations have no effect TODO: many more
+        if (right->value == Literal(int32_t(0))) {
+          if (binary->op == ShlInt32 || binary->op == ShrUInt32 || binary->op == ShrSInt32) {
+            return binary->left;
+          }
+        }
         // the square of some operations can be merged
         if (auto* left = binary->left->dynCast<Binary>()) {
           if (left->op == binary->op) {

src/passes/Vacuum.cpp

@@ -232,6 +232,16 @@ struct Vacuum : public WalkerPass<PostWalker<Vacuum>> {
       replaceCurrent(child);
       return;
     }
+    // if the condition is unreachable, just return it
+    if (curr->condition->type == unreachable) {
+      typeUpdater.noteRecursiveRemoval(curr->ifTrue);
+      if (curr->ifFalse) {
+        typeUpdater.noteRecursiveRemoval(curr->ifFalse);
+      }
+      replaceCurrent(curr->condition);
+      return;
+    }
+    // from here on, we can assume the condition executed
     if (curr->ifFalse) {
       if (curr->ifFalse->is<Nop>()) {
         curr->ifFalse = nullptr;

src/wasm-validator.h

@@ -220,7 +220,7 @@ struct WasmValidator : public PostWalker<WasmValidator> {
   void shouldBeIntOrUnreachable(WasmType ty, Expression* curr, const char* text);
   void validateAlignment(size_t align, WasmType type, Index bytes, bool isAtomic,
                          Expression* curr);
-  void validateMemBytes(uint8_t bytes, WasmType ty, Expression* curr);
+  void validateMemBytes(uint8_t bytes, WasmType type, Expression* curr);
   void validateBinaryenIR(Module& wasm);
 };
 

src/wasm/wasm-validator.cpp

@@ -261,14 +261,17 @@ void WasmValidator::visitAtomicCmpxchg(AtomicCmpxchg* curr) {
   shouldBeEqualOrFirstIsUnreachable(curr->replacement->type, curr->type, curr, "Cmpxchg result type must match replacement");
   shouldBeIntOrUnreachable(curr->expected->type, curr, "Atomic operations are only valid on int types");
 }
-void WasmValidator::validateMemBytes(uint8_t bytes, WasmType ty, Expression* curr) {
+void WasmValidator::validateMemBytes(uint8_t bytes, WasmType type, Expression* curr) {
+  if (type == unreachable) {
+    return; // nothing to validate in this case
+  }
   switch (bytes) {
     case 1:
     case 2:
     case 4:
       break;
     case 8: {
-      shouldBeEqual(getWasmTypeSize(ty), 8U, curr, "8-byte mem operations are only allowed with 8-byte wasm types");
+      shouldBeEqual(getWasmTypeSize(type), 8U, curr, "8-byte mem operations are only allowed with 8-byte wasm types");
       break;
     }
     default: fail("Memory operations must be 1,2,4, or 8 bytes", curr);

test/passes/coalesce-locals.txt

@@ -7,6 +7,7 @@
  (type $FUNCSIG$i (func (result i32)))
  (type $FUNCSIG$vi (func (param i32)))
  (type $7 (func (param i32) (result i32)))
+ (type $8 (func (param f64 i32) (result i64)))
  (import "env" "_emscripten_autodebug_i32" (func $_emscripten_autodebug_i32 (param i32 i32) (result i32)))
  (import "env" "get" (func $get (result i32)))
  (import "env" "set" (func $set (param i32)))
@@ -1112,4 +1113,44 @@
   )
   (i32.const 1)
  )
+ (func $unused-tee-with-child-if-no-else (type $4) (param $0 i32)
+  (loop $label$0
+   (drop
+    (if
+     (br $label$0)
+     (nop)
+    )
+   )
+  )
+ )
+ (func $tee_if_with_unreachable_else (type $8) (param $0 f64) (param $1 i32) (result i64)
+  (call $tee_if_with_unreachable_else
+   (if (result f64)
+    (get_local $1)
+    (get_local $0)
+    (tee_local $0
+     (unreachable)
+    )
+   )
+   (f64.lt
+    (f64.const -128)
+    (get_local $0)
+   )
+  )
+ )
+ (func $tee_if_with_unreachable_true (type $8) (param $0 f64) (param $1 i32) (result i64)
+  (call $tee_if_with_unreachable_else
+   (if (result f64)
+    (get_local $1)
+    (tee_local $0
+     (unreachable)
+    )
+    (get_local $0)
+   )
+   (f64.lt
+    (f64.const -128)
+    (get_local $0)
+   )
+  )
+ )
 )