[PATCH] D90948: [WebAssembly] call_indirect issues table number relocs

[Thomas Lively via Phabricator via llvm-commits]

tlively added inline comments.


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:530-531
+    // symbol has a symtab entry.
+    if (HasReferenceTypes)
+      Sym->setUsedInReloc();
     // Any time we have a TABLE_INDEX relocation against a function symbol, we
----------------
What happens if reference-types is not enabled? It looks like we would still have a table number relocation and a table symbol, but the table symbol isn't marked as used. I assume the addition of `if (WasmSym->isNoStrip())` above causes the symbol not to be emitted in this case, but is the relocation still emitted?

Overall the WasmObjectWriter seems like the wrong place to be checking target features. The encoding of a Wasm object's contents shouldn't depend on whether reference-types is enabled; it's the contents themselves that should depend on whether reference-types is enabled. Can we encapsulate all the target feature checking in codegen so that we only emit table number expressions/relocations/symbols if reference-types is enabled?



================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:478
+    MCSymbolWasm *Sym = GetOrCreateFunctionTableSymbol(getContext(), TableName);
+    if (STI->checkFeatures("+reference-types")) {
+      auto *Val = MCSymbolRefExpr::create(Sym, getContext());
----------------
I have a similar comment here. IMO, if the assembly contains a table symbol, it should be unconditionally parsed as containing a table symbol. It should be the responsibility of the producer of the assembly to not use table symbols if they want to be compatible with older tools.

I'm not sure what to do about the special case of `__indirect_function_table`, though. When was that introduced?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp:883
+      // ensure the table is live.
+      Table->setNoStrip();
+      MIB.addImm(0);
----------------
Why do we need a table symbol at all in this case (and also in DAG ISel), especially since it can't be emitted?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90948