[PATCH] D42495: [WebAssembly] Add symbol table to LLVM, 2/2

[Nicholas Wilson via Phabricator via llvm-commits]

ncw added inline comments.


================
Comment at: include/llvm/BinaryFormat/Wasm.h:233
+  WASM_SYMTAB_DATA     = 0x1,
+  WASM_SYMTAB_GLOBAL   = 0x2,
+};
----------------
sbc100 wrote:
> [Not really part of this change but...]  Should we add a separate type of BSS here?  Or can BSS symbols be expresses as DATA with no segment?
> 
> Should these be WASM_SYMTYPE_XXX or WASM_SYMBOL_XXX or maybe just WASM_SYM_XXX ?  
Renamed to `WASM_SYMBOL_TYPE_XXX` - a bit verbose but matches conventions for the other enums.

Regarding BSS: I've been thinking/planning how to handle that; I can work on it after these patches if you want. What I had in mind was that the segment-metadata would gain the ability to describe BSS segments via a flag (currently no segment flags are defined), and for BSS segments there would be no corresponding Wasm segment. Currently we have Wasm segments mapping 1-to-1 to segment metadata; in the new scheme we'd have possibly more segment metadata than Wasm segments, and BSS segment metadata wouldn't have a corresponding Wasm segment.


================
Comment at: include/llvm/BinaryFormat/Wasm.h:235
+};
+
 const unsigned WASM_SYMBOL_BINDING_MASK       = 0x3;
----------------
sbc100 wrote:
> I think maybe we should expose the symtab struct type here, and have WasmObjectFile have a symtab() method for iterating them directly.
> 
> My preference is for all the the structures that map directly from the binary should be declared here in BinaryFormat/Wasm.h.
> 
> Then perhaps we can even from the existing WasmSymbol from the other Wasm.h?  Or at least have it be a wrapper around the one declared here?
Good idea, makes sense. I've added it to the LinkingMetadata struct rather than have a new `symtab()` method on the WasmObjectFile.

In fact, I've got a future commit where I'll add the Comdats to the LinkingMetadata (rather than their location as a freestanding method on WasmObjectFile).

I've stripped the guts out of llvm::object::WasmSymbol, but I've kept it around for the time being. It currently contains some extra information that isn't stored directly in the binary format, so it makes sense to keep it - in the same way that llvm::object::WasmSegment and WasmSection wrap the underlying BinaryFormat structs.


================
Comment at: include/llvm/Object/Wasm.h:41
+    DATA     = wasm::WASM_SYMTAB_DATA,
+    GLOBAL   = wasm::WASM_SYMTAB_GLOBAL,
   };
----------------
sbc100 wrote:
> ncw wrote:
> > sbc100 wrote:
> > > If these mirror the existing types then maybe just drop this enum completely?
> > Possibly, yes, but it does mirror existing usage. The enums in BinaryFormat are `enum : unsigned` and are used for assigning values read out of a binary, and the enums in Object are `enum class` and are used for logical operations like switch statements where you want the strongest compiler warnings.
> I don't think we need to duplicate this just for that reason.   I think we should just remove this completely.
Ok, removed.


Repository:
  rL LLVM

https://reviews.llvm.org/D42495