This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Read prefixed opcodes as ULEB128s
ClosedPublic

Authored by tlively on Nov 5 2018, 8:20 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

tlively created this revision.Nov 5 2018, 8:20 PM
tlively updated this revision to Diff 172785.Nov 6 2018, 10:07 AM
  • Replace magic table size value with constant
tlively updated this revision to Diff 172786.Nov 6 2018, 10:08 AM
  • Remove accidentally included old commits
tlively updated this revision to Diff 172787.Nov 6 2018, 10:09 AM
  • Fix commits once and for all
Harbormaster completed remote builds in B24617: Diff 172787.
aheejin added inline comments.Nov 6 2018, 5:01 PM
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 updated this revision to Diff 173117.Nov 7 2018, 8:24 PM
  • Refactor!
tlively marked an inline comment as done.Nov 7 2018, 8:25 PM
aheejin accepted this revision.Nov 8 2018, 3:44 PM
This revision is now accepted and ready to land.Nov 8 2018, 3:44 PM
This revision was automatically updated to reflect the committed changes.
aardappel added inline comments.Nov 9 2018, 9:27 AM
llvm/trunk/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp
141

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

tlively added inline comments.Nov 9 2018, 11:05 AM
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 added a subscriber: MTC.Nov 12 2018, 4:34 AM
MTC added inline comments.
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 added inline comments.Nov 13 2018, 1:19 PM
llvm/trunk/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp
69

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

aheejin added a comment.EditedNov 15 2018, 2:06 AM

Someone reported the same warning here.

tlively added inline comments.Nov 15 2018, 11:03 AM
llvm/trunk/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp
69

This should be fixed in rL346979.