[PATCH] D53842: [WebAssembly] Parsing missing directives to produce valid .o

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:138
+  std::string LastTypedSymbolName;
+  wasm::WasmSymbolType LastTypeOfSymbol;
+  const MCSubtargetInfo &STI;
----------------
LastSymbolType?


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:145
+      : MCTargetAsmParser(Options, STI, MII), Parser(Parser),
+        Lexer(Parser.getLexer()), LastSymbol(nullptr), LastFunction(nullptr),
+        LastTypeOfSymbol(wasm::WASM_SYMBOL_TYPE_FUNCTION), STI(STI) {
----------------
Use `= nullptr` above instead?


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:282
+                                            Triple::wasm64);
+          // TODO: do we ideally want more generic handling like this code
+          // below instead of checkKnownSymbol?
----------------
aardappel wrote:
> dschuff wrote:
> > aardappel wrote:
> > > I could remove the code below, left in for code review purposes.
> > In general, I think making the assembler dumb is not a bad thing. Also you could argue that conventions for symbol names are compiler/ABI conventions and should not be automatically assumed by the assembler. So I guess if we thought of it that way it would mean that we'd have something like this generic code here, and the compiler would be required to output a directive declaring the type of `__stack_pointer` as a global, right? I think that is actually better than making the assembler smart about the compiler's ABI. Even the factoring of the Assembler/MC code reflects this idea; it's why there's no good place to put that code if it were shared.
> Ok, so I could make the InstPrinter print out this info, but really the only information that should be in the .S output is wether it is global (which is already in there, and I can recover using the commented code below), and the type of the var, which I can also infer in the assembler. So really I don't need to share the function, the only reason to do was that a) it happens the same in both places, and b) that if someone decides to refactor this code they'll be forced to look at both.
> 
> I could make the assembler dumber if the InstPrinter also outputs the type. This could be in a directive like `.global __stack_pointer i32` but there is no obvious prelude/post output in InstPrinter (on the wasm side). Or we could pack it all in the symbol reference somehow, like `__stack_pointer at GLOBAL:I32` (would maybe require changes from the general parser) or `__stack_pointer at GLOBAL_I32` (polute with wasm specific symbol types).
Seem like either `.global __stack_pointer` or `__stack_pointer at GLOBAL_I32` would be preferable to checkKnownSymbol.   For one thing, it would be nice if authors of .S files could declare (or at least use)  globals.

Also, `.global` is probably a bad directive name because `.globl` already exists for symbol binding.




================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:376
+                  .Case("global", wasm::WASM_SYMBOL_TYPE_GLOBAL)
+                  .Case("section", wasm::WASM_SYMBOL_TYPE_SECTION)
+                  .NoDefault();
----------------
Can we limit this to supporting just `function` and `global` here?   Sections are created using `.section` directive and I think data is the default.

Then maybe you can avoid the StringSwitch change too.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:396
+        return Error("Cannot parse .size expression: ", Lexer.getTok());
+      LastFunction->setSize(Exp);
     } else if (DirectiveID.getString() == ".param" ||
----------------
Isn't `.size` already handled by the ELFAsmParser, e.g. `ELFAsmParser::ParseDirectiveSize`?  Or do we not use the ELFAsmParser at all anymore?


Repository:
  rL LLVM

https://reviews.llvm.org/D53842