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

Wouter van Oortmerssen via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Nov 1 12:47:31 PDT 2018


aardappel added inline comments.


================
Comment at: include/llvm/ADT/StringSwitch.h:214
+    return std::move(Result);
+  }
+
----------------
kristina wrote:
> aardappel wrote:
> > dschuff wrote:
> > > aardappel wrote:
> > > > Since this is a core LLVM class, should I get someone in particular to review? Or am I better off writing the calling code differently?
> > > For our use case, we have `T = wasm::WasmSymbolType` and `R` defaults to `T`. Could we set `R` to `Optional<wasm::WasmSymbolType>` and just use `Default` instead of `NoDefault`?
> > That doesn't work because the existing `Default` always returns a `T`, i.e. there is no way to indicate a null `Optional`.
> The general convention is to have an "invalid" type as a default which can be explicitly checked for, something like `wasm::WASM_SYMBOL_TYPE_INVALID`. IMO if the enum has no default value and yet it's possible for it to end up in an "invalid" state, it *should* have a default value, which makes it much easier to understand the code.
I agree, and most enums have these. I didn't want to add such an enum value here since `0` was already taken by a legal value, and I thought it should be possible to detect that no case applies. But this is the best solution I will add such a value, will need to be value `4` though (enum storage type is `unsigned`).


================
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?
----------------
dschuff wrote:
> 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.
We use it as `.type    test0, at function` already, so I could add parsing of `.type    __stack_pointer, at global,i32` ?

I'd have to output such directives at the end of a `.s` though, if `InstPrinter` permits, since I won't know what globals I will encounter at the start. 


Repository:
  rL LLVM

https://reviews.llvm.org/D53842





More information about the llvm-commits mailing list