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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

FYI you can check error message of an invalid test case by appending `not` in the `RUN` line and redirect the stderr to stdout, like this test <https://github.com/llvm-mirror/llvm/blob/master/test/ObjectYAML/wasm/invalid_section_order.yaml>.



================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:183
+    Undefined,
+  };
+  std::vector<NestingType> NestingStack;
----------------
We have `Try` now too


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:230
+    case Else:
+      return " else";
+    default:
----------------
+ `try` handling


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:241
+      return error(Twine("End of block construct with no start:") +
+                   NestingString(NT1) + NestingString(NT2));
+    auto Top = NestingStack.back();
----------------
Nit: Maybe add " or " between to strings to clarify we require only one of them?


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:246
+      return error(Twine("Block construct type mismatch, expected:") +
+                   NestingString(NT1) + NestingString(NT2) +
+                   ", instead got:" + NestingString(Top));
----------------
Nit: Maybe add " or " between to strings to clarify we require only one of them?


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:403
+      if (Pop(Block))
+        return true;
+    } else if (BaseName == "end_function") {
----------------
We have `end_try` too


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:405
+    } else if (BaseName == "end_function") {
+      if (Pop(Function) || CheckFunctionEnd())
+        return true;
----------------
Did you mean to use `&&`? Not sure if `||` makes sense here 


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:559
+        if (CheckFunctionEnd())
+          return true;
         CurrentState = FunctionStart;
----------------
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?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55797