refactor and improve break validation. breaks names are unique, so we don't need a stack, and break targets must exist even if they are not actually taken

Author: kripken

src/ast/branch-utils.h

@@ -37,6 +37,15 @@ inline bool isBranchTaken(Switch* sw) {
                         sw->condition->type != unreachable;
 }
 
+inline bool isBranchTaken(Expression* expr) {
+  if (auto* br = expr->dynCast<Break>()) {
+    return isBranchTaken(br);
+  } else if (auto* sw = expr->dynCast<Switch>()) {
+    return isBranchTaken(sw);
+  }
+  WASM_UNREACHABLE();
+}
+
 // returns the set of targets to which we branch that are
 // outside of a node
 inline std::set<Name> getExitingBranches(Expression* ast) {

src/binaryen-c.cpp

@@ -30,6 +30,7 @@
 #include "wasm-s-parser.h"
 #include "wasm-validator.h"
 #include "cfg/Relooper.h"
+#include "ast_utils.h"
 #include "shell-interface.h"
 
 using namespace wasm;

src/wasm-validator.h

@@ -58,8 +58,9 @@ struct WasmValidator : public PostWalker<WasmValidator> {
     BreakInfo(WasmType type, Index arity) : type(type), arity(arity) {}
   };
 
-  std::map<Name, std::vector<Expression*>> breakTargets; // more than one block/loop may use a label name, so stack them
+  std::map<Name, Expression*> breakTargets;
   std::map<Expression*, BreakInfo> breakInfos;
+  std::set<Name> namedBreakTargets; // even breaks not taken must not be named if they go to a place that does not exist
 
   WasmType returnType = unreachable; // type used in returns
 
@@ -91,14 +92,14 @@ struct WasmValidator : public PostWalker<WasmValidator> {
 
   static void visitPreBlock(WasmValidator* self, Expression** currp) {
     auto* curr = (*currp)->cast<Block>();
-    if (curr->name.is()) self->breakTargets[curr->name].push_back(curr);
+    if (curr->name.is()) self->breakTargets[curr->name] = curr;
   }
 
   void visitBlock(Block *curr);
 
   static void visitPreLoop(WasmValidator* self, Expression** currp) {
     auto* curr = (*currp)->cast<Loop>();
-    if (curr->name.is()) self->breakTargets[curr->name].push_back(curr);
+    if (curr->name.is()) self->breakTargets[curr->name] = curr;
   }
 
   void visitLoop(Loop *curr);

src/wasm/wasm-validator.cpp

@@ -57,7 +57,8 @@ void WasmValidator::visitBlock(Block *curr) {
         }
       }
     }
-    breakTargets[curr->name].pop_back();
+    breakTargets.erase(curr->name);
+    namedBreakTargets.erase(curr->name);
   }
   if (curr->list.size() > 1) {
     for (Index i = 0; i < curr->list.size() - 1; i++) {
@@ -88,7 +89,8 @@ void WasmValidator::visitBlock(Block *curr) {
 void WasmValidator::visitLoop(Loop *curr) {
   if (curr->name.is()) {
     noteLabelName(curr->name);
-    breakTargets[curr->name].pop_back();
+    breakTargets.erase(curr->name);
+    namedBreakTargets.erase(curr->name);
     if (breakInfos.count(curr) > 0) {
       auto& info = breakInfos[curr];
       shouldBeEqual(info.arity, Index(0), curr, "breaks to a loop cannot pass a value");
@@ -120,15 +122,20 @@ void WasmValidator::visitIf(If *curr) {
 }
 
 void WasmValidator::noteBreak(Name name, Expression* value, Expression* curr) {
+  if (!BranchUtils::isBranchTaken(curr)) {
+    // if not actually taken, just note the name
+    namedBreakTargets.insert(name);
+    return;
+  }
   WasmType valueType = none;
   Index arity = 0;
   if (value) {
     valueType = value->type;
     shouldBeUnequal(valueType, none, curr, "breaks must have a valid value");
     arity = 1;
   }
-  if (!shouldBeTrue(breakTargets[name].size() > 0, curr, "all break targets must be valid")) return;
-  auto* target = breakTargets[name].back();
+  if (!shouldBeTrue(breakTargets.count(name) > 0, curr, "all break targets must be valid")) return;
+  auto* target = breakTargets[name];
   if (breakInfos.count(target) == 0) {
     breakInfos[target] = BreakInfo(valueType, arity);
   } else {
@@ -146,23 +153,17 @@ void WasmValidator::noteBreak(Name name, Expression* value, Expression* curr) {
   }
 }
 void WasmValidator::visitBreak(Break *curr) {
-  // note breaks (that are actually taken)
-  if (BranchUtils::isBranchTaken(curr)) {
-    noteBreak(curr->name, curr->value, curr);
-  }
+  noteBreak(curr->name, curr->value, curr);
   if (curr->condition) {
     shouldBeTrue(curr->condition->type == unreachable || curr->condition->type == i32, curr, "break condition must be i32");
   }
 }
 
 void WasmValidator::visitSwitch(Switch *curr) {
-  // note breaks (that are actually taken)
-  if (BranchUtils::isBranchTaken(curr)) {
-    for (auto& target : curr->targets) {
-      noteBreak(target, curr->value, curr);
-    }
-    noteBreak(curr->default_, curr->value, curr);
+  for (auto& target : curr->targets) {
+    noteBreak(target, curr->value, curr);
   }
+  noteBreak(curr->default_, curr->value, curr);
   shouldBeTrue(curr->condition->type == unreachable || curr->condition->type == i32, curr, "br_table condition must be i32");
 }
 void WasmValidator::visitCall(Call *curr) {
@@ -467,7 +468,7 @@ void WasmValidator::visitGlobal(Global* curr) {
   shouldBeTrue(curr->init != nullptr, curr->name, "global init must be non-null");
   shouldBeTrue(curr->init->is<Const>() || curr->init->is<GetGlobal>(), curr->name, "global init must be valid");
   if (!shouldBeEqual(curr->type, curr->init->type, curr->init, "global init must have correct type")) {
-    std::cerr << "(on global " << curr->name << '\n';
+    std::cerr << "(on global " << curr->name << ")\n";
   }
 }
 
@@ -480,6 +481,9 @@ void WasmValidator::visitFunction(Function *curr) {
   if (returnType != unreachable) {
     shouldBeEqual(curr->result, returnType, curr->body, "function result must match, if function has returns");
   }
+  if (!shouldBeTrue(namedBreakTargets.empty(), curr->body, "all named break targets must exist (even if not taken)")) {
+    std::cerr << "(on label " << *namedBreakTargets.begin() << ")\n";
+  }
   returnType = unreachable;
   labelNames.clear();
 }