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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

Maybe there are two logical parts we can separate here:

1. Support for first class tables in input files

2. Support for the special `__indirect_function_table`

I guess they are somewhat intertwined by I can imagine landing (1) without changing the way we handle the special table 0.    Maybe its not worth the work to split up?



================
Comment at: lld/wasm/Driver.cpp:803
 
+static TableSymbol *provideIndirectFunctionTable(const StringRef &Name) {
+  const uint32_t InvalidIndex = -1;
----------------
We use a slightly different naming convention in lld.  

I think the plan is to transition all of llvm eventually to this style and we are trying it out.   So locals and parameters start with lowercase, but are otherwise camelcase.  



================
Comment at: lld/wasm/InputFiles.cpp:490
+    }
+  }
 }
----------------
Could we make this separate function just to split it up?

How about:

```
if (!haveTableSymbol)
  synthesizeTableSymbols();
```




================
Comment at: lld/wasm/SymbolTable.h:110
+
+  TableSymbol *indirectFunctionTableSymbol = nullptr;
 
----------------
The other globally-known symbols are kept in `Symbols.h:WasmSym` ..  seems like maybe this should go there too?


================
Comment at: lld/wasm/Symbols.cpp:19
 #include "lld/Common/ErrorHandler.h"
+#include "lld/Common/Memory.h"
 #include "lld/Common/Strings.h"
----------------
Why this new include?


================
Comment at: lld/wasm/SyntheticSections.h:369
+
+  TableSymbol *indirectFunctionTableSymbol;
 };
----------------
This looks unused.


================
Comment at: lld/wasm/Writer.cpp:787
+    symtab->indirectFunctionTableSymbol->setLimits(limits);
+  }
+
----------------
This last seems a little out of place since its not assigning indexes.     Would this make more sense alongside wherever `out.elemSec` is populated once we know its complete?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91870