[WebAssembly] MC layer writes table symbols to object files

[wingo]

Summary

Now that the linker handles table symbols, we can allow the frontend to
produce them.

Depends on D91870.

[wingo]

Rebase

[wingo]

Rebase

[wingo]

Ready for review when you have a moment. Thanks! @sbc100

[sbc100]

llvm/lib/MC/WasmObjectWriter.cpp

523

I thought the plan was to include this table symbol in a new TABLE_NUMBER reloc to go along with the TABLE_INDEX reloc? Then each call_indirect could have up to two different relocations.

[wingo]

llvm/lib/MC/WasmObjectWriter.cpp

523

The problem isn't the call_indirect, which has a TABLE_NUMBER reloc, and doesn't reach this case. This code is for function pointers, which don't record which table they are against. It ensures that there is an __indirect_function_table if there are function pointers, even if there is never any call_indirect. Does that make sense?

[sbc100]

still lgtm

llvm/lib/MC/WasmObjectWriter.cpp

523

Hmm... yes I think makes sense. How about something like this: "any time we have a TABLE_INDEX relocation against a function symbol we need to ensure that table itself is part of the final output too". Perhaps other types of symbols or other types of relocation could apply to other tables, but for now at least TABLE_INDEX against a function symbol always means an index into __indirect_function_table... maybe I'm just re-stating what you already wrote :)

[wingo]

Reword comment

llvm/lib/MC/WasmObjectWriter.cpp

523

Thanks for the reword; applied to the comment. Cheers :)

[wingo]

Relanded after fixup in precursor; hopefully emscripten builds go green this time!

[dmajor]

Hi, after this commit our builds of wasi-sdk have broken because wasi-libc's makefile [1] finds an undefined symbol for __indirect_function_table that wasn't in its expected list [2].

Just to check, is this intended? Does the undefined-symbols.txt need an update?

[1] https://github.com/WebAssembly/wasi-libc/blob/master/Makefile#L506
[2] https://github.com/WebAssembly/wasi-libc/blob/master/expected/wasm32-wasi/undefined-symbols.txt

[sbc100]

In D92215#2505347, @dmajor wrote:

Hi, after this commit our builds of wasi-sdk have broken because wasi-libc's makefile [1] finds an undefined symbol for __indirect_function_table that wasn't in its expected list [2].

Just to check, is this intended? Does the undefined-symbols.txt need an update?

[1] https://github.com/WebAssembly/wasi-libc/blob/master/Makefile#L506
[2] https://github.com/WebAssembly/wasi-libc/blob/master/expected/wasm32-wasi/undefined-symbols.txt

My understanding is that __indirect_function_table should not be imported (at least not under normal circumstances) but should be created by the linker at link time. Do you happen to have the failing link command or a link to the build log?

[dmajor]

Here's the log: https://firefoxci.taskcluster-artifacts.net/U_L_Cc_QRxiRefZVNDs8KQ/0/public/logs/live_backing.log

[sbc100]

In D92215#2505565, @dmajor wrote:

Here's the log: https://firefoxci.taskcluster-artifacts.net/U_L_Cc_QRxiRefZVNDs8KQ/0/public/logs/live_backing.log

Ah yes, that list is a list of undefined symbols in the library/object files. In that case the new symbol *is* expected to exist and wasi-sdk will need to be modified to ignore or handle this new symbol somehow. Probably just ignoring it for now makes sense (so that the Makefile continues to work with new and old versions of llvm).

[wingo]

In D92215#2505347, @dmajor wrote:

Hi, after this commit our builds of wasi-sdk have broken because wasi-libc's makefile [1] finds an undefined symbol for __indirect_function_table that wasn't in its expected list [2].

Just to check, is this intended? Does the undefined-symbols.txt need an update?

[1] https://github.com/WebAssembly/wasi-libc/blob/master/Makefile#L506
[2] https://github.com/WebAssembly/wasi-libc/blob/master/expected/wasm32-wasi/undefined-symbols.txt

Yeah as @sbc100 says, this is intended -- object files now can have table symbols, and if the compilation unit takes the address of a function or makes an indirect call, it will have a __indirect_function_table symbol. So the expectations need updating; sorry for the bother!

Note that this could happen again in the future if/when multiple memory support is added, for memory symbols.

