Page MenuHomePhabricator

[WebAssembly] Error out when block/loop markers mismatch
ClosedPublic

Authored by aheejin on Oct 23 2018, 3:37 PM.

Details

Summary

Currently InstPrinter ignores if there are mismatches between block/loop
and end markers by skipping the case if ControlFlowStack is empty. I
guess it is better to explicitly error out in this case, because this
signals invalid input.

Diff Detail

Repository
rL LLVM

Event Timeline

aheejin created this revision.Oct 23 2018, 3:37 PM

This kind of code should never be produced by the wasm backend, right? In which case this could be an assert/unreachable rather than a fatal error. But I guess this code also prints insts that are parsed from an obj or asm file, right? In that case the input could just be bogus and we shouldn't assert.
So I'm not sure if it's possible here to tell which one to ideally use.
But also:

  1. can this test be an llvm-mc test (which has assembly input and prints assembly)? I think the answer is "yes" (at least eventually) but I'm not sure if the asm parser is far enough along yet for that.
  2. I wonder if there's some kind of infrastructure in MC for printing errors (e.g. for malformed asm) other than just the big hammer of report_fatal_error? If so it probably makes sense to use that (even if we can't tell the difference between bogus code from the compiler and bogus input from an asm user).

This kind of code should never be produced by the wasm backend, right. I made these not assertions but fatal errors because I thought there can be cases when we do only parsing and printing, such as in llvm-mc. But on a second thought, maybe in that case the errors should rather be detected not in InstPrinter but in AsmParser. AsmParser seems to provide better error handling, in which every method returns Error object and such. So I think these are better be assertions, and we rely on AsmParser on the correct error handling. What do you think? But anyway, in either case I don't think we should silently ignore these errors as the current code does.

And yes, I will give a try on llvm-mc test. Thanks.

aheejin updated this revision to Diff 170841.Oct 24 2018, 3:55 AM
  • Add llvm-mc test case (and delete the old one)
  • Change report_fatal_error to assertions
aheejin updated this revision to Diff 170842.Oct 24 2018, 3:56 AM
  • Tidy up test case

Hmm strange. The test currently fails after I switch to assertions. If we use report_fatal_error, the test runs fine. Not sure why.

-- Testing: 1 tests, 1 threads --
FAIL: LLVM :: MC/WebAssembly/block-mismatch.s (1 of 1)
******************** TEST 'LLVM :: MC/WebAssembly/block-mismatch.s' FAILED ********************
Script:
--
: 'RUN: at line 1';   not /usr/local/google/home/aheejin/llvm-git/build.debug/bin/llvm-mc -triple=wasm32-unknown-unknown /usr/local/google/home/aheejin/llvm-git/llvm/test/MC/WebAssembly/block-mismatch.s -o - 2>&1 | /usr/local/google/home/aheejin/llvm-git/build.debug/bin/FileCheck /usr/local/google/home/aheejin/llvm-git/llvm/test/MC/WebAssembly/block-mismatch.s
--
Exit Code: 1

But If I run that command manually, it succeeds. I think the reason might be the way assert fails is different from just returning non-zero exit code (assert _aborts_). Any ideas?

It doesn't seem like a good idea to rely on the string in the assert. Perhaps adding an error to the asm parser and checking for that would make more sense?

Having said that I don't know why the assert approach wouldn't pass the test assuming assertions are enabled.

dschuff added a comment.EditedOct 24 2018, 8:51 AM

I think catching block mismatch errors at parse time makes sense; although the third way that MCInst can be generated (aside from the compiler and asmparser) is the object file parser. So those checks would have to go both in the asm parser and object file parser. But if that means that we can hook into some existing error handling mechanism instead of using report_fatal_error, then it's probably worth it. (in that case we'd probably want to have asserts here too).
I agree that we can't use an assert string in a CHECK. The test wouldn't pass in a release non-asserts build (our release binaries have assertions, but it's not the default, and the official release binaries don't).

I agree using existing error handling mechanisms in AsmParser (and obj parser too) is better, but I don't think that should be included in this CL to be landed. Do assertions for this CL look fine? But if I use assertions, I don't seem to be able to add a test case. Can I commit that without the test?

I think assertions here are OK as long as the plan of record is is to put error handling into the parsers. If we have assertions, we can't have a test case (until we can test the parser errors) because it would always fail on release builds without asserts.

If the only path to this assert would be through code (from our backend), I would agree that asserts are the right thing. But if a user can create a .s file and assemble it with llvm-mc and then print, this should result in an error, or succeed without asserts. That, or we can guarantee in other paths that this can never happen, by making the assembler check for this, then an assert is ok again.

There are builds where assert becomes nop, so messages in there are not sufficient.

Oh, feel free to assign a bug to me to add CF nesting checking to the assembler, if you think this is the right way forward. Sounds like something we should have anyway?

aheejin added a comment.EditedOct 25 2018, 2:54 PM

Even llvm-mc has to parse something (asm or obj file) before printing, so I think it makes sense to do the error checking on the parser. And also it seems methods in AsmParser return Error token to signal an error has occurred during parsing, so it seems to provide more infrastructure as well. I'll file a bug on that, thanks.

Then can we land this as is?

aardappel accepted this revision.Oct 25 2018, 3:06 PM

Yes, for now this is certainly fine.

This revision is now accepted and ready to land.Oct 25 2018, 3:06 PM
aheejin updated this revision to Diff 171213.EditedOct 25 2018, 4:11 PM
  • Delete test case. Assertions can't be tested.
This revision was automatically updated to reflect the committed changes.