[WebAssembly] Read prefixed opcodes as ULEB128s

[tlively]

Summary

Depends on D54126.

[tlively]

• Replace magic table size value with constant

[tlively]

• Remove accidentally included old commits

[tlively]

• Fix commits once and for all

[aheejin]

lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp

135 ↗

(On Diff #172787)

I'm not too familiar with this disassembler part of the code, but the types with which Opc seem a bit confusing. So Opc is first declared above in

auto Opc = nextByte(Bytes, Size);

It is auto, but I guess because nextByte reutrns int, that's gonna be an int type.

And nextByte takes uint8_t array but returns an int. Now the same variable Opc is used to store the return type of decodeULEB128 whose return type is uint64_t. I know I'm talking about both something pre-existing and things added in this CL, but do you think it would make more sense to change the return type of nextByte to uint8_t too and split the two Opcs, and maybe not make them auto?

[tlively]

• Refactor!

[aardappel]

llvm/trunk/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp

141

wait.. the extension opcode is a LEB, yet has max 256 possible values?

[tlively]

llvm/trunk/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp

141

This is true currently, which led me to open https://github.com/WebAssembly/simd/issues/46. However, in the future we might have more than 265 SIMD opcodes, in which case we could just bump WebAssemblyInstructionTableSize in the TableGen backend and have everything just work. This check just makes sure that bad input cannot cause us to read past the end of the disassembler tables.

[MTC]

llvm/trunk/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp

69

@tlively Just to remind, change int to uint8_t will trigger the following warning, see line 123.

warning: comparison is always false due to limited range of data type [-Wtype-limits]
if (Opc < 0)
           ^

[tlively]

llvm/trunk/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp

69

Thanks for the heads up! I'll fix this soon.

[aheejin]

Someone reported the same warning here.

[tlively]

llvm/trunk/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp

69

This should be fixed in rL346979.

llvm/trunk/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp

 extern "C" void LLVMInitializeWebAssemblyDisassembler() {
   // Register the disassembler for each target.
   TargetRegistry::RegisterMCDisassembler(getTheWebAssemblyTarget32(),
                                          createWebAssemblyDisassembler);
   TargetRegistry::RegisterMCDisassembler(getTheWebAssemblyTarget64(),
                                          createWebAssemblyDisassembler);
 }
 
-static int nextByte(ArrayRef<uint8_t> Bytes, uint64_t &Size) {
+static uint8_t nextByte(ArrayRef<uint8_t> Bytes, uint64_t &Size) {
   if (Size >= Bytes.size())
     return -1;
   auto V = Bytes[Size];
   Size++;
   return V;
 }
 
-static bool parseLEBImmediate(MCInst &MI, uint64_t &Size,
-                              ArrayRef<uint8_t> Bytes, bool Signed) {
+static bool nextLEB(int64_t &Val, ArrayRef<uint8_t> Bytes, uint64_t &Size,
+                    bool Signed = false) {
   unsigned N = 0;
   const char *Error = nullptr;
-  auto Val = Signed ? decodeSLEB128(Bytes.data() + Size, &N,
-                                    Bytes.data() + Bytes.size(), &Error)
-                    : static_cast<int64_t>(
-                          decodeULEB128(Bytes.data() + Size, &N,
-                                        Bytes.data() + Bytes.size(), &Error));
+  Val = Signed ? decodeSLEB128(Bytes.data() + Size, &N,
+               : static_cast<int64_t>(decodeULEB128(Bytes.data() + Size, &N,
+                                                    Bytes.data() + Bytes.size(),
+                                                    &Error));
   if (Error)
     return false;
   Size += N;
+  return true;
+}
+
+static bool parseLEBImmediate(MCInst &MI, uint64_t &Size,
+                              ArrayRef<uint8_t> Bytes, bool Signed) {
+  int64_t Val;
+  if (!nextLEB(Val, Bytes, Size, Signed))
+    return false;
   MI.addOperand(MCOperand::createImm(Val));
   return true;
 }
 
 template <typename T>
 bool parseImmediate(MCInst &MI, uint64_t &Size, ArrayRef<uint8_t> Bytes) {
   if (Size + sizeof(T) > Bytes.size())
     return false;
if (WasmInst->ET == ET_Prefix) {
     for (auto PT = PrefixTable; PT->Table; PT++) {
       if (PT->Prefix == Opc) {
         WasmInst = PT->Table;
         break;
       }
     }
     if (!WasmInst)
       return MCDisassembler::Fail;
-    Opc = nextByte(Bytes, Size);
-    if (Opc < 0)
+    int64_t PrefixedOpc;
+    if (!nextLEB(PrefixedOpc, Bytes, Size))
+      return MCDisassembler::Fail;
+    if (PrefixedOpc < 0 || PrefixedOpc >= WebAssemblyInstructionTableSize)
       return MCDisassembler::Fail;
-    WasmInst += Opc;
+    WasmInst += PrefixedOpc;
   }
   if (WasmInst->ET == ET_Unused)
     return MCDisassembler::Fail;
   // At this point we must have a valid instruction to decode.
   assert(WasmInst->ET == ET_Instruction);
   MI.setOpcode(WasmInst->Opcode);
   // Parse any operands.
   for (uint8_t OPI = 0; OPI < WasmInst->NumOperands; OPI++) {

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

 0xFC 0x07
 
 # v128.const is arbitrarily disassembled as v16i8
 # CHECK: v128.const 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15
 0xFD 0x02 0x00 0x01 0x02 0x03 0x04 0x05 0x06 0x07 0x08 0x09 0x0A 0x0B 0x0C 0x0D 0x0E 0x0F
 
 # CHECK: v8x16.shuffle 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15
 0xFD 0x03 0x00 0x01 0x02 0x03 0x04 0x05 0x06 0x07 0x08 0x09 0x0A 0x0B 0x0C 0x0D 0x0E 0x0F
+
+# Check LEB128 encoding of SIMD instructions
+# CHECK: i64x2.all_true
+0xFD 0x86 0x01
+
+# Including non-canonical LEB128 encodings
+# CHECK: i64x2.any_true
+# CHECK-NOT: i64.div_u
+0xFD 0x85 0x81 0x80 0x80 0x80 0x80 0x00

llvm/trunk/utils/TableGen/WebAssemblyDisassemblerEmitter.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "WebAssemblyDisassemblerEmitter.h"
 #include "llvm/TableGen/Record.h"
 
 namespace llvm {
 
+static constexpr int WebAssemblyInstructionTableSize = 256;
+
 void emitWebAssemblyDisassemblerTables(
     raw_ostream &OS,
     const ArrayRef<const CodeGenInstruction *> &NumberedInstructions) {
   // First lets organize all opcodes by (prefix) byte. Prefix 0 is the
   // starting table.
   std::map<unsigned,
            std::map<unsigned, std::pair<unsigned, const CodeGenInstruction *>>>
       OpcodeTable;
if (IsStackBased && !CGIP.second) {
       // currently the except_ref one, though this shouldn't matter for
       // disassembly purposes.
       CGIP = std::make_pair(I, &CGI);
     }
   }
   OS << "#include \"MCTargetDesc/WebAssemblyMCTargetDesc.h\"\n";
   OS << "\n";
   OS << "namespace llvm {\n\n";
+  OS << "static constexpr int WebAssemblyInstructionTableSize = ";
+  OS << WebAssemblyInstructionTableSize << ";\n\n";
   OS << "enum EntryType : uint8_t { ";
   OS << "ET_Unused, ET_Prefix, ET_Instruction };\n\n";
   OS << "struct WebAssemblyInstruction {\n";
   OS << "  uint16_t Opcode;\n";
   OS << "  EntryType ET;\n";
   OS << "  uint8_t NumOperands;\n";
   OS << "  uint16_t OperandStart;\n";
   OS << "};\n\n";
   std::vector<std::string> OperandTable, CurOperandList;
   // Output one table per prefix.
   for (auto &PrefixPair : OpcodeTable) {
     if (PrefixPair.second.empty())
       continue;
     OS << "WebAssemblyInstruction InstructionTable" << PrefixPair.first;
     OS << "[] = {\n";
-    for (unsigned I = 0; I <= 0xFF; I++) {
+    for (unsigned I = 0; I < WebAssemblyInstructionTableSize; I++) {
       auto InstIt = PrefixPair.second.find(I);
       if (InstIt != PrefixPair.second.end()) {
         // Regular instruction.
         assert(InstIt->second.second);
         auto &CGI = *InstIt->second.second;
         OS << "  // 0x";
         OS.write_hex(static_cast<unsigned long long>(I));
         OS << ": " << CGI.AsmString << "\n";