[PATCH] D55797: [WebAssembly] Make assembler check for proper nesting of control flow.

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel marked 16 inline comments as done.
aardappel added a comment.

Added a test that tests the errors too.



================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:403
+      if (Pop(Block))
+        return true;
+    } else if (BaseName == "end_function") {
----------------
aheejin wrote:
> We have `end_try` too
Ok, added support for try/catch/catch_all/end_try


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:405
+    } else if (BaseName == "end_function") {
+      if (Pop(Function) || CheckFunctionEnd())
+        return true;
----------------
aheejin wrote:
> Did you mean to use `&&`? Not sure if `||` makes sense here 
in the parser, functions return true on failure.. which is a bit odd, yes.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:559
+        if (CheckFunctionEnd())
+          return true;
         CurrentState = FunctionStart;
----------------
aheejin wrote:
> Nit: Given that this is not a function end but rather a function start, how about renaming the function to something like `ensureEmptyControlFlowStack` or something and change the error message accordingly?
It is also possibly the end of the previous function (if `end_function` was forgotten. But sure, I'll change the name.


================
Comment at: test/MC/WebAssembly/simd-encodings.s:4
+main:
+    .functype main () -> ()
+
----------------
aheejin wrote:
> I guess this is not relevant to this CL?
It is.. it had an `end_function` without a start of a function


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55797/new/

https://reviews.llvm.org/D55797