[PATCH] D42495: [WebAssembly] Add symbol table to LLVM, 2/2
Nicholas Wilson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 7 09:00:19 PST 2018
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
More information about the llvm-commits
mailing list