[WebAssembly] Error out when block/loop markers mismatch

[aheejin]

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.

[dschuff]

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

[aheejin]

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]

• Add llvm-mc test case (and delete the old one)
• Change report_fatal_error to assertions

[aheejin]

• Tidy up test case

[aheejin]

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?

[sbc100]

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]

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

[aheejin]

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?

[dschuff]

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.

[aardappel]

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.

[aardappel]

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]

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]

Yes, for now this is certainly fine.

[aheejin]

• Delete test case. Assertions can't be tested.

[aheejin]

Thanks!

lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp

case WebAssembly::LOOP_S: {
       break;
     }
     case WebAssembly::BLOCK:
     case WebAssembly::BLOCK_S:
       ControlFlowStack.push_back(std::make_pair(ControlFlowCounter++, false));
       break;
     case WebAssembly::END_LOOP:
     case WebAssembly::END_LOOP_S:
-      // Have to guard against an empty stack, in case of mismatched pairs
-      // in assembly parsing.
-      if (!ControlFlowStack.empty())
-        ControlFlowStack.pop_back();
+      assert(!ControlFlowStack.empty() && "End marker mismatch!");
       break;
     case WebAssembly::END_BLOCK:
     case WebAssembly::END_BLOCK_S:
-      if (!ControlFlowStack.empty())
-        printAnnotation(
-            OS, "label" + utostr(ControlFlowStack.pop_back_val().first) + ':');
+      assert(!ControlFlowStack.empty() && "End marker mismatch!");
       break;
     }
 
     // Annotate any control flow label references.
     unsigned NumFixedOperands = Desc.NumOperands;
     SmallSet<uint64_t, 8> Printed;
     for (unsigned i = 0, e = MI->getNumOperands(); i < e; ++i) {
       if (!(i < NumFixedOperands

test/MC/WebAssembly/block-mismatch.s

+# RUN: not llvm-mc -triple=wasm32-unknown-unknown %s -o - 2>&1 | FileCheck %s
+
+# This tests if there are block/loop marker mismatches, the program crashes.
+  .text
+  .type  test0,@function
+test0:
+  block
+  end_block
+  # CHECK: End marker mismatch!
+  end_block
+  end_function