Page MenuHomePhabricator

[WebAssembly] Read prefixed opcodes as ULEB128s

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

Diff Detail


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

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

tlively added inline comments.Nov 9 2018, 11:05 AM

This is true currently, which led me to open 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.

@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

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

This should be fixed in rL346979.