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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

Nice.  I like that we were able to delete almost as much code as was added here.

Just a few more comments but mostly lgtm.



================
Comment at: lld/wasm/Driver.cpp:791
 
+static TableSymbol *provideIndirectFunctionTable(const StringRef &name) {
+  const uint32_t invalidIndex = -1;
----------------
provide seems like  a slightly odd verb.  (Or is it just me?)  How about calling this `createIndirectFunctionTable` or `defineIndirectFunctionTable`?

Or maybe `creatDefinedIndirectFunctionTable` to match the function below?


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


================
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);
----------------
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?   


================
Comment at: lld/wasm/Driver.cpp:823
+  // --import-table or --export-table, ensure a table is residualized.
+  const StringRef name(functionTableName);
+  if (config->importTable)
----------------
Can you just use functionTableName directly below rather than adding this local?


================
Comment at: lld/wasm/InputFiles.cpp:313
 
+void ObjFile::synthesizeTableSymbols() {
+  uint32_t tableNumber = 0;
----------------
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?


================
Comment at: lld/wasm/InputFiles.cpp:314
+void ObjFile::synthesizeTableSymbols() {
+  uint32_t tableNumber = 0;
+  const WasmGlobalType *globalType = nullptr;
----------------
Hoa 


================
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;
----------------
In other places we use `saver` which is `StringSaver` defined in `lld/include/lld/Common/Memory.h`


================
Comment at: lld/wasm/InputFiles.cpp:491
   }
+
+  // As a stopgap measure while implementing table support, if the object file
----------------
Ah.. I see you added a comment here rather than in the definition.      Seems reasonable.


================
Comment at: lld/wasm/SymbolTable.h:108
   std::vector<InputGlobal *> syntheticGlobals;
+  std::vector<InputTable *> syntheticTables;
 
----------------
IIUC this is only need to support legacy object files?  Perhaps add a TODO or a note here that to that effect?


================
Comment at: lld/wasm/Writer.cpp:775
+  // function table.
+  finalizeIndirectFunctionTable();
 }
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91870