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

Fri Feb 19 12:04:39 PST 2021

tlively 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();
----------------
wingo wrote:
> 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.
Acknowledged. I asked @aardappel and he didn't feel strongly about it and I'd like to get feedback from @sbc100 about it but he's out of office today. I'd be ok landing this as-is and potentially revisiting it with feedback from more stakeholders. (The more I think about it, the less strongly I feel about this, too.)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90948