[PATCH] D97843: [lld][WebAssembly] -Bsymbolic creates indirect function table if needed

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

lgtm % comments / nits



================
Comment at: lld/wasm/Driver.cpp:800
+  const bool required = false;
+  return symtab->resolveIndirectFunctionTable(required);
 }
----------------
This function can probably just be removed now.


================
Comment at: lld/wasm/SymbolTable.cpp:683
+      return createUndefinedIndirectFunctionTable(functionTableName);
+    return nullptr;
+  } else if ((existing && existing->isLive()) || config->exportTable ||
----------------
Remove this line?


================
Comment at: lld/wasm/SyntheticSections.cpp:304
+  WasmSym::indirectFunctionTable =
+      symtab->resolveIndirectFunctionTable(required);
+}
----------------
you can do `resolveIndirectFunctionTable(/*required =*/true);` to avoid the extra local

Perhaps the early return is then overkill if if the body is just one line anyway?


================
Comment at: lld/wasm/SyntheticSections.cpp:315
+  if (auto *F = dyn_cast<FunctionSymbol>(sym)) {
+    handleIndirectFunctionTableLateRequirement();
     out.elemSec->addEntry(F);
----------------
How about calling this `ensureIndirectFunctionTable` ? 


================
Comment at: lld/wasm/Writer.cpp:748
+  if (!indirectFunctionTablePresentWhenComputingImports &&
+      shouldImport(WasmSym::indirectFunctionTable)) {
+    // Processing -Bsymbolic relocations resulted in a late requirement that the
----------------
This kind of ordering violation kind of makes me sad, but I'm not sure I have any better ideas.


================
Comment at: lld/wasm/Writer.cpp:748
+  if (!indirectFunctionTablePresentWhenComputingImports &&
+      shouldImport(WasmSym::indirectFunctionTable)) {
+    // Processing -Bsymbolic relocations resulted in a late requirement that the
----------------
sbc100 wrote:
> This kind of ordering violation kind of makes me sad, but I'm not sure I have any better ideas.
Can this be replaced by:

```
// In some cases indirectFunctionTable is added during scanRelocations and therefore was not
// added to the import list at the normal time. 
if (shouldImport(WasmSym::indirectFunctionTable) && !WasmSym::indirectFunctionTable.hasTableNumber()) {
    ...
````

This would avoid the extra `indirectFunctionTablePresentWhenComputingImports` variable which would simplify the patch a little I think (you could revert the conversion of finalizeIndirectFunctionTable to a member for example).

Its still ugly but less intrusive maybe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97843