[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–136

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 ↗

(On Diff #173256)

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

[tlively]

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

141 ↗

(On Diff #173256)

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 ↗

(On Diff #173256)

@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 ↗

(On Diff #173256)

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 ↗

(On Diff #173256)

This should be fixed in rL346979.

lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp

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)
+    unsigned N = 0;
+    const char *Error = nullptr;
+    Opc = decodeULEB128(Bytes.data() + Size, &N, Bytes.data() + Bytes.size(),
+                        &Error);
+    if (Error || Opc < 0 || Opc > 255)
       return MCDisassembler::Fail;
+    Size += N;
     WasmInst += Opc;
   }
   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.

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 0x8C 0x01
+
+# Including non-canonical LEB128 encodings
+# CHECK: i64x2.any_true
+# CHECK-NOT: i64.div_u
+0xFD 0x8B 0x81 0x80 0x80 0x80 0x80 0x00