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

[Derek Schuff via Phabricator via llvm-commits]

dschuff added inline comments.


================
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:
> sbc100 wrote:
> > 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.
> > 
> > 
> Talked with @sunfish also and he seemed to prefer a directive.
> 
> What name should we use instead? .wasm_global? That's a bit odd as none of the other directives have wasm_ in them. Maybe .globaltype __stack_pointer i32 makes sense.
I like the directive idea too. ELF has `.type` (https://sourceware.org/binutils/docs/as/Type.html) for symbol types. I think we have been using it for functions and global variables already as a consequence of our overlap/inheritance of ELF. It seems like this would fit in the same space. Whether we directly use ELF's semantics for the possible types or the ELF implementation code would be another question, but I think conceptually `.type` makes sense.


Repository:
  rL LLVM

https://reviews.llvm.org/D53842