[dmajor]

In D92215#2506002, @wingo wrote:

Yeah as @sbc100 says, this is intended -- object files now can have table symbols, and if the compilation unit takes the address of a function or makes an indirect call, it will have a __indirect_function_table symbol. So the expectations need updating; sorry for the bother!

Looks like this grep -v would be a good place: https://github.com/WebAssembly/wasi-libc/blob/378fd4b21aab6d390f3a1c1817d53c422ad00a62/Makefile#L452

I've locally changed that to "^__mul\|^__indirect_function_table" and it's been working fine for the past few days.

@sbc100, I see that you're a project contributor, would you mind landing that to /wasi-libc and /wasi-sdk? I'm also happy to send a pair of PRs but I don't have my account handy this week.

[sbc100]

In D92215#2516547, @dmajor wrote:

In D92215#2506002, @wingo wrote:

Yeah as @sbc100 says, this is intended -- object files now can have table symbols, and if the compilation unit takes the address of a function or makes an indirect call, it will have a __indirect_function_table symbol. So the expectations need updating; sorry for the bother!

Looks like this grep -v would be a good place: https://github.com/WebAssembly/wasi-libc/blob/378fd4b21aab6d390f3a1c1817d53c422ad00a62/Makefile#L452

I've locally changed that to "^__mul\|^__indirect_function_table" and it's been working fine for the past few days.

@sbc100, I see that you're a project contributor, would you mind landing that to /wasi-libc and /wasi-sdk? I'm also happy to send a pair of PRs but I don't have my account handy this week.

Yes, just send a PR when are you get a chance. I don't think its super urgent since most wasi-libc users I think use using stable llvm releases.

[sbc100]

In D92215#2517632, @sbc100 wrote:

In D92215#2516547, @dmajor wrote:

In D92215#2506002, @wingo wrote:

Yeah as @sbc100 says, this is intended -- object files now can have table symbols, and if the compilation unit takes the address of a function or makes an indirect call, it will have a __indirect_function_table symbol. So the expectations need updating; sorry for the bother!

Looks like this grep -v would be a good place: https://github.com/WebAssembly/wasi-libc/blob/378fd4b21aab6d390f3a1c1817d53c422ad00a62/Makefile#L452

I've locally changed that to "^__mul\|^__indirect_function_table" and it's been working fine for the past few days.

@sbc100, I see that you're a project contributor, would you mind landing that to /wasi-libc and /wasi-sdk? I'm also happy to send a pair of PRs but I don't have my account handy this week.

Yes, just send a PR when are you get a chance. I don't think its super urgent since most wasi-libc users I think use using stable llvm releases.

Out of interest how did you notice this breakage? Are you integrating ToT llvm with wasi-libc for a specific reason?

[dmajor]

In D92215#2517633, @sbc100 wrote:

Out of interest how did you notice this breakage? Are you integrating ToT llvm with wasi-libc for a specific reason?

In an unofficial repo we do frequent builds of Firefox with a ToT clang, motivated by some past releases where we waited until RC1 to start testing and had a bad time.

We don't specifically set out to test the wasi bits, but some Firefox builds enable wasm sandboxing, which pulls in that code. I must admit I don't fully understand the interaction between the build systems, we have lost some expertise in that area and I'm mostly just keeping things limping along.

[wingo]

Reopening, sadly, due to https://reviews.llvm.org/D95420 and https://github.com/WebAssembly/wasi-libc/pull/228.

[wingo]

Abandoning, to merge into D90948.

llvm/lib/MC/WasmObjectWriter.cpp

if (Sym) {
             "symbol '__indirect_function_table' is not a function table");
     } else {
       Sym = cast<MCSymbolWasm>(Ctx.getOrCreateSymbol(TableName));
       Sym->setFunctionTable();
       // The default function table is synthesized by the linker.
       Sym->setUndefined();
     }
     Sym->setUsedInReloc();
