[PATCH] D92321: [WebAssembly] Allow element sections for nonzero table numbers

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: lld/wasm/SyntheticSections.cpp:450
+    writeU8(os, elemKind, "elem kind");
+  }
+
----------------
sbc100 wrote:
> This flag is never set so this is dead code isn't it?
> 
> I'm guessing that when we add actual multi table here we will still want to separate the linker-synthetic indirectFunctionTable from other tables that are composed of actual input segments, so I'm not sure it makes since to make this code too generic here.
I still think this block should be completely removed.   This method exists only for writing the indirectFunctionTable which by definition does not have this flag and does need this code block.   If we ever change the type of indirectFunctionTable we can change this function.  

When we add a method for writing a generic element section it most likely won't be a SyntheticSections like this one is but one that is made up of InputChunk..  that method will want to be more generic but I don't see why this one should be.  


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:908
+    const MCSymbolWasm *IndirectFunctionTable, ArrayRef<uint32_t> TableElems) {
+  assert(IndirectFunctionTable || TableElems.empty());
   if (TableElems.empty())
----------------
How about moving the assert below the early return and changing it to just `assert(IndirectFunctionTable)`


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:917
+
+  assert(WasmIndices.count(cast<MCSymbolWasm>(IndirectFunctionTable)));
+  uint32_t TableNumber = WasmIndices.find(IndirectFunctionTable)->second;
----------------
This cast looks redundant maybe?


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:1839
+    auto &Ctx = Asm.getContext();
+    auto *IndirectFunctionTable = Ctx.lookupSymbol("__indirect_function_table");
+    writeElemSection(cast_or_null<const MCSymbolWasm>(IndirectFunctionTable),
----------------
llvm policy it to avoid using `auto` on the left unless the type is completely obvious (e.g. already stated on the same line): https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:1840
+    auto *IndirectFunctionTable = Ctx.lookupSymbol("__indirect_function_table");
+    writeElemSection(cast_or_null<const MCSymbolWasm>(IndirectFunctionTable),
+                     TableElems);
----------------
I wonder if


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92321