[WebAssembly] Made InstPrinter more robust

[aardappel]

Summary

Instead of asserting on certain kinds of malformed instructions, it
now still print, but instead adds an annotation indicating the
problem, and/or indicates invalid_type etc.

We're using the InstPrinter from many contexts that can't always
guarantee values are within range (e.g. the disassembler), where having
output is more valueable than asserting.

[aardappel]

Rebased.

[aheejin]

• Can we have a test case that shows the warning messages? Maybe adding a small separate file would be better than adding erroneous lines to basic-assembly.s
• In case you forgot, please clang-format

lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp

304 ↗

(On Diff #179951)

So does this print "invalid_type" even when we have an invalid instruction within itself?

lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.h

59 ↗

(On Diff #179951)

Function names should be camelCase

60 ↗

(On Diff #179951)

Function names should be camelCase

[aardappel]

Added test.

[aardappel]

lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp

304 ↗

(On Diff #179951)

I'm not sure what you mean. It currently prints this only when an instruction explicitly tries to print an operand that is a block_type or other signature, and it happens to be an unknown value.

[aheejin]

lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp

304 ↗

(On Diff #179951)

Oh what I meant was.. I was not sure if printing instructions like

block invalid_type

was ok.

br 32 # invalid depth argument!

seemed ok because even if it's out of context, br 32 itself is a valid instruction, and we are giving hints that this instruction does not make sense within the current context in the comment anyway. But block invalid_type is not an instruction and cannot be parsed. So I think in this case it deserves an assertion, but I don't feel very strongly about it. What do you think?

[aardappel]

lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp

304 ↗

(On Diff #179951)

Well, there's no way to print that block instruction with a valid type. I could print block void # Invalid type! instead but that would be highly confusing. I don't think there should be an assert here, no, as per my original motivation for this commit.

llvm/trunk/lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.h

public:
 
   // Autogenerated by tblgen.
   void printInstruction(const MCInst *MI, raw_ostream &O);
   static const char *getRegisterName(unsigned RegNo);
 };
 
 namespace WebAssembly {
 
-const char *TypeToString(wasm::ValType Ty);
+const char *typeToString(wasm::ValType Ty);
+const char *anyTypeToString(unsigned Ty);
 
 } // end namespace WebAssembly
 
 } // end namespace llvm
 
 #endif

llvm/trunk/lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp

if (CommentStream) {
     case WebAssembly::TRY_S:
       ControlFlowStack.push_back(std::make_pair(ControlFlowCounter++, false));
       EHPadStack.push_back(EHPadStackCounter++);
       LastSeenEHInst = TRY;
       break;
 
     case WebAssembly::END_LOOP:
     case WebAssembly::END_LOOP_S:
-      assert(!ControlFlowStack.empty() && "End marker mismatch!");
+      if (ControlFlowStack.empty()) {
+        printAnnotation(OS, "End marker mismatch!");
+      } else {
         ControlFlowStack.pop_back();
+      }
       break;
 
     case WebAssembly::END_BLOCK:
     case WebAssembly::END_BLOCK_S:
-      assert(!ControlFlowStack.empty() && "End marker mismatch!");
+      if (ControlFlowStack.empty()) {
+        printAnnotation(OS, "End marker mismatch!");
+      } else {
         printAnnotation(
             OS, "label" + utostr(ControlFlowStack.pop_back_val().first) + ':');
+      }
       break;
 
     case WebAssembly::END_TRY:
     case WebAssembly::END_TRY_S:
-      assert(!ControlFlowStack.empty() && "End marker mismatch!");
+      if (ControlFlowStack.empty()) {
+        printAnnotation(OS, "End marker mismatch!");
+      } else {
         printAnnotation(
             OS, "label" + utostr(ControlFlowStack.pop_back_val().first) + ':');
         LastSeenEHInst = END_TRY;
+      }
       break;
 
     case WebAssembly::CATCH_I32:
     case WebAssembly::CATCH_I32_S:
     case WebAssembly::CATCH_I64:
     case WebAssembly::CATCH_I64_S:
     case WebAssembly::CATCH_ALL:
     case WebAssembly::CATCH_ALL_S:
       // There can be multiple catch instructions for one try instruction, so we
       // print a label only for the first 'catch' label.
       if (LastSeenEHInst != CATCH) {
-        assert(!EHPadStack.empty() && "try-catch mismatch!");
-        printAnnotation(OS, "catch" + utostr(EHPadStack.pop_back_val()) + ':');
+        if (EHPadStack.empty()) {
+          printAnnotation(OS, "try-catch mismatch!");
+        } else {
+          printAnnotation(OS,
+                          "catch" + utostr(EHPadStack.pop_back_val()) + ':');
+        }
       }
       LastSeenEHInst = CATCH;
       break;
     }
 
     // Annotate any control flow label references.
     unsigned NumFixedOperands = Desc.NumOperands;
     SmallSet<uint64_t, 8> Printed;
for (unsigned i = 0, e = MI->getNumOperands(); i < e; ++i) {
         if (!MI->getOperand(i).isImm())
           continue;
       }
       uint64_t Depth = MI->getOperand(i).getImm();
       if (!Printed.insert(Depth).second)
         continue;
 
       if (Opc == WebAssembly::RETHROW || Opc == WebAssembly::RETHROW_S) {
-        assert(Depth <= EHPadStack.size() && "Invalid depth argument!");
-        if (Depth == EHPadStack.size()) {
+        if (Depth > EHPadStack.size()) {
+          printAnnotation(OS, "Invalid depth argument!");
+        } else if (Depth == EHPadStack.size()) {
           // This can happen when rethrow instruction breaks out of all nests
           // and throws up to the current function's caller.
           printAnnotation(OS, utostr(Depth) + ": " + "to caller");
         } else {
           uint64_t CatchNo = EHPadStack.rbegin()[Depth];
           printAnnotation(OS, utostr(Depth) + ": " + "down to catch" +
                                   utostr(CatchNo));
         }
 
       } else {
-        assert(Depth < ControlFlowStack.size() && "Invalid depth argument!");
+        if (Depth >= ControlFlowStack.size()) {
+          printAnnotation(OS, "Invalid depth argument!");
+        } else {
           const auto &Pair = ControlFlowStack.rbegin()[Depth];
           printAnnotation(OS, utostr(Depth) + ": " +
                                   (Pair.second ? "up" : "down") + " to label" +
                                   utostr(Pair.first));
         }
       }
     }
   }
+}
 
 static std::string toString(const APFloat &FP) {
   // Print NaNs with custom payloads specially.
   if (FP.isNaN() && !FP.bitwiseIsEqual(APFloat::getQNaN(FP.getSemantics())) &&
       !FP.bitwiseIsEqual(
           APFloat::getQNaN(FP.getSemantics(), /*Negative=*/true))) {
     APInt AI = FP.bitcastToAPInt();
     return std::string(AI.isNegative() ? "-" : "") + "nan:0x" +
void WebAssemblyInstPrinter::printWebAssemblyP2AlignOperand(const MCInst *MI,
   if (Imm == WebAssembly::GetDefaultP2Align(MI->getOpcode()))
     return;
   O << ":p2align=" << Imm;
 }
 
 void WebAssemblyInstPrinter::printWebAssemblySignatureOperand(const MCInst *MI,
                                                               unsigned OpNo,
                                                               raw_ostream &O) {
-  int64_t Imm = MI->getOperand(OpNo).getImm();
-  switch (WebAssembly::ExprType(Imm)) {
-  case WebAssembly::ExprType::Void:
-    break;
-  case WebAssembly::ExprType::I32:
-    O << "i32";
-    break;
-  case WebAssembly::ExprType::I64:
-    O << "i64";
-    break;
-  case WebAssembly::ExprType::F32:
-    O << "f32";
-    break;
-  case WebAssembly::ExprType::F64:
-    O << "f64";
-    break;
-  case WebAssembly::ExprType::V128:
-    O << "v128";
-    break;
-  case WebAssembly::ExprType::ExceptRef:
-    O << "except_ref";
-    break;
-  default:
-    llvm_unreachable("invalid WebAssembly::ExprType");
-  }
+  auto Imm = static_cast<unsigned>(MI->getOperand(OpNo).getImm());
+  if (Imm != wasm::WASM_TYPE_NORESULT)
+    O << WebAssembly::anyTypeToString(Imm);
 }
 
-const char *llvm::WebAssembly::TypeToString(wasm::ValType Ty) {
+// We have various enums representing a subset of these types, use this
+// function to convert any of them to text.
+const char *llvm::WebAssembly::anyTypeToString(unsigned Ty) {
   switch (Ty) {
-  case wasm::ValType::I32:
+  case wasm::WASM_TYPE_I32:
     return "i32";
-  case wasm::ValType::I64:
+  case wasm::WASM_TYPE_I64:
     return "i64";
-  case wasm::ValType::F32:
+  case wasm::WASM_TYPE_F32:
     return "f32";
-  case wasm::ValType::F64:
+  case wasm::WASM_TYPE_F64:
     return "f64";
-  case wasm::ValType::V128:
+  case wasm::WASM_TYPE_V128:
     return "v128";
-  case wasm::ValType::EXCEPT_REF:
+  case wasm::WASM_TYPE_ANYFUNC:
+    return "anyfunc";
+  case wasm::WASM_TYPE_FUNC:
+    return "func";
+  case wasm::WASM_TYPE_EXCEPT_REF:
     return "except_ref";
+  case wasm::WASM_TYPE_NORESULT:
+    return "void";
+  default:
+    return "invalid_type";
   }
-  llvm_unreachable("Unknown wasm::ValType");
+}
+
+const char *llvm::WebAssembly::typeToString(wasm::ValType Ty) {
+  return anyTypeToString(static_cast<unsigned>(Ty));
 }

llvm/trunk/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp

 static void printTypes(formatted_raw_ostream &OS,
                        ArrayRef<wasm::ValType> Types) {
   bool First = true;
   for (auto Type : Types) {
     if (First)
       First = false;
     else
       OS << ", ";
-    OS << WebAssembly::TypeToString(Type);
+    OS << WebAssembly::typeToString(Type);
   }
   OS << '\n';
 }
 
 void WebAssemblyTargetAsmStreamer::emitLocal(ArrayRef<wasm::ValType> Types) {
   if (!Types.empty()) {
     OS << "\t.local  \t";
     printTypes(OS, Types);

 }
 
 void WebAssemblyTargetAsmStreamer::emitParamList(
     const wasm::WasmSignature *Sig) {
   auto &Params = Sig->Params;
   for (auto &Ty : Params) {
     if (&Ty != &Params[0])
       OS << ", ";
-    OS << WebAssembly::TypeToString(Ty);
+    OS << WebAssembly::typeToString(Ty);
   }
 }
 
 void WebAssemblyTargetAsmStreamer::emitReturnList(
     const wasm::WasmSignature *Sig) {
   auto &Returns = Sig->Returns;
   for (auto &Ty : Returns) {
     if (&Ty != &Returns[0])
       OS << ", ";
-    OS << WebAssembly::TypeToString(Ty);
+    OS << WebAssembly::typeToString(Ty);
   }
 }
 
 void WebAssemblyTargetAsmStreamer::emitFunctionType(const MCSymbolWasm *Sym) {
   assert(Sym->isFunction());
   OS << "\t.functype\t" << Sym->getName() << " ";
   emitSignature(Sym->getSignature());
   OS << "\n";
 }
 
 void WebAssemblyTargetAsmStreamer::emitGlobalType(const MCSymbolWasm *Sym) {
   assert(Sym->isGlobal());
-  OS << "\t.globaltype\t" << Sym->getName() << ", " <<
-        WebAssembly::TypeToString(
-          static_cast<wasm::ValType>(Sym->getGlobalType().Type)) <<
-        '\n';
+  OS << "\t.globaltype\t" << Sym->getName() << ", "
+     << WebAssembly::typeToString(
+            static_cast<wasm::ValType>(Sym->getGlobalType().Type))
+     << '\n';
 }
 
 void WebAssemblyTargetAsmStreamer::emitEventType(const MCSymbolWasm *Sym) {
   assert(Sym->isEvent());
   OS << "\t.eventtype\t" << Sym->getName() << " ";
   emitParamList(Sym->getSignature());
   OS << "\n";
 }

llvm/trunk/test/MC/Disassembler/WebAssembly/wasm-error.txt

+# RUN: llvm-mc --disassemble %s -triple=wasm32-unknown-unknown | FileCheck %s
+
+# CHECK: .text
+
+# CHECK: block invalid_type
+0x02 0x00
+
+# CHECK: br 16 # Invalid depth argument!
+0x0C 0x10