+    // Any time we have a TABLE_INDEX relocation against a function symbol, we
+    // need to ensure that table itself is part of the final output too.  In the
+    // future we may want to define a new kind of reloc against both the
+    // function and the table, so that the linker can see that the function
+    // symbol keeps the table alive, but for now manually mark the table as
+    // live.
+    Sym->setNoStrip();
     Asm.registerSymbol(*Sym);
   }
 
   // Relocation other than R_WASM_TYPE_INDEX_LEB are required to be
   // against a named symbol.
   if (Type != wasm::R_WASM_TYPE_INDEX_LEB) {
     if (SymA->getName().empty())
       report_fatal_error("relocations against un-named temporaries are not yet "
uint64_t WasmObjectWriter::writeOneObject(MCAssembler &Asm,
 
   // Finally, populate the symbol table itself, in its "natural" order.
   for (const MCSymbol &S : Asm.symbols()) {
     const auto &WS = static_cast<const MCSymbolWasm &>(S);
     if (!isInSymtab(WS)) {
       WS.setIndex(InvalidIndex);
       continue;
     }
-    if (WS.isTable() && WS.getName() == "__indirect_function_table") {
-      // For the moment, don't emit table symbols -- wasm-ld can't handle them.
-      continue;
-    }
     LLVM_DEBUG(dbgs() << "adding to symtab: " << WS << "\n");
 
     uint32_t Flags = 0;
     if (WS.isWeak())
       Flags |= wasm::WASM_SYMBOL_BINDING_WEAK;
     if (WS.isHidden())
       Flags |= wasm::WASM_SYMBOL_VISIBILITY_HIDDEN;
     if (!WS.isExternal() && WS.isDefined())

llvm/test/MC/WebAssembly/debug-info.ll

 ; CHECK-NEXT:  Section {
 ; CHECK-NEXT:    Type: CUSTOM (0x0)
 ; CHECK-NEXT:    Size: 57
 ; CHECK-NEXT:    Offset: 656
 ; CHECK-NEXT:    Name: .debug_line
 ; CHECK-NEXT:  }
 ; CHECK-NEXT:  Section {
 ; CHECK-NEXT:    Type: CUSTOM (0x0)
-; CHECK-NEXT:    Size: 91
+; CHECK-NEXT:    Size: 95
 ; CHECK-NEXT:    Offset: 731
 ; CHECK-NEXT:    Name: linking
 ; CHECK-NEXT:  }
 ; CHECK-NEXT:  Section {
 ; CHECK-NEXT:    Type: CUSTOM (0x0)
 ; CHECK-NEXT:    Size: 9
-; CHECK-NEXT:    Offset: 836
+; CHECK-NEXT:    Offset: 840
 ; CHECK-NEXT:    Name: reloc.DATA
 ; CHECK-NEXT:  }
 ; CHECK-NEXT:  Section {
 ; CHECK-NEXT:    Type: CUSTOM (0x0)
 ; CHECK-NEXT:    Size: 61
-; CHECK-NEXT:    Offset: 862
+; CHECK-NEXT:    Offset: 866
 ; CHECK-NEXT:    Name: reloc..debug_info
 ; CHECK-NEXT:  }
 ; CHECK-NEXT:  Section {
 ; CHECK-NEXT:    Type: CUSTOM (0x0)
 ; CHECK-NEXT:    Size: 6
-; CHECK-NEXT:    Offset: 947
+; CHECK-NEXT:    Offset: 951
 ; CHECK-NEXT:    Name: reloc..debug_pubnames
 ; CHECK-NEXT:  }
 ; CHECK-NEXT:  Section {
 ; CHECK-NEXT:    Type: CUSTOM (0x0)
 ; CHECK-NEXT:    Size: 6
-; CHECK-NEXT:    Offset: 981
+; CHECK-NEXT:    Offset: 985
 ; CHECK-NEXT:    Name: reloc..debug_pubtypes
 ; CHECK-NEXT:  }
 ; CHECK-NEXT:  Section {
 ; CHECK-NEXT:    Type: CUSTOM (0x0)
 ; CHECK-NEXT:    Size: 6
-; CHECK-NEXT:    Offset: 1015
+; CHECK-NEXT:    Offset: 1019
 ; CHECK-NEXT:    Name: reloc..debug_line
 ; CHECK-NEXT:  }
 ; CHECK-NEXT:  Section {
 ; CHECK-NEXT:    Type: CUSTOM (0x0)
 ; CHECK-NEXT:    Size: 77
-; CHECK-NEXT:    Offset: 1045
+; CHECK-NEXT:    Offset: 1049
 ; CHECK-NEXT:    Name: producers
 ; CHECK-NEXT:  }
 ; CHECK-NEXT:]
 ; CHECK-NEXT:Relocations [
 ; CHECK-NEXT:  Section (7) DATA {
 ; CHECK-NEXT:    0x6 R_WASM_MEMORY_ADDR_I32 myextern 0
 ; CHECK-NEXT:    0xF R_WASM_TABLE_INDEX_I32 f2
 ; CHECK-NEXT:  }

 ; CHECK-NEXT:  Symbol {
 ; CHECK-NEXT:    Name: .debug_line
 ; CHECK-NEXT:    Type: SECTION (0x3)
 ; CHECK-NEXT:    Flags [ (0x2)
 ; CHECK-NEXT:      BINDING_LOCAL (0x2)
 ; CHECK-NEXT:    ]
 ; CHECK-NEXT:    ElementIndex: 0xC
 ; CHECK-NEXT:  }
+; CHECK-NEXT:  Symbol {
+; CHECK-NEXT:    Name: __indirect_function_table
+; CHECK-NEXT:    Type: TABLE (0x5)
+; CHECK-NEXT:    Flags [ (0x90)
+; CHECK-NEXT:      NO_STRIP (0x80)
+; CHECK-NEXT:      UNDEFINED (0x10)
+; CHECK-NEXT:    ]
+; CHECK-NEXT:    ImportModule: env
+; CHECK-NEXT:    ElementIndex: 0x0
+; CHECK-NEXT:  }
 ; CHECK-NEXT:]
 
 ; generated from the following C code using: clang --target=wasm32 -g -O0 -S -emit-llvm test.c
 ; extern int myextern;
 ; void f2(void) { return; }
 ;
 ; int* foo = &myextern;
 ; void (*ptr2)(void) = f2;

llvm/test/MC/WebAssembly/debug-info64.ll

 ; CHECK-NEXT:   Section {
 ; CHECK-NEXT:     Type: CUSTOM (0x0)
 ; CHECK-NEXT:     Size: 61
 ; CHECK-NEXT:     Offset: 680
 ; CHECK-NEXT:     Name: .debug_line
 ; CHECK-NEXT:   }
 ; CHECK-NEXT:   Section {
 ; CHECK-NEXT:     Type: CUSTOM (0x0)
-; CHECK-NEXT:     Size: 91
+; CHECK-NEXT:     Size: 95
 ; CHECK-NEXT:     Offset: 759
 ; CHECK-NEXT:     Name: linking
 ; CHECK-NEXT:   }
 ; CHECK-NEXT:   Section {
 ; CHECK-NEXT:     Type: CUSTOM (0x0)
 ; CHECK-NEXT:     Size: 9
-; CHECK-NEXT:     Offset: 864
+; CHECK-NEXT:     Offset: 868
 ; CHECK-NEXT:     Name: reloc.DATA
 ; CHECK-NEXT:   }
 ; CHECK-NEXT:   Section {
 ; CHECK-NEXT:     Type: CUSTOM (0x0)
 ; CHECK-NEXT:     Size: 61
-; CHECK-NEXT:     Offset: 890
+; CHECK-NEXT:     Offset: 894
 ; CHECK-NEXT:     Name: reloc..debug_info
 ; CHECK-NEXT:   }
 ; CHECK-NEXT:   Section {
 ; CHECK-NEXT:     Type: CUSTOM (0x0)
 ; CHECK-NEXT:     Size: 6
-; CHECK-NEXT:     Offset: 975
+; CHECK-NEXT:     Offset: 979
 ; CHECK-NEXT:     Name: reloc..debug_pubnames
 ; CHECK-NEXT:   }
 ; CHECK-NEXT:   Section {
 ; CHECK-NEXT:     Type: CUSTOM (0x0)
 ; CHECK-NEXT:     Size: 6
-; CHECK-NEXT:     Offset: 1009
+; CHECK-NEXT:     Offset: 1013
 ; CHECK-NEXT:     Name: reloc..debug_pubtypes
 ; CHECK-NEXT:   }
 ; CHECK-NEXT:   Section {
 ; CHECK-NEXT:     Type: CUSTOM (0x0)
 ; CHECK-NEXT:     Size: 6
-; CHECK-NEXT:     Offset: 1043
+; CHECK-NEXT:     Offset: 1047
 ; CHECK-NEXT:     Name: reloc..debug_line
 ; CHECK-NEXT:   }
 ; CHECK-NEXT:   Section {
 ; CHECK-NEXT:     Type: CUSTOM (0x0)
 ; CHECK-NEXT:     Size: 77
-; CHECK-NEXT:     Offset: 1073
+; CHECK-NEXT:     Offset: 1077
 ; CHECK-NEXT:     Name: producers
 ; CHECK-NEXT:   }
 ; CHECK-NEXT: ]
 ; CHECK-NEXT: Relocations [
 ; CHECK-NEXT:   Section (7) DATA {
 ; CHECK-NEXT:     0x6 R_WASM_MEMORY_ADDR_I64 myextern 0
 ; CHECK-NEXT:     0x13 R_WASM_TABLE_INDEX_I64 f2
 ; CHECK-NEXT:   }

 ; CHECK-NEXT:   Symbol {
 ; CHECK-NEXT:     Name: .debug_line
 ; CHECK-NEXT:     Type: SECTION (0x3)
 ; CHECK-NEXT:     Flags [ (0x2)
 ; CHECK-NEXT:       BINDING_LOCAL (0x2)
 ; CHECK-NEXT:     ]
 ; CHECK-NEXT:     ElementIndex: 0xC
 ; CHECK-NEXT:   }
+; CHECK-NEXT:   Symbol {
+; CHECK-NEXT:     Name: __indirect_function_table
+; CHECK-NEXT:     Type: TABLE (0x5)
+; CHECK-NEXT:     Flags [ (0x90)
+; CHECK-NEXT:       NO_STRIP (0x80)
+; CHECK-NEXT:       UNDEFINED (0x10)
+; CHECK-NEXT:     ]
+; CHECK-NEXT:     ImportModule: env
+; CHECK-NEXT:     ElementIndex: 0x0
+; CHECK-NEXT:   }
 ; CHECK-NEXT: ]
 
 ; generated from the following C code using: clang --target=wasm64 -g -O0 -S -emit-llvm test.c
 ; extern int myextern;
 ; void f2(void) { return; }
 ;
 ; int* foo = &myextern;
 ; void (*ptr2)(void) = f2;

llvm/test/MC/WebAssembly/function-alias.ll

 ; CHECK-NEXT:   }
 ; CHECK-NEXT:   Symbol {
 ; CHECK-NEXT:     Name: bar
 ; CHECK-NEXT:     Type: FUNCTION (0x0)
 ; CHECK-NEXT:     Flags [ (0x0)
 ; CHECK-NEXT:     ]
 ; CHECK-NEXT:     ElementIndex: 0x0
 ; CHECK-NEXT:   }
+; CHECK-NEXT:   Symbol {
+; CHECK-NEXT:     Name: __indirect_function_table
+; CHECK-NEXT:     Type: TABLE (0x5)
+; CHECK-NEXT:     Flags [ (0x90)
+; CHECK-NEXT:       NO_STRIP (0x80)
+; CHECK-NEXT:       UNDEFINED (0x10)
+; CHECK-NEXT:     ]
+; CHECK-NEXT:     ImportModule: env
+; CHECK-NEXT:     ElementIndex: 0x0
+; CHECK-NEXT:   }
 ; CHECK-NEXT: ]

