[PATCH] D40989: [WebAssembly] De-dup indirect function table

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: wasm/InputFiles.cpp:85
 uint32_t ObjFile::relocateTableIndex(uint32_t Original) const {
-  return Original + TableIndexOffset;
+  const Symbol *Sym = getTableSymbol(Original);
+  uint32_t Index = Sym->getTableIndex();
----------------
nit: we don't use `const` in such a short function as it is obvious that it is not re-assigned.


================
Comment at: wasm/InputFiles.cpp:193
 
-  DEBUG(dbgs() << "Functions: " << FunctionSymbols.size() << "\n");
-  DEBUG(dbgs() << "Globals  : " << GlobalSymbols.size() << "\n");
+  uint32_t SegmentCount = WasmObj->elements().size();
+  if (SegmentCount) {
----------------
Can you add a comment to describe what this code block is expected to do?


================
Comment at: wasm/InputFiles.h:115
   const std::vector<Symbol *> &getSymbols() { return Symbols; }
+  const std::vector<Symbol *> &getTableSymbols() { return TableSymbols; }
 
----------------
Forgot to mention in previous review threads, but we have ArrayRef class, and we usually use that class instead of `const std::vector<T> &`. You don't need to update it in this patch, but do you mind if I ask you update it in a follow-up patch?


================
Comment at: wasm/Writer.cpp:238
 void Writer::createTableSection() {
+  // Alwasy output a table section, even if there are no indirect calls.
+  // There are two reasons for this:
----------------
always


================
Comment at: wasm/Writer.cpp:240
+  // There are two reasons for this:
+  //  1. For executables it is usefull to have an empty table slot at 0
+  //     which can be filled with a null function call handler.
----------------
useful


================
Comment at: wasm/Writer.cpp:339
+    writeUleb128(OS, Sym->getOutputIndex(), "function index");
+    TableIndex++;
+  }
----------------
nit: pre-increment is more conventional.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D40989