This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Made InstPrinter more robust
ClosedPublic

Authored by aardappel on Jan 2 2019, 1:53 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

aardappel created this revision.Jan 2 2019, 1:53 PM
  • 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 updated this revision to Diff 180123.Jan 3 2019, 12:25 PM
aardappel marked 4 inline comments as done.

Added test.

aardappel added inline comments.Jan 3 2019, 12:26 PM
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 added inline comments.Jan 3 2019, 2:05 PM
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 marked an inline comment as done.Jan 3 2019, 2:12 PM
aardappel added inline comments.
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.

aheejin accepted this revision.Jan 3 2019, 2:30 PM
This revision is now accepted and ready to land.Jan 3 2019, 2:30 PM
This revision was automatically updated to reflect the committed changes.