llvm/test/MC/WebAssembly/global-ctor-dtor.ll

 ; CHECK-NEXT:         Name:            func1
 ; CHECK-NEXT:         Flags:           [ UNDEFINED ]
 ; CHECK-NEXT:         Function:        3
 ; CHECK-NEXT:       - Index:           10
 ; CHECK-NEXT:         Kind:            FUNCTION
 ; CHECK-NEXT:         Name:            func0
 ; CHECK-NEXT:         Flags:           [ UNDEFINED ]
 ; CHECK-NEXT:         Function:        4
+; CHECK-NEXT:       - Index:           11
+; CHECK-NEXT:         Kind:            TABLE
+; CHECK-NEXT:         Name:            __indirect_function_table
+; CHECK-NEXT:         Flags:           [ UNDEFINED, NO_STRIP ]
+; CHECK-NEXT:         Table:           0
 ; CHECK-NEXT:     SegmentInfo:
 ; CHECK-NEXT:       - Index:           0
 ; CHECK-NEXT:         Name:            .data.global1
 ; CHECK-NEXT:         Alignment:       3
 ; CHECK-NEXT:         Flags:           [ ]
 ; CHECK-NEXT:     InitFunctions:
 ; CHECK-NEXT:       - Priority: 42
 ; CHECK-NEXT:         Symbol: 9
 ; CHECK-NEXT:       - Priority: 42
 ; CHECK-NEXT:         Symbol: 2
 ; CHECK-NEXT:       - Priority: 65535
 ; CHECK-NEXT:         Symbol: 10
 ; CHECK-NEXT:       - Priority: 65535
 ; CHECK-NEXT:         Symbol: 7
 ; CHECK-NEXT: ...

