[lld][WebAssembly] Allow element sections for nonzero table numbers

[wingo]

Summary

This patch fixes LLD to allow element sections for tables whose number
is nonzero. We also add a test for linking multiple tables, showing
that nonzero table numbers for the indirect function table,
user-declared imported tables, and local user table definitions work.

[wingo]

Rebase

[wingo]

Rebase

[wingo]

Copy/paste GetOrCreateFunctionTableSymbol into WebAssemblyAsmParser

[wingo]

Revert change to incorrect changeset

[wingo]

Address clang-format nits

[wingo]

OK let's do this! This one finally makes multi-table linking work as it should, and includes what i think is a reasonably thorough test.

[wingo]

Fix typo introduced while addressing clang-format nits

[wingo]

Rebase and adapt test for latest [+-]reference-types expectations

[wingo]

This one is ready for review. +tlively for a second eye on how the expected behavior should work (see lld/test/wasm/multi-table.s).

[sbc100]

lld/wasm/SyntheticSections.cpp

458

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.

llvm/include/llvm/ObjectYAML/WasmYAML.h

68 ↗

(On Diff #323966)

It would be nice to see all of the non-linker parts of this change tested by llvm/test/MC/WebAssembly/tables.s.

However, I IIRC, the reason that might be hard today is that we don't support declaring elements in the assembly syntax. Perhaps we should take should figure out how that will work so we can land the core part here without depending on the linker to test them?

llvm/lib/MC/WasmObjectWriter.cpp

923 ↗

(On Diff #323966)

Might as well use if (TableNumber) again here for consistency with above.

936 ↗

(On Diff #323966)

This is dead code no? Since that flag cannot be set?

In fact all these changes to writeElemSection look unused (and therefore untested), perhaps we can split this out and make it part of a future PR?

[wingo]

llvm/lib/MC/WasmObjectWriter.cpp

936 ↗

(On Diff #323966)

So the tableNumber changes above are definitely used and tested. Definitely possible to have non-zero table numbers for the indirect function table. I could remove this HAS_ELEM_KIND bit here but let me push back gently -- we would like to test that this output is spec-conforming and also that it "makes" sense" from a LLVM point of view (e.g. can round trip through tables.s). But the former is more important than the latter. By structuring this function to look more like the [binary spec]((https://webassembly.github.io/reference-types/core/binary/modules.html#binary-elemsec)) -- e.g. checking the HAS_TABLE_NUMBER flag instead of checking TableNumber -- I have greater confidence reading the code that it does what the spec says.

[sbc100]

llvm/lib/MC/WasmObjectWriter.cpp

936 ↗

(On Diff #323966)

Sure, I don't really care if you check TableNumber of HAS_TABLE_NUMBER above.

But these six lines seems compltely pointless and in fact that are dead code right? This function doesn't write arbitrary tables it *only* writes the special __indirect_function_table (at least today). Can that ever have an ElemKind?

[wingo]

lld/wasm/SyntheticSections.cpp

433

This assertion fails for the bsymbolic test; see https://bugs.llvm.org/show_bug.cgi?id=49397.

[wingo]

Address feedback; bsymbolic test still failing

[sbc100]

lld/wasm/SyntheticSections.cpp

458

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.

llvm/lib/MC/WasmObjectWriter.cpp

908 ↗

(On Diff #327437)

How about moving the assert below the early return and changing it to just assert(IndirectFunctionTable)

917 ↗

(On Diff #327437)

This cast looks redundant maybe?

1839 ↗

(On Diff #327437)

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

1840 ↗

(On Diff #327437)

I wonder if

[wingo]

lld/wasm/SyntheticSections.cpp

458

Aaah yes I had removed it locally; I think I must have fat-fingered the "arc diff". Tx for sharp eye.

[wingo]

lld/wasm/SyntheticSections.cpp

458

As noted below, this isn't dead code at all. You have to write the elem kind if the table number is nonzero. LMK your thoughts here.

llvm/lib/MC/WasmObjectWriter.cpp

936 ↗

(On Diff #323966)

Paging this back in -- this code was not dead. See the definition for WASM_ELEM_SEGMENT_HAS_ELEM_KIND. I have restored it here. WDYT?

[wingo]

Address feedback

[sbc100]

llvm/include/llvm/ObjectYAML/WasmYAML.h

68 ↗

(On Diff #323966)

I'd still think it would be good to see at least one llvm-side test (not just the lld integration test) test for this code.

llvm/lib/MC/WasmObjectWriter.cpp

936 ↗

(On Diff #323966)

Paging this back in -- this code was not dead. See the definition for WASM_ELEM_SEGMENT_HAS_ELEM_KIND. I have restored it here. WDYT?

Oh I see.. WASM_ELEM_SEGMENT_HAS_ELEM_KIND is defined as ~SOME_OTHER_FLAG.. that seem odd at best. Perhaps better write to little helper function elemSegmentNeedsElemKind() rather than trying to make that part of the enum? Otherwise folks will think that WASM_ELEM_SEGMENT_HAS_ELEM_KIND is just another flag they can | together. Either that or maybe add MASK to its name and take it out of the enum with the others.

It looks like WASM_ELEM_SEGMENT_HAS_ELEM_KIND is basically PASSIVE | HAS_TABLE_NUMBER ... maybe its more clear to just write that out here?

[wingo]

Split off D97923

[wingo]

Add _MASK to HAS_ELEM_KIND name

lld/test/wasm/multi-table.s

+# RUN: not llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t.a1.o %s 2>&1 | FileCheck %s --check-prefix=MVP
+# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -mattr=+reference-types -o %t.a1.rt.o %s
+# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown %p/Inputs/call-indirect.s -o %t.a2.o
+# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -mattr=+reference-types %p/Inputs/call-indirect.s -o %t.a2.rt.o
+# RUN: not wasm-ld --allow-undefined --export-dynamic --no-entry -o %t.wasm %t.a1.rt.o %t.a2.o 2>&1 | FileCheck %s --check-prefix=RT-MVP
+# RUN: wasm-ld --allow-undefined --export-dynamic --no-entry -o- %t.a1.rt.o %t.a2.rt.o | obj2yaml | FileCheck %s
+
+  .globl  table_a
+  .tabletype table_a, funcref
+
+  .globl  table_b
+table_b:
+  .tabletype table_b, funcref
+
+  .globl  table_c
+table_c:
+  .tabletype table_c, externref
+
+  .globl  call_indirect_explicit_tables
+call_indirect_explicit_tables:
+  .functype call_indirect_explicit_tables () -> ()
+  i32.const 0
+  call_indirect table_a, () -> ()
+# MVP: error: Expected (, instead got: table_a
+  i32.const 0
+  call_indirect table_b, () -> ()
+  end_function
+
+# RT-MVP: wasm-ld: error: object file not built with 'reference-types' feature conflicts with import of table table_a by file
+
+# CHECK:      --- !WASM
+# CHECK-NEXT: FileHeader:
+# CHECK-NEXT:   Version:         0x1
+# CHECK-NEXT: Sections:
+# CHECK-NEXT:   - Type:            TYPE
+# CHECK-NEXT:     Signatures:
+# CHECK-NEXT:       - Index:           0
+# CHECK-NEXT:         ParamTypes:      []
+# CHECK-NEXT:         ReturnTypes:     []
+# CHECK-NEXT:       - Index:           1
+# CHECK-NEXT:         ParamTypes:      []
+# CHECK-NEXT:         ReturnTypes:
+# CHECK-NEXT:           - I64
+# CHECK-NEXT:       - Index:           2
+# CHECK-NEXT:         ParamTypes:      []
+# CHECK-NEXT:         ReturnTypes:
+# CHECK-NEXT:           - I32
+# CHECK-NEXT:   - Type:            IMPORT
+# CHECK-NEXT:     Imports:
+# CHECK-NEXT:       - Module:          env
+# CHECK-NEXT:         Field:           table_a
+# CHECK-NEXT:         Kind:            TABLE
+# CHECK-NEXT:         Table:
+# CHECK-NEXT:           Index:           0
+# CHECK-NEXT:           ElemType:        FUNCREF
+# CHECK-NEXT:           Limits:
+# CHECK-NEXT:             Initial:         0x0
+# CHECK-NEXT:       - Module:          env
+# CHECK-NEXT:         Field:           foo
+# CHECK-NEXT:         Kind:            FUNCTION
+# CHECK-NEXT:         SigIndex:        2
+# CHECK-NEXT:   - Type:            FUNCTION
+# CHECK-NEXT:     FunctionTypes:   [ 0, 1, 0 ]
+# CHECK-NEXT:   - Type:            TABLE
+# CHECK-NEXT:     Tables:
+# CHECK-NEXT:       - Index:           1
+# CHECK-NEXT:         ElemType:        FUNCREF
+# CHECK-NEXT:         Limits:
+# CHECK-NEXT:           Initial:         0x0
+# CHECK-NEXT:       - Index:           2
+# CHECK-NEXT:         ElemType:        EXTERNREF
+# CHECK-NEXT:         Limits:
+# CHECK-NEXT:           Initial:         0x0
+# CHECK-NEXT:       - Index:           3
+# CHECK-NEXT:         ElemType:        FUNCREF
+# CHECK-NEXT:         Limits:
+# CHECK-NEXT:           Flags:           [ HAS_MAX ]
+# CHECK-NEXT:           Initial:         0x3
+# CHECK-NEXT:           Maximum:         0x3
+# CHECK-NEXT:   - Type:            MEMORY
+# CHECK-NEXT:     Memories:
+# CHECK-NEXT:       - Initial:         0x2
+# CHECK-NEXT:   - Type:            GLOBAL
+# CHECK-NEXT:     Globals:
+# CHECK-NEXT:       - Index:           0
+# CHECK-NEXT:         Type:            I32
+# CHECK-NEXT:         Mutable:         true
+# CHECK-NEXT:         InitExpr:
+# CHECK-NEXT:           Opcode:          I32_CONST
+# CHECK-NEXT:           Value:           66576
+# CHECK-NEXT:   - Type:            EXPORT
+# CHECK-NEXT:     Exports:
+# CHECK-NEXT:       - Name:            memory
+# CHECK-NEXT:         Kind:            MEMORY
+# CHECK-NEXT:         Index:           0
+# CHECK-NEXT:       - Name:            table_b
+# CHECK-NEXT:         Kind:            TABLE
+# CHECK-NEXT:         Index:           1
+# CHECK-NEXT:       - Name:            table_c
+# CHECK-NEXT:         Kind:            TABLE
+# CHECK-NEXT:         Index:           2
+# CHECK-NEXT:       - Name:            call_indirect_explicit_tables
+# CHECK-NEXT:         Kind:            FUNCTION
+# CHECK-NEXT:         Index:           1
+# CHECK-NEXT:       - Name:            bar
+# CHECK-NEXT:         Kind:            FUNCTION
+# CHECK-NEXT:         Index:           2
+# CHECK-NEXT:       - Name:            call_bar_indirect
+# CHECK-NEXT:         Kind:            FUNCTION
+# CHECK-NEXT:         Index:           3
+# CHECK-NEXT:   - Type:            ELEM
+# CHECK-NEXT:     Segments:
+# CHECK-NEXT:       - Flags:           2
+# CHECK-NEXT:         TableNumber:     3
+# CHECK-NEXT:         ElemKind:        FUNCREF
+# CHECK-NEXT:         Offset:
+# CHECK-NEXT:           Opcode:          I32_CONST
+# CHECK-NEXT:           Value:           1
+# CHECK-NEXT:         Functions:       [ 2, 0 ]
+# CHECK-NEXT:   - Type:            CODE
+# CHECK-NEXT:     Functions:
+# CHECK-NEXT:       - Index:           1
+# CHECK-NEXT:         Locals:          []
+# CHECK-NEXT:         Body:            41001180808080008080808000410011808080800081808080000B
+# CHECK-NEXT:       - Index:           2
+# CHECK-NEXT:         Locals:          []
+# CHECK-NEXT:         Body:            42010B
+# CHECK-NEXT:       - Index:           3
+# CHECK-NEXT:         Locals:          []
+# CHECK-NEXT:         Body:            41002802808880800011818080800083808080001A41002802848880800011828080800083808080001A0B
+# CHECK-NEXT:   - Type:            DATA
+# CHECK-NEXT:     Segments:
+# CHECK-NEXT:       - SectionOffset:   7
+# CHECK-NEXT:         InitFlags:       0
+# CHECK-NEXT:         Offset:
+# CHECK-NEXT:           Opcode:          I32_CONST
+# CHECK-NEXT:           Value:           1024
+# CHECK-NEXT:         Content:         '0100000002000000'
+# CHECK-NEXT:   - Type:            CUSTOM
+# CHECK-NEXT:     Name:            name
+# CHECK-NEXT:     FunctionNames:
+# CHECK-NEXT:       - Index:           0
+# CHECK-NEXT:         Name:            foo
+# CHECK-NEXT:       - Index:           1
+# CHECK-NEXT:         Name:            call_indirect_explicit_tables
+# CHECK-NEXT:       - Index:           2
+# CHECK-NEXT:         Name:            bar
+# CHECK-NEXT:       - Index:           3
+# CHECK-NEXT:         Name:            call_bar_indirect
+# CHECK-NEXT:     GlobalNames:
+# CHECK-NEXT:       - Index:           0
+# CHECK-NEXT:         Name:            __stack_pointer

lld/wasm/SyntheticSections.cpp

if (config->legacyFunctionTable &&
       cast<DefinedTable>(WasmSym::indirectFunctionTable)->table == table) {
     if (out.importSec->getNumImportedTables()) {
       // Alack!  Some other input imported a table, meaning that we are unable
       // to assign table number 0 to the indirect function table.
       for (const auto *culprit : out.importSec->importedSymbols) {
         if (isa<UndefinedTable>(culprit)) {
           error("object file not built with 'reference-types' feature "
                 "conflicts with import of table " +
-                culprit->getName() + "by file " + toString(culprit->getFile()));
+                culprit->getName() + " by file " +
+                toString(culprit->getFile()));
           return;
         }
       }
       llvm_unreachable("failed to find conflicting table import");
     }
     inputTables.insert(inputTables.begin(), table);
     return;
   }
if (sym->hasTableIndex() || sym->isStub)
     return;
   sym->setTableIndex(config->tableBase + indirectFunctions.size());
   indirectFunctions.emplace_back(sym);
 }
 
 void ElemSection::writeBody() {
   raw_ostream &os = bodyOutputStream;
 
+  assert(WasmSym::indirectFunctionTable);
   writeUleb128(os, 1, "segment count");
-  writeUleb128(os, 0, "table index");
+  uint32_t tableNumber = WasmSym::indirectFunctionTable->getTableNumber();
+  uint32_t flags = 0;
+  if (tableNumber)
+    flags |= WASM_ELEM_SEGMENT_HAS_TABLE_NUMBER;
+  writeUleb128(os, flags, "elem segment flags");
+  if (flags & WASM_ELEM_SEGMENT_HAS_TABLE_NUMBER)
+    writeUleb128(os, tableNumber, "table number");
+
   WasmInitExpr initExpr;
   if (config->isPic) {
     initExpr.Opcode = WASM_OPCODE_GLOBAL_GET;
     initExpr.Value.Global = WasmSym::tableBase->getGlobalIndex();
   } else {
     initExpr.Opcode = WASM_OPCODE_I32_CONST;
     initExpr.Value.Int32 = config->tableBase;
   }
   writeInitExpr(os, initExpr);
-  writeUleb128(os, indirectFunctions.size(), "elem count");
 
+  if (flags & WASM_ELEM_SEGMENT_MASK_HAS_ELEM_KIND) {
+    // We only write active function table initializers, for which the elem kind
+    // is specified to be written as 0x00 and interpreted to mean "funcref".
+    const uint8_t elemKind = 0;
+    writeU8(os, elemKind, "elem kind");
+  }
+
+  writeUleb128(os, indirectFunctions.size(), "elem count");
   uint32_t tableIndex = config->tableBase;
   for (const FunctionSymbol *sym : indirectFunctions) {
     assert(sym->getTableIndex() == tableIndex);
     writeUleb128(os, sym->getFunctionIndex(), "function index");
     ++tableIndex;
   }
 }