[PATCH] D55149: [WebAssembly] Enforce assembler emits to streamer in order.

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel marked 5 inline comments as done.
aardappel added inline comments.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:161
       : MCTargetAsmParser(Options, STI, MII), Parser(Parser),
-        Lexer(Parser.getLexer()) {
+        Lexer(Parser.getLexer()), CurrentState(FileStart), LastLabel(nullptr) {
     setAvailableFeatures(ComputeAvailableFeatures(STI.getFeatureBits()));
----------------
dschuff wrote:
> aardappel wrote:
> > aheejin wrote:
> > > Nit: Can we use C++11-style in-class initialization for these two variables?
> > Sure.. I guess I was being conservative, not knowing what level of C++ I can use.
> In LLVM, we love ALL the C++! but there's still those pesky older versions of compilers, distros, etc. see https://llvm.org/docs/CodingStandards.html#languages-libraries-and-standards . See also http://lists.llvm.org/pipermail/llvm-dev/2018-October/127045.html
Yes, I use C++17 on some projects, C++0x (pre-C++11) on others, so I tend to get confused on what I can use. I'll re-read that section to remind myself :)


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:408
+        CurrentState = FunctionStart;
+      }
       auto Signature = make_unique<wasm::WasmSignature>();
----------------
aardappel wrote:
> sbc100 wrote:
> > aardappel wrote:
> > > This code is not super elegant, but kinda has to be this way because we have .functype which both introduces a function and ones which just declare external ones. Now if you misspel a label you'll fail to start a new function. 
> > > 
> > > More ideal would be split these, and have a .functype with no name that annotates the start of a function (and the assembler enforces only comes after a label) and a different one, e.g. .funcdecl, that does have its own name and is only used for external types. Not sure if such a refactoring is worth it though, after we just re-did all the tests with .functype.
> > I like the repeated name in the .functype directive because it seems to match existing assembly directives.
> > 
> > If we want to distinguish between types for internal vs external functions perhaps we would want a new directive such as ".extern_functype" but as you say I'm not sure its worth it. 
> Also note that in the past we decided that we like the assembler to be relatively "dumb", in terms of not needing to have a lot of structural knowledge about wasm. Turns out that is not possible with how streamers work, we need the assembler to enforce a certain level of structure.
Ok, will leave it as-is unless anyone else feels strongly about it.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55149