llvm/test/MC/WebAssembly/reloc-pic.s

 # CHECK-NEXT:         Kind:            DATA
 # CHECK-NEXT:         Name:            __table_base
 # CHECK-NEXT:         Flags:           [ UNDEFINED ]
 # CHECK-NEXT:       - Index:           9
 # CHECK-NEXT:         Kind:            FUNCTION
 # CHECK-NEXT:         Name:            hidden_func
 # CHECK-NEXT:         Flags:           [ BINDING_LOCAL ]
 # CHECK-NEXT:         Function:        5
+# CHECK-NEXT:       - Index:           10
+# CHECK-NEXT:         Kind:            TABLE
+# CHECK-NEXT:         Name:            __indirect_function_table
+# CHECK-NEXT:         Flags:           [ UNDEFINED, NO_STRIP ]
+# CHECK-NEXT:         Table:           0
 # CHECK-NEXT:     SegmentInfo:
 # CHECK-NEXT:       - Index:           0
 # CHECK-NEXT:         Name:            .rodata.hidden_data
 # CHECK-NEXT:         Alignment:       0
 # CHECK-NEXT:         Flags:           [  ]
 # CHECK-NEXT: ...

llvm/test/MC/WebAssembly/type-index.s

 # BIN-NEXT:     Name:            linking
 # BIN-NEXT:     Version:         2
 # BIN-NEXT:     SymbolTable:
 # BIN-NEXT:       - Index:           0
 # BIN-NEXT:         Kind:            FUNCTION
 # BIN-NEXT:         Name:            test0
 # BIN-NEXT:         Flags:           [ BINDING_LOCAL ]
 # BIN-NEXT:         Function:        0
