[PATCH] D91870: [WebAssembly] Add support for table linking to wasm-ld

[Andy Wingo via Phabricator via llvm-commits]

wingo added inline comments.


================
Comment at: lld/wasm/Driver.cpp:805
+static TableSymbol *
+createUndefinedIndirectFunctionTable(const StringRef &name) {
+  WasmLimits limits{0, 0, 0}; // Set by the writer.
----------------
sbc100 wrote:
> I don't think it ever make sense to pass StringRef by reference does it?
Good point :)


================
Comment at: lld/wasm/Driver.cpp:822
+  // Even though we may not need a table, if the user explicitly specified
+  // --import-table or --export-table, ensure a table is residualized.
+  const StringRef name(functionTableName);
----------------
sbc100 wrote:
> I guess this is existing behaviour.   However I wonder if it would be better (in the future) to have --import-table and --export-table only effect the table if exists?   
Good question.  Is it reasonable to make the rest of the toolchain inspect the output module to see if it actually needed a table?  My initial thought was "well they asked for it, let's give them what they expect" but perhaps this is not the right thing.


================
Comment at: lld/wasm/InputFiles.cpp:313
 
+void ObjFile::synthesizeTableSymbols() {
+  uint32_t tableNumber = 0;
----------------
sbc100 wrote:
> Could you add a comment about the purpose of this function either in the header or here.  IIUC this can be removed if ever drop support for older objects files, is that right?
Good point!  And yes it could be dropped in some future.


================
Comment at: lld/wasm/InputFiles.cpp:314
+void ObjFile::synthesizeTableSymbols() {
+  uint32_t tableNumber = 0;
+  const WasmGlobalType *globalType = nullptr;
----------------
sbc100 wrote:
> Hoa 
What does this mean? :)


================
Comment at: lld/wasm/InputFiles.cpp:323
+        // FIXME: Synthesize as Module.Field.  Need somewhere to put the
+        // string backing buffer though.
+        info->Name = import.Field;
----------------
sbc100 wrote:
> In other places we use `saver` which is `StringSaver` defined in `lld/include/lld/Common/Memory.h`
Good tip, thanks.  Though, I think the comment was actually erroneous; shouldn't the name be just (e.g.) "__indirect_function_table" ?


================
Comment at: lld/wasm/InputFiles.cpp:491
   }
+
+  // As a stopgap measure while implementing table support, if the object file
----------------
sbc100 wrote:
> Ah.. I see you added a comment here rather than in the definition.      Seems reasonable.
The updated patch includes a comment both places; lmk if i should remove one / change something / etc.


================
Comment at: lld/wasm/SymbolTable.h:108
   std::vector<InputGlobal *> syntheticGlobals;
+  std::vector<InputTable *> syntheticTables;
 
----------------
sbc100 wrote:
> IIUC this is only need to support legacy object files?  Perhaps add a TODO or a note here that to that effect?
I think not, actually -- before, there was some custom logic to reify an __indirect_function_table as part of the writer.  Now, for all files, if an indirect function table definition is needed, it will be made with respect to a synthesized InputTable.  So this vector will only ever have 1 or 0 elements.


================
Comment at: lld/wasm/Writer.cpp:775
+  // function table.
+  finalizeIndirectFunctionTable();
 }
----------------
sbc100 wrote:
> Can/should we call this after `scanRelocations` in the caller?
> 
> There are many other things that need to happen only after relocations are scanned.  I don't see why this one should inlined, unless I'm missing something?
Sure, no prob.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91870/new/

https://reviews.llvm.org/D91870