[WebAssembly] Allow AsmTypeCheck detect multiple errors in function (#109705)

This allows multiple errors to be reported within a function, rather
than returning on the first error and not looking at the rest of the
function.

I think the rationale for the previous behavior was that upon
encountering the first error, the value stack was not in the correct
status anymore and the rest of the function checking was not very
meaningful. But this patch makes the instruction push the correct result
type upon its completion, so the while the possibility of previous error
affecting later instructions is not zero, I think this can be more
helpful to assembly hand-writers. This also allows us to write multiple
error test cases without creating as many functions.

This is what Wabt and Binaryen wast checker/validator do as well.

Also this makes sure we return a value (true/false) within an `if` for
each instruction, removing the need for the long `if`-`else if`-`else
if` chain and making them all just `if`s. I also added newlines between
the `if`s, which I feel is easier to read.

2 files changed, 192 insertions, 158 deletions

diff --git a/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp b/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp
index 4ff19a9..8b1e1dc 100644
--- a/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp
+++ b/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp
@@ -70,14 +70,9 @@ void WebAssemblyAsmTypeCheck::dumpTypeStack(Twine Msg) {
 }
 
 bool WebAssemblyAsmTypeCheck::typeError(SMLoc ErrorLoc, const Twine &Msg) {
-  // Once you get one type error in a function, it will likely trigger more
-  // which are mostly not helpful.
-  if (TypeErrorThisFunction)
-    return true;
   // If we're currently in unreachable code, we suppress errors completely.
   if (Unreachable)
     return false;
-  TypeErrorThisFunction = true;
   dumpTypeStack("current stack: ");
   return Parser.Error(ErrorLoc, Msg);
 }
@@ -171,11 +166,11 @@ bool WebAssemblyAsmTypeCheck::checkEnd(SMLoc ErrorLoc, bool PopVals) {
 
 bool WebAssemblyAsmTypeCheck::checkSig(SMLoc ErrorLoc,
                                        const wasm::WasmSignature &Sig) {
+  bool Error = false;
   for (auto VT : llvm::reverse(Sig.Params))
-    if (popType(ErrorLoc, VT))
-      return true;
+    Error |= popType(ErrorLoc, VT);
   Stack.insert(Stack.end(), Sig.Returns.begin(), Sig.Returns.end());
-  return false;
+  return Error;
 }
 
 bool WebAssemblyAsmTypeCheck::getSymRef(SMLoc ErrorLoc, const MCOperand &SymOp,
@@ -260,17 +255,16 @@ bool WebAssemblyAsmTypeCheck::getSignature(SMLoc ErrorLoc,
 }
 
 bool WebAssemblyAsmTypeCheck::endOfFunction(SMLoc ErrorLoc) {
+  bool Error = false;
   // Check the return types.
-  for (auto RVT : llvm::reverse(ReturnTypes)) {
-    if (popType(ErrorLoc, RVT))
-      return true;
-  }
+  for (auto RVT : llvm::reverse(ReturnTypes))
+    Error |= popType(ErrorLoc, RVT);
   if (!Stack.empty()) {
     return typeError(ErrorLoc, std::to_string(Stack.size()) +
                                    " superfluous return values");
   }
   Unreachable = true;
-  return false;
+  return Error;
 }
 
 bool WebAssemblyAsmTypeCheck::typeCheck(SMLoc ErrorLoc, const MCInst &Inst,
@@ -279,179 +273,221 @@ bool WebAssemblyAsmTypeCheck::typeCheck(SMLoc ErrorLoc, const MCInst &Inst,
   auto Name = getMnemonic(Opc);
   dumpTypeStack("typechecking " + Name + ": ");
   wasm::ValType Type;
+
   if (Name == "local.get") {
-    if (getLocal(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
-      return true;
-    Stack.push_back(Type);
-  } else if (Name == "local.set") {
-    if (getLocal(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
-      return true;
-    if (popType(ErrorLoc, Type))
-      return true;
-  } else if (Name == "local.tee") {
-    if (getLocal(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
-      return true;
-    if (popType(ErrorLoc, Type))
-      return true;
-    Stack.push_back(Type);
-  } else if (Name == "global.get") {
-    if (getGlobal(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
-      return true;
-    Stack.push_back(Type);
-  } else if (Name == "global.set") {
-    if (getGlobal(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
-      return true;
-    if (popType(ErrorLoc, Type))
-      return true;
-  } else if (Name == "table.get") {
-    if (getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
-      return true;
-    if (popType(ErrorLoc, wasm::ValType::I32))
-      return true;
-    Stack.push_back(Type);
-  } else if (Name == "table.set") {
-    if (getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
-      return true;
-    if (popType(ErrorLoc, Type))
-      return true;
-    if (popType(ErrorLoc, wasm::ValType::I32))
-      return true;
-  } else if (Name == "table.size") {
-    if (getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
-      return true;
+    if (!getLocal(Operands[1]->getStartLoc(), Inst.getOperand(0), Type)) {
+      Stack.push_back(Type);
+      return false;
+    }
+    return true;
+  }
+
+  if (Name == "local.set") {
+    if (!getLocal(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
+      return popType(ErrorLoc, Type);
+    return true;
+  }
+
+  if (Name == "local.tee") {
+    if (!getLocal(Operands[1]->getStartLoc(), Inst.getOperand(0), Type)) {
+      bool Error = popType(ErrorLoc, Type);
+      Stack.push_back(Type);
+      return Error;
+    }
+    return true;
+  }
+
+  if (Name == "global.get") {
+    if (!getGlobal(Operands[1]->getStartLoc(), Inst.getOperand(0), Type)) {
+      Stack.push_back(Type);
+      return false;
+    }
+    return true;
+  }
+
+  if (Name == "global.set") {
+    if (!getGlobal(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
+      return popType(ErrorLoc, Type);
+    return true;
+  }
+
+  if (Name == "table.get") {
+    bool Error = popType(ErrorLoc, wasm::ValType::I32);
+    if (!getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type)) {
+      Stack.push_back(Type);
+      return Error;
+    }
+    return true;
+  }
+
+  if (Name == "table.set") {
+    bool Error = false;
+    if (!getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
+      Error |= popType(ErrorLoc, Type);
+    else
+      Error = true;
+    Error |= popType(ErrorLoc, wasm::ValType::I32);
+    return Error;
+  }
+
+  if (Name == "table.size") {
+    bool Error = getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type);
     Stack.push_back(wasm::ValType::I32);
-  } else if (Name == "table.grow") {
-    if (getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
-      return true;
-    if (popType(ErrorLoc, wasm::ValType::I32))
-      return true;
-    if (popType(ErrorLoc, Type))
-      return true;
+    return Error;
+  }
+
+  if (Name == "table.grow") {
+    bool Error = popType(ErrorLoc, wasm::ValType::I32);
+    if (!getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
+      Error |= popType(ErrorLoc, Type);
+    else
+      Error = true;
     Stack.push_back(wasm::ValType::I32);
-  } else if (Name == "table.fill") {
-    if (getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
-      return true;
-    if (popType(ErrorLoc, wasm::ValType::I32))
-      return true;
-    if (popType(ErrorLoc, Type))
-      return true;
-    if (popType(ErrorLoc, wasm::ValType::I32))
-      return true;
-  } else if (Name == "memory.fill") {
+    return Error;
+  }
+
+  if (Name == "table.fill") {
+    bool Error = popType(ErrorLoc, wasm::ValType::I32);
+    if (!getTable(Operands[1]->getStartLoc(), Inst.getOperand(0), Type))
+      Error |= popType(ErrorLoc, Type);
+    else
+      Error = true;
+    Error |= popType(ErrorLoc, wasm::ValType::I32);
+    return Error;
+  }
+
+  if (Name == "memory.fill") {
     Type = Is64 ? wasm::ValType::I64 : wasm::ValType::I32;
-    if (popType(ErrorLoc, Type))
-      return true;
-    if (popType(ErrorLoc, wasm::ValType::I32))
-      return true;
-    if (popType(ErrorLoc, Type))
-      return true;
-  } else if (Name == "memory.copy") {
+    bool Error = popType(ErrorLoc, Type);
+    Error |= popType(ErrorLoc, wasm::ValType::I32);
+    Error |= popType(ErrorLoc, Type);
+    return Error;
+  }
+
+  if (Name == "memory.copy") {
     Type = Is64 ? wasm::ValType::I64 : wasm::ValType::I32;
-    if (popType(ErrorLoc, Type))
-      return true;
-    if (popType(ErrorLoc, Type))
-      return true;
-    if (popType(ErrorLoc, Type))
-      return true;
-  } else if (Name == "memory.init") {
+    bool Error = popType(ErrorLoc, Type);
+    Error |= popType(ErrorLoc, Type);
+    Error |= popType(ErrorLoc, Type);
+    return Error;
+  }
+
+  if (Name == "memory.init") {
     Type = Is64 ? wasm::ValType::I64 : wasm::ValType::I32;
-    if (popType(ErrorLoc, wasm::ValType::I32))
-      return true;
-    if (popType(ErrorLoc, wasm::ValType::I32))
-      return true;
-    if (popType(ErrorLoc, Type))
-      return true;
-  } else if (Name == "drop") {
-    if (popType(ErrorLoc, {}))
-      return true;
-  } else if (Name == "try" || Name == "block" || Name == "loop" ||
-             Name == "if") {
-    if (Name == "if" && popType(ErrorLoc, wasm::ValType::I32))
-      return true;
+    bool Error = popType(ErrorLoc, wasm::ValType::I32);
+    Error |= popType(ErrorLoc, wasm::ValType::I32);
+    Error |= popType(ErrorLoc, Type);
+    return Error;
+  }
+
+  if (Name == "drop") {
+    return popType(ErrorLoc, {});
+  }
+
+  if (Name == "try" || Name == "block" || Name == "loop" || Name == "if") {
     if (Name == "loop")
       BrStack.emplace_back(LastSig.Params.begin(), LastSig.Params.end());
     else
       BrStack.emplace_back(LastSig.Returns.begin(), LastSig.Returns.end());
-  } else if (Name == "end_block" || Name == "end_loop" || Name == "end_if" ||
-             Name == "else" || Name == "end_try" || Name == "catch" ||
-             Name == "catch_all" || Name == "delegate") {
-    if (checkEnd(ErrorLoc,
-                 Name == "else" || Name == "catch" || Name == "catch_all"))
+    if (Name == "if" && popType(ErrorLoc, wasm::ValType::I32))
       return true;
+    return false;
+  }
+
+  if (Name == "end_block" || Name == "end_loop" || Name == "end_if" ||
+      Name == "else" || Name == "end_try" || Name == "catch" ||
+      Name == "catch_all" || Name == "delegate") {
+    bool Error = checkEnd(ErrorLoc, Name == "else" || Name == "catch" ||
+                                        Name == "catch_all");
     Unreachable = false;
     if (Name == "catch") {
       const wasm::WasmSignature *Sig = nullptr;
-      if (getSignature(Operands[1]->getStartLoc(), Inst.getOperand(0),
-                       wasm::WASM_SYMBOL_TYPE_TAG, Sig))
-        return true;
-      // catch instruction pushes values whose types are specified in the tag's
-      // "params" part
-      Stack.insert(Stack.end(), Sig->Params.begin(), Sig->Params.end());
+      if (!getSignature(Operands[1]->getStartLoc(), Inst.getOperand(0),
+                        wasm::WASM_SYMBOL_TYPE_TAG, Sig))
+        // catch instruction pushes values whose types are specified in the
+        // tag's "params" part
+        Stack.insert(Stack.end(), Sig->Params.begin(), Sig->Params.end());
+      else
+        Error = true;
     }
-  } else if (Name == "br") {
+    return Error;
+  }
+
+  if (Name == "br") {
     const MCOperand &Operand = Inst.getOperand(0);
     if (!Operand.isImm())
-      return false;
-    if (checkBr(ErrorLoc, static_cast<size_t>(Operand.getImm())))
-      return true;
-  } else if (Name == "return") {
-    if (endOfFunction(ErrorLoc))
       return true;
-  } else if (Name == "call_indirect" || Name == "return_call_indirect") {
+    return checkBr(ErrorLoc, static_cast<size_t>(Operand.getImm()));
+  }
+
+  if (Name == "return") {
+    return endOfFunction(ErrorLoc);
+  }
+
+  if (Name == "call_indirect" || Name == "return_call_indirect") {
     // Function value.
-    if (popType(ErrorLoc, wasm::ValType::I32))
-      return true;
-    if (checkSig(ErrorLoc, LastSig))
-      return true;
+    bool Error = popType(ErrorLoc, wasm::ValType::I32);
+    Error |= checkSig(ErrorLoc, LastSig);
     if (Name == "return_call_indirect" && endOfFunction(ErrorLoc))
       return true;
-  } else if (Name == "call" || Name == "return_call") {
+    return Error;
+  }
+
+  if (Name == "call" || Name == "return_call") {
+    bool Error = false;
     const wasm::WasmSignature *Sig = nullptr;
-    if (getSignature(Operands[1]->getStartLoc(), Inst.getOperand(0),
-                     wasm::WASM_SYMBOL_TYPE_FUNCTION, Sig))
-      return true;
-    if (checkSig(ErrorLoc, *Sig))
-      return true;
+    if (!getSignature(Operands[1]->getStartLoc(), Inst.getOperand(0),
+                      wasm::WASM_SYMBOL_TYPE_FUNCTION, Sig))
+      Error |= checkSig(ErrorLoc, *Sig);
+    else
+      Error = true;
     if (Name == "return_call" && endOfFunction(ErrorLoc))
       return true;
-  } else if (Name == "unreachable") {
+    return Error;
+  }
+
+  if (Name == "unreachable") {
     Unreachable = true;
-  } else if (Name == "ref.is_null") {
-    if (popRefType(ErrorLoc))
-      return true;
+    return false;
+  }
+
+  if (Name == "ref.is_null") {
+    bool Error = popRefType(ErrorLoc);
     Stack.push_back(wasm::ValType::I32);
-  } else if (Name == "throw") {
+    return Error;
+  }
+
+  if (Name == "throw") {
     const wasm::WasmSignature *Sig = nullptr;
-    if (getSignature(Operands[1]->getStartLoc(), Inst.getOperand(0),
-                     wasm::WASM_SYMBOL_TYPE_TAG, Sig))
-      return true;
-    if (checkSig(ErrorLoc, *Sig))
-      return true;
-  } else {
-    // The current instruction is a stack instruction which doesn't have
-    // explicit operands that indicate push/pop types, so we get those from
-    // the register version of the same instruction.
-    auto RegOpc = WebAssembly::getRegisterOpcode(Opc);
-    assert(RegOpc != -1 && "Failed to get register version of MC instruction");
-    const auto &II = MII.get(RegOpc);
-    // First pop all the uses off the stack and check them.
-    for (unsigned I = II.getNumOperands(); I > II.getNumDefs(); I--) {
-      const auto &Op = II.operands()[I - 1];
-      if (Op.OperandType == MCOI::OPERAND_REGISTER) {
-        auto VT = WebAssembly::regClassToValType(Op.RegClass);
-        if (popType(ErrorLoc, VT))
-          return true;
-      }
-    }
-    // Now push all the defs onto the stack.
-    for (unsigned I = 0; I < II.getNumDefs(); I++) {
-      const auto &Op = II.operands()[I];
-      assert(Op.OperandType == MCOI::OPERAND_REGISTER && "Register expected");
+    if (!getSignature(Operands[1]->getStartLoc(), Inst.getOperand(0),
+                      wasm::WASM_SYMBOL_TYPE_TAG, Sig))
+      return checkSig(ErrorLoc, *Sig);
+    return true;
+  }
+
+  // The current instruction is a stack instruction which doesn't have
+  // explicit operands that indicate push/pop types, so we get those from
+  // the register version of the same instruction.
+  auto RegOpc = WebAssembly::getRegisterOpcode(Opc);
+  assert(RegOpc != -1 && "Failed to get register version of MC instruction");
+  const auto &II = MII.get(RegOpc);
+  bool Error = false;
+  // First pop all the uses off the stack and check them.
+  for (unsigned I = II.getNumOperands(); I > II.getNumDefs(); I--) {
+    const auto &Op = II.operands()[I - 1];
+    if (Op.OperandType == MCOI::OPERAND_REGISTER) {
       auto VT = WebAssembly::regClassToValType(Op.RegClass);
-      Stack.push_back(VT);
+      Error |= popType(ErrorLoc, VT);
     }
   }
-  return false;
+  // Now push all the defs onto the stack.
+  for (unsigned I = 0; I < II.getNumDefs(); I++) {
+    const auto &Op = II.operands()[I];
+    assert(Op.OperandType == MCOI::OPERAND_REGISTER && "Register expected");
+    auto VT = WebAssembly::regClassToValType(Op.RegClass);
+    Stack.push_back(VT);
+  }
+  return Error;
 }
 
 } // end namespace llvm
diff --git a/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.h b/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.h
index 2a654d79..972162d 100644
--- a/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.h
+++ b/llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.h
@@ -33,7 +33,6 @@ class WebAssemblyAsmTypeCheck final {
   SmallVector<wasm::ValType, 16> LocalTypes;
   SmallVector<wasm::ValType, 4> ReturnTypes;
   wasm::WasmSignature LastSig;
-  bool TypeErrorThisFunction = false;
   bool Unreachable = false;
   bool Is64;
 
@@ -68,7 +67,6 @@ public:
     BrStack.clear();
     LocalTypes.clear();
     ReturnTypes.clear();
-    TypeErrorThisFunction = false;
     Unreachable = false;
   }
 };