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

[Andy Wingo via Phabricator via llvm-commits]

wingo marked an inline comment as done.
wingo added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:234-241
+  void Initialize(MCAsmParser &Parser) override {
+    MCAsmParserExtension::Initialize(Parser);
+
+    DefaultFunctionTable = GetOrCreateFunctionTableSymbol(
+        getContext(), "__indirect_function_table");
+    if (!STI->checkFeatures("+reference-types"))
+      DefaultFunctionTable->setOmitFromLinkingSection();
----------------
tlively wrote:
> The code in this patch fixes up the user's assembly by adding in table operands that did not appear in the source. IIUC, the motivation for this is to maintain backward compatibility with existing assembly that does not contain explicit table operands. IMO, rather than magically fixing the user's code, it would be better to error out on any call_indirect without an explicit table operand when reference-types is enabled. The downside would be that users would need to edit their assembly code when they enable reference-types, but I think the benefit of removing this "magic" from the assembler would be worth it.
Context was https://reviews.llvm.org/D90948#inline-849046, FWIW.  Sure, I can make this change, but note that the other downside is that reference-types compilation units that want to link to MVP objects will need to correctly spell "__indirect_function_table" in their call_indirect instances, otherwise you could accidentally use the wrong table.


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:611
+              WasmSym->setType(SymbolType.getValue());
+            }
+          }
----------------
tlively wrote:
> wingo wrote:
> > sbc100 wrote:
> > > I guess it makes sense to piggy back on the existing symbol handling at the end of the instruction...  but would having the symbol immediate come before the signature make more sense for this instruction?
> > I wasn't sure, tbh, and in the end went with the order in which the fields appear in the binary: https://webassembly.github.io/reference-types/core/binary/instructions.html#control-instructions.  If you think I should change it, happy to do so.
> In the text format the table index appears first. I think it would be good to be consistent with that rather than the binary format.
Sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90948