+# BIN-NEXT:       - Index:           1
+# BIN-NEXT:         Kind:            TABLE
+# BIN-NEXT:         Name:            __indirect_function_table
+# BIN-NEXT:         Flags:           [ UNDEFINED, NO_STRIP ]
+# BIN-NEXT:         Table:           0
 # BIN-NEXT: ...

llvm/test/MC/WebAssembly/weak-alias.s

 # CHECK-NEXT:         Segment:         0
 # CHECK-NEXT:         Size:            4
 # CHECK-NEXT:       - Index:           9
 # CHECK-NEXT:         Kind:            DATA
 # CHECK-NEXT:         Name:            bar_alias
 # CHECK-NEXT:         Flags:           [ BINDING_WEAK, VISIBILITY_HIDDEN, NO_STRIP ]
 # CHECK-NEXT:         Segment:         0
 # CHECK-NEXT:         Size:            4
+# CHECK-NEXT:       - Index:           10
+# CHECK-NEXT:         Kind:            TABLE
+# CHECK-NEXT:         Name:            __indirect_function_table
+# CHECK-NEXT:         Flags:           [ UNDEFINED, NO_STRIP ]
+# CHECK-NEXT:         Table:           0
 # CHECK-NEXT:     SegmentInfo:
 # CHECK-NEXT:       - Index:           0
 # CHECK-NEXT:         Name:            .data.bar
 # CHECK-NEXT:         Alignment:       3
 # CHECK-NEXT:         Flags:           [ ]
 # CHECK-NEXT:       - Index:           1
 # CHECK-NEXT:         Name:            .data.direct_address
 # CHECK-NEXT:         Alignment:       3