[PATCH] D43264: [WebAssembly] Add explicit symbol table

[Nicholas Wilson via Phabricator via llvm-commits]

ncw accepted this revision.
ncw added a comment.
This revision is now accepted and ready to land.

All looks good, thanks for integrating with the Symbols.h changes!



================
Comment at: wasm/InputChunks.h:183
+// form the final GLOBALS section.
+class InputGlobal : public InputChunk {
+public:
----------------
sbc100 wrote:
> ruiu wrote:
> > Chunk is a concept to represent some contiguous bytes in an input file or a synthesized one. Essentially, a chunk is a section or something similar.
> > 
> > It doesn't seems to me that this class falls in that category, though I may be wrong due to lack of knowledge on wasm. Is this an appropriate abstraction?
> Yes, are you very astute.  This was my feedback on @ncw 's original change too.  However after playing around with it locally a bunch I decided it might be OK (for now anyway).
> 
> I did put a TODO below about this.
> 
> One idea which would work OK is to remove this from the "InputChunk" class hierarchy.   However, if I do this then this new class is out on its own, maybe in a new file.  Not a big deal I guess.
> 
> If we come up with higher level concept such as a more general "ElementFromIntoFile" :)  perhaps we could share a bare class?  I'm happy to experiment more with that once this lands.  Or if you feel strongly, before it lands.
My feedback when Sam asked about this was: Well it's not exactly a chunk of contiguous bytes with relocations, but it is nonetheless a piece of "data" from the input file that has to be copied to the output file.

The data is a struct WasmInitExpr; it's a fixed-size chunk of data that doesn't support any relocations, but is nonetheless a kind of input section, where the contents satisfy certain constraints. We could in fact have the `data()` method return a reference to its binary encoding as the "Body" of the chunk that will be written out in the GLOBALS part of the Wasm file, and then get rid of the `getInitExpr()` method. Would that be a suitable post-commit tidy? 


================
Comment at: wasm/InputFiles.cpp:183
+FunctionSymbol *ObjFile::getFunctionSymbol(uint32_t Index) const {
+  return dyn_cast<FunctionSymbol>(Symbols[Index]);
+}
----------------
Could just use `cast<>` to get the assertion for free?


================
Comment at: wasm/Symbols.h:135
 
-  // Explict function type, needed for undefined or synthetic functions only.
+  // Explicit function type, needed for undefined or synthetic functions only.
   const WasmSignature *FunctionType = nullptr;
----------------
Could add a comment explaining that the symbol type is available via the InputChunk for DefinedFunction. Even though I implemented that, and forgot about it just now when I was reviewing, and spent a minute trying to remember how checkSymbolTypes was able to work if the FunctionType wasn't set for defined functions!


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D43264