[WebAssembly][lld] Preassign table number 0 to indirect function table for MVP inputs

[wingo]

Summary

MVP object files may import at most one table, and if they do, it must
be assigned table number zero in the output, as the references to that
table are not relocatable. Ensure that this is the case, even if some
inputs define other tables.

[wingo]

Looking over this changeset, I see that we need some tests. But also there is a fair amount of code duplication for the "precoloring" logic. I can make it be a part of TableSymbol::setTableNumber and InputTable::setTableNumber; it would seem that that much duplication is necessary. But then the errors would probably be less helpful because there's less context at that point. Thoughts?

[tlively]

In D96001#2541348, @wingo wrote:

Looking over this changeset, I see that we need some tests. But also there is a fair amount of code duplication for the "precoloring" logic. I can make it be a part of TableSymbol::setTableNumber and InputTable::setTableNumber; it would seem that that much duplication is necessary. But then the errors would probably be less helpful because there's less context at that point. Thoughts?

IMO having helpful error messages is well worth some code duplication. Code duplication won't cause users to file the same bug every other week about their links failing, but unhelpful error messages will!

lld/wasm/Driver.cpp

802

Can this dyn_cast ever fail? If not, it would be better to use cast here or even make the parameter a TableSymbol.

lld/wasm/InputFiles.cpp

338

Same question about this dyn_cast.

lld/wasm/SyntheticSections.cpp

116–117

Would it be possible to include the name of an object file that contains the offending non-relocatable table? I've anecdotally found that including file names in similar error messages about unlinkable MVP and atomics objects has dramatically decreased the number of bugs that get filed about it.

lld/wasm/Writer.cpp

591

It would be good to have a short comment here as well about why some tables need to be deferred.

[sbc100]

I'm not familiar with the use of term precoloring. Perhaps I should be.. but maybe we could use something more common like preassignTableNumber rather than precolorTableNumber? Certainly I don't think we want to use the term in the user-facing errors.

lld/wasm/InputFiles.cpp

341

I'm not sure this is a very usefull user-facing error. Can it ever actually happen? Can we improve it or perhaps make it an assert?

343

Can we simplify the code here by asserting that the table number is always zero and that there is only one table.

In fact, perhaps this should be called synthesizeTableSymbol (non-plural) and just return after for first call to addTable?

lld/wasm/SymbolTable.cpp

213 ↗

(On Diff #321326)

I think this can be an unconditionally cast<> can't it?

219 ↗

(On Diff #321326)

Similarly, since there is exactly one nonrelocatable table number (zero)... can we simplify this code based on that fact?

[sbc100]

lld/wasm/SyntheticSections.cpp

119

Can this error actually happy? Wouldn't it be a bug in lld if it did? It seems like in order for this to happen there would be a bug in the pre-coloring logic.

[wingo]

Thanks very much for the review!

The "precoloring" name is a bit sloppy -- comes from register allocation FWIW, where you solve the graph coloring problem for the interference graph of register uses. Some nodes require that a register be in a specific place, e.g. %rdi for argument 0 on x86; imposing that constraint is called precoloring. So for tables, we also have a kind of precoloring constraint. But, probably best to just used the term "preassign".

lld/wasm/Driver.cpp

802

I was thinking that it could fail if the existing symbol isn't a table. In that case we would have already issued an error, but control flow could still fall through here. But now I see that we make an early-return in that case below; will fix.

lld/wasm/InputFiles.cpp

338

Unfortunately it is possible that e.g. SymbolTable::createUndefinedTable returns a non-table symbol, if there was an existing symtab entry. We would have issued an error at that point though.

341

This is a sloppy error and I need to reword it; tx for catching.

343

Humm, good question! Old linkers would not propagate "additional" tables from input to output. Also, old compilers wouldn't define a table locally -- the indirect function table would only be imported. So maybe it would be good to avoid adding a new form of acceptable input, in which object files have many table symbols.

If we tighten this restriction, we can issue an error for inputs that

• have no table symbols. and:
  • import any table not named __indirect_function_table, or
  • import more than one table, or
  • define any table

WDYT?

lld/wasm/SymbolTable.cpp

213 ↗

(On Diff #321326)

AFAICS, sadly, no :( Otherwise we could simplify checkTableType as well.

219 ↗

(On Diff #321326)

Sure. I can add a isPreassignedToTableNumberZero method or so. Surely with a better name.

lld/wasm/SyntheticSections.cpp

116–117

Sure.

119

I was thinking that it could happen if you had an MVP input that defined a table locally (not imported), but a reftypes input imported a table. But prompted by your questions I think I see that MVP inputs wouldn't ever define tables, so this error can't happen.

lld/wasm/Writer.cpp

591

Sure.

[wingo]

Address feedback

[sbc100]

lld/wasm/InputFiles.cpp

343

Sounds great!

[wingo]

Fixup casts

[wingo]

Tighten up restrictions on MVP inputs

[wingo]

Add test

[wingo]

So.... this one is ready for review again I think. I haven't been able to test some of the the error cases in synthesizeMVPIndirectFunctionTableIfNeeded because you can't actually make files with those characteristics currently.

[sbc100]

lld/wasm/InputFiles.cpp

331

For this last paragraphs how about:

"Table usage in MVP objects cannot be relocatates so the linker must ensure that this
table gets assigned index zero".

[sbc100]

lld/wasm/Driver.cpp

802

Can these two lines be moved into SymbolTable::addSyntheticTable (which already has a check for an existing symbol). Then you don't need to pass the extra existing arg here, and you don't need to extra existingTable table below?

hmm.. maybe that would make addSyntheticTable a little less tidy, but it seems like logical place... either that or replaceSymbol itself.

In general this notion of preassigned index seems to complicate things in a way that I'm not a fan of. I wish we could find a simpler way to force the indirect table to always have index 0.

[wingo]

lld/wasm/Driver.cpp

802

Right, the whole notion of symbols with preassigned addresses (table numbers in this case) is... unsatisfying. But it does seem to be essential to the constraints we're operating under. I thought that keeping the bits that much about with preassigned addresses local to the parts that deal in tables (and specifically the indirect function table) would limit the "taint".

Probably replaceSymbol is not the place for this to go, as other kinds of symbols rightly don't have a notion of preassigned addresses.

Incidentally, looking again, I don't think the code as it is works, because of the placement new in replaceSymbol :/ Will fix, I guess in addSyntheticTable.

[wingo]

Address feedback; fix placement-new bug

[wingo]

Fix comment

[wingo]

PTAL, I think all comments are addressed. Adding tlively in case he would like to weigh in.

[tlively]

This looks great, thanks @wingo! My only comments are about comments and error message contents. It would also be good to add tests for more of the error messages, especially the one about "Cannot preassign table number 0 to indirect function table..." because I expect that to be a common user-facing issue.

lld/wasm/InputFiles.cpp

323

348–349

I would remove the last sentence here and in the errors below. -mattr=+reference-types is an llc flag but not a clang flag, so it is probably not what the user wants. Also, clang should never produce objects that produce this error no matter what flags are passed to it.

lld/wasm/Symbols.cpp

389

It would be good to add a comment here along the lines of "zero is the only table number that can be preassigned." Otherwise from just the name of the function, it looks strange that other numbers aren't handled gracefully.

lld/wasm/SyntheticSections.cpp

246–247

The indirect function table is imported in Emscripten's dynamic linking scheme, so this isn't necessarily unexpected.

257–259

-mreference-types is the clang flags for enabling this feature.

Or another option would be to be tool- and flag-agnostic by saying "feature 'reference-types'" instead of naming particular flags.

[sbc100]

we when we comes to assigning indexes we do something like this:

lld/wasm/Symbols.h

382 ↗

(On Diff #322937)

I feel a little bad for pushing back more on the this change, but it still feels a little more invasive than it could be.

Rather than adding these two methods to the table class what do you think about this appoach:

• Add a global config variable that gets set if we have MVP objects in the link. We could call this something like`bool legacyFunctionTable = false`. Or alternatively "forceFunctioTableToBeZero"? Or "functionTableMustBeZero"? I kind of like the idea of a bool here because it clearly indicates that are two different modes.
• We already track the global symbol indirectFunctionTable.
• Then, we when we comes to assigning indexes we do something like this:

if (condig->legacyFunctionTable) {
     placeIndirectFunctionTableFirst()   <-- here we could report errors if the sorting is impossible
}

This sorting and error reporting could be done in the TableSection::assignIndexes().

Hopefully this should avoid adding the preassignTableNumberZero/hasPreassignedTableNumberZero methods and should avoid extra logic to too much extra logic addTable method.

WDYT?

(I guess it would also need to happen in ImportSection::assignIndexes...)

These are mostly cosmetic changes, that actual behaviour LGTM.

[sbc100]

lld/wasm/SyntheticSections.cpp

259

Can we be more succinct here? If you feel its is helpful/necessary to convey all this information than perhaps this verbosity is unavoidable.

Would something shorter such as "object file was not built -mattr=+reference-types enabled. This is incompatible with the use tables imports in <other-object-name>" be enough?

[wingo]

lld/wasm/Symbols.h

382 ↗

(On Diff #322937)

This can work. It's not quite as minimal as this because tables can also get assigned numbers via the import section. I'll give it a go and see.

lld/wasm/SyntheticSections.cpp

259

We can certainly be more succinct but I don't know how to be as informative. As the comment notes, we don't have a good way to know where the "conflicting" import comes from AFAIU.

[wingo]

Fix error messages and rebase

[wingo]

Instead of tracking unrelocatable uses of the indirect function table on the table symbol, just use a global flag in the config. The writer will take care to handle the indirect function table symbol specially if this flag is set, writing it before any other table.

[wingo]

Addressed feedback; @sbc100 would you mind having another look?

[sbc100]

Nice! I like this much better now. Thanks for making all those changes.

Just a few more nits and I think we can land this.

lld/test/wasm/invalid-mvp-table-use.s

4

Can you elaborate on this comment. What makes this an MVP file ? Is it simply the lack of -mattr=+reference-types.

Not part of this PR, but should we have llvm-mc fail here to catch this earlier?

lld/wasm/InputFiles.cpp

375–384

Can we replace this above block with simply:

lld/wasm/InputFiles.h

161

Can we call this addLegacyIndirectFunctionTableSymbolIfNeeded now so matches the config name?

lld/wasm/SyntheticSections.cpp

210

nit: Moving these method back down below writeBody would make the diff easier to read (smaller).

217

Can we combine these two into a single condition to remove one level of testing? In fact can we combine all three maybe?

lld/wasm/Writer.cpp

582

This can be combined into single condition (with no braces).

589–590

Oh nice so we always import this first, regardless of the config flag.. I like that.

[wingo]

Address feedback

lld/test/wasm/invalid-mvp-table-use.s

4

Elaborated. As regards llvm-mc, there will need to be some changes when we finally add (again) support for symtab entries; will follow up then.

lld/wasm/InputFiles.cpp

375–384

Missing comment here, perhaps?

[sbc100]

lld/wasm/InputFiles.cpp

375–384

Sorry, didn't mean to send that.

lld/test/wasm/invalid-mvp-table-use.s

+# RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown -o %t.o %s
+#
+# If any table is defined or declared besides the __indirect_function_table,
+# the compilation unit should be compiled with -mattr=+reference-types,
+# causing symbol table entries to be emitted for all tables.
+# RUN: not wasm-ld --no-entry %t.o -o %t.wasm 2>&1 | FileCheck -check-prefix=CHECK-ERR %s
+
+.global call_indirect
+call_indirect:
+        .functype call_indirect () -> ()
+  i32.const 1
+        call_indirect () -> ()
+        end_function
+
+.globl table
+table:
+        .tabletype table, externref
+
+# CHECK-ERR: expected one symbol table entry for each of the 2 table(s) present, but got 1 symbol(s) instead.

lld/test/wasm/shared.ll

 ; CHECK:        - Type:            IMPORT
 ; CHECK-NEXT:     Imports:
 ; CHECK-NEXT:       - Module:          env
 ; CHECK-NEXT:         Field:           memory
 ; CHECK-NEXT:         Kind:            MEMORY
 ; CHECK-NEXT:         Memory:
 ; CHECK-NEXT:           Initial:       0x1
 ; CHECK-NEXT:       - Module:          env
+; CHECK-NEXT:         Field:           __indirect_function_table
+; CHECK-NEXT:         Kind:            TABLE
+; CHECK-NEXT:         Table:
+; CHECK-NEXT:           Index:           0
+; CHECK-NEXT:           ElemType:        FUNCREF
+; CHECK-NEXT:           Limits:
+; CHECK-NEXT:             Initial:         0x2
+; CHECK-NEXT:       - Module:          env
 ; CHECK-NEXT:         Field:           __stack_pointer
 ; CHECK-NEXT:         Kind:            GLOBAL
 ; CHECK-NEXT:         GlobalType:      I32
 ; CHECK-NEXT:         GlobalMutable:   true
 ; CHECK-NEXT:       - Module:          env
 ; CHECK-NEXT:         Field:           __memory_base
 ; CHECK-NEXT:         Kind:            GLOBAL
 ; CHECK-NEXT:         GlobalType:      I32
 ; CHECK-NEXT:         GlobalMutable:   false
 ; CHECK-NEXT:       - Module:          env
 ; CHECK-NEXT:         Field:           __table_base
 ; CHECK-NEXT:         Kind:            GLOBAL
 ; CHECK-NEXT:         GlobalType:      I32
 ; CHECK-NEXT:         GlobalMutable:   false
 ; CHECK-NEXT:       - Module:          env
 ; CHECK-NEXT:         Field:           func_external
 ; CHECK-NEXT:         Kind:            FUNCTION
 ; CHECK-NEXT:         SigIndex:        1
-; CHECK-NEXT:       - Module:          env
-; CHECK-NEXT:         Field:           __indirect_function_table
-; CHECK-NEXT:         Kind:            TABLE
-; CHECK-NEXT:         Table:
-; CHECK-NEXT:           Index:           0
-; CHECK-NEXT:           ElemType:        FUNCREF
-; CHECK-NEXT:           Limits:
-; CHECK-NEXT:             Initial:         0x2
 ; CHECK-NEXT:       - Module:          GOT.mem
 ; CHECK-NEXT:         Field:           indirect_func
 ; CHECK-NEXT:         Kind:            GLOBAL
 ; CHECK-NEXT:         GlobalType:      I32
 ; CHECK-NEXT:         GlobalMutable:   true
 ; CHECK-NEXT:       - Module:          GOT.func
 ; CHECK-NEXT:         Field:           func_external
 ; CHECK-NEXT:         Kind:            GLOBAL

lld/wasm/Config.h

struct Configuration {
   llvm::Optional<std::vector<std::string>> features;
 
   // The following config options do not directly correspond to any
   // particualr command line options.
 
   // True if we are creating position-independent code.
   bool isPic;
 
+  // True if we have an MVP input that uses __indirect_function_table and which
+  // requires it to be allocated to table number 0.
+  bool legacyFunctionTable = false;
+
   // The table offset at which to place function addresses.  We reserve zero
   // for the null function pointer.  This gets set to 1 for executables and 0
   // for shared libraries (since they always added to a dynamic offset at
   // runtime).
   uint32_t tableBase = 0;
 };
 
 // The only instance of Configuration struct.
 extern Configuration *config;
 
 } // namespace wasm
 } // namespace lld
 
 #endif

lld/wasm/Driver.cpp

static TableSymbol *createDefinedIndirectFunctionTable(StringRef name) {
   InputTable *table = make<InputTable>(desc, nullptr);
   uint32_t flags = config->exportTable ? 0 : WASM_SYMBOL_VISIBILITY_HIDDEN;
   TableSymbol *sym = symtab->addSyntheticTable(name, flags, table);
   sym->markLive();
   sym->forceExport = config->exportTable;
   return sym;
 }
 
 static TableSymbol *createUndefinedIndirectFunctionTable(StringRef name) {
   WasmLimits limits{0, 0, 0}; // Set by the writer.
   WasmTableType *type = make<WasmTableType>();
   type->ElemType = uint8_t(ValType::FUNCREF);
   type->Limits = limits;
   StringRef module(defaultModule);
   uint32_t flags = config->exportTable ? 0 : WASM_SYMBOL_VISIBILITY_HIDDEN;
   flags |= WASM_SYMBOL_UNDEFINED;
   Symbol *sym =
       symtab->addUndefinedTable(name, name, module, flags, nullptr, type);
   sym->markLive();
   sym->forceExport = config->exportTable;
   return cast<TableSymbol>(sym);
 }
 
 static TableSymbol *resolveIndirectFunctionTable() {
-  Symbol *existingTable = symtab->find(functionTableName);
-  if (existingTable) {
-    if (!isa<TableSymbol>(existingTable)) {
+  Symbol *existing = symtab->find(functionTableName);
+  if (existing) {
+    if (!isa<TableSymbol>(existing)) {
       error(Twine("reserved symbol must be of type table: `") +
             functionTableName + "`");
       return nullptr;
     }
-    if (existingTable->isDefined()) {
+    if (existing->isDefined()) {
       error(Twine("reserved symbol must not be defined in input files: `") +
             functionTableName + "`");
       return nullptr;
     }
   }
 
   if (config->importTable) {
-    if (existingTable)
-      return cast<TableSymbol>(existingTable);
+    if (existing)
+      return cast<TableSymbol>(existing);
     else
       return createUndefinedIndirectFunctionTable(functionTableName);
-  } else if ((existingTable && existingTable->isLive()) ||
-             config->exportTable) {
+  } else if ((existing && existing->isLive()) || config->exportTable) {
     // A defined table is required.  Either because the user request an exported
     // table or because the table symbol is already live.  The existing table is
     // guaranteed to be undefined due to the check above.
     return createDefinedIndirectFunctionTable(functionTableName);
   }
 
   // An indirect function table will only be present in the symbol table if
   // needed by a reloc; if we get here, we don't need one.

lld/wasm/InputFiles.h

public:
   EventSymbol *getEventSymbol(uint32_t index) const;
   TableSymbol *getTableSymbol(uint32_t index) const;
 
 private:
   Symbol *createDefined(const WasmSymbol &sym);
   Symbol *createUndefined(const WasmSymbol &sym, bool isCalledDirectly);
 
   bool isExcludedByComdat(InputChunk *chunk) const;
-  void synthesizeTableSymbols();
+  void addLegacyIndirectFunctionTableIfNeeded(uint32_t tableSymbolCount);
 
   std::unique_ptr<WasmObjectFile> wasmObj;
 };
 
 // .so file.
 class SharedFile : public InputFile {
 public:
   explicit SharedFile(MemoryBufferRef m) : InputFile(SharedKind, m) {}
   static bool classof(const InputFile *f) { return f->kind() == SharedKind; }

lld/wasm/InputFiles.cpp

auto relocsStart = std::lower_bound(relocsNext, relocsEnd,
                                         c->getInputSectionOffset(), relocLess);
     relocsNext = std::lower_bound(
         relocsStart, relocsEnd, c->getInputSectionOffset() + c->getInputSize(),
         relocLess);
     c->setRelocations(ArrayRef<WasmRelocation>(relocsStart, relocsNext));
   }
 }
 
-// Since LLVM 12, we expect that if an input file defines or uses a table, it
-// declares the tables using symbols and records each use with a relocation.
-// This way when the linker combines inputs, it can collate the tables used by
-// the inputs, assigning them distinct table numbers, and renumber all the uses
-// as appropriate.  At the same time, the linker has special logic to build the
+// An object file can have two approaches to tables.  With the reference-types
+// feature enabled, input files that define or use tables declare the tables
+// using symbols, and record each use with a relocation.  This way when the
+// linker combines inputs, it can collate the tables used by the inputs,
+// assigning them distinct table numbers, and renumber all the uses as
+// appropriate.  At the same time, the linker has special logic to build the
 // indirect function table if it is needed.
 //
-// However, object files produced by LLVM 11 and earlier neither write table
-// symbols nor record relocations, and yet still use tables via call_indirect,
-// and via function pointer bitcasts.  We can detect these object files, as they
-// declare tables as imports or define them locally, but don't have table
-// symbols.  synthesizeTableSymbols serves as a shim when loading these older
-// input files, defining the missing symbols to allow the indirect function
-// table to be built.
+// However, MVP object files (those that target WebAssembly 1.0, the "minimum
+// viable product" version of WebAssembly) neither write table symbols nor
+// record relocations.  These files can have at most one table, the indirect
+// function table used by call_indirect and which is the address space for
+// function pointers.  If this table is present, it is always an import.  If we
+// have a file with a table import but no table symbols, it is an MVP object
+// file.  synthesizeMVPIndirectFunctionTableSymbolIfNeeded serves as a shim when
+// loading these input files, defining the missing symbol to allow the indirect
+// function table to be built.
 //
-// Table uses in these older files won't be relocated, as they have no
-// relocations.  In practice this isn't a problem, as these object files
-// typically just declare a single table named __indirect_function_table and
-// having table number 0, so relocation would be idempotent anyway.
-void ObjFile::synthesizeTableSymbols() {
-  uint32_t tableNumber = 0;
-  const WasmGlobalType *globalType = nullptr;
-  const WasmEventType *eventType = nullptr;
-  const WasmSignature *signature = nullptr;
-  if (wasmObj->getNumImportedTables()) {
-    for (const auto &import : wasmObj->imports()) {
-      if (import.Kind == WASM_EXTERNAL_TABLE) {
-        auto *info = make<WasmSymbolInfo>();
-        info->Name = import.Field;
-        info->Kind = WASM_SYMBOL_TYPE_TABLE;
-        info->ImportModule = import.Module;
-        info->ImportName = import.Field;
-        info->Flags = WASM_SYMBOL_UNDEFINED;
-        info->Flags |= WASM_SYMBOL_NO_STRIP;
-        info->ElementIndex = tableNumber++;
-        LLVM_DEBUG(dbgs() << "Synthesizing symbol for table import: "
-                          << info->Name << "\n");
-        auto *wasmSym = make<WasmSymbol>(*info, globalType, &import.Table,
-                                         eventType, signature);
-        symbols.push_back(createUndefined(*wasmSym, false));
-        // Because there are no TABLE_NUMBER relocs in this case, we can't
-        // compute accurate liveness info; instead, just mark the symbol as
-        // always live.
-        symbols.back()->markLive();
-      }
-    }
+// As indirect function table table usage in MVP objects cannot be relocated,
+// the linker must ensure that this table gets assigned index zero.
+void ObjFile::addLegacyIndirectFunctionTableIfNeeded(
+    uint32_t tableSymbolCount) {
+  uint32_t tableCount = wasmObj->getNumImportedTables() + tables.size();
+
+  // If there are symbols for all tables, then all is good.
+  if (tableCount == tableSymbolCount)
+    return;
+
+  // It's possible for an input to define tables and also use the indirect
+  // function table, but forget to compile with -mattr=+reference-types.
+  // For these newer files, we require symbols for all tables, and
+  // relocations for all of their uses.
+  if (tableSymbolCount != 0) {
+    error(toString(this) +
+          ": expected one symbol table entry for each of the " +
+          Twine(tableCount) + " table(s) present, but got " +
+          Twine(tableSymbolCount) + " symbol(s) instead.");
+    return;
+  }
+
+  // An MVP object file can have up to one table import, for the indirect
+  // function table, but will have no table definitions.
+  if (tables.size()) {
+    error(toString(this) +
+          ": unexpected table definition(s) without corresponding "
+          "symbol-table entries.");
+    return;
+  }
+
+  // An MVP object file can have only one table import.
+  if (tableCount != 1) {
+    error(toString(this) +
+          ": multiple table imports, but no corresponding symbol-table "
+          "entries.");
+    return;
+  }
+
+  const WasmImport *tableImport = nullptr;
+      assert(!tableImport);
+      tableImport = &import;
+  assert(tableImport);
+
+  // We can only synthesize a symtab entry for the indirect function table; if
+  // it has an unexpected name or type, assume that it's not actually the
+  // indirect function table.
+  if (tableImport->Field != functionTableName ||
+      tableImport->Table.ElemType != uint8_t(ValType::FUNCREF)) {
+    error(toString(this) + ": table import " + Twine(tableImport->Field) +
+          " is missing a symbol table entry.");
+    return;
   }
-  for (const auto &table : tables) {
-    auto *info = make<llvm::wasm::WasmSymbolInfo>();
-    // Empty name.
-    info->Kind = WASM_SYMBOL_TYPE_TABLE;
-    info->Flags = WASM_SYMBOL_BINDING_LOCAL;
-    info->Flags |= WASM_SYMBOL_VISIBILITY_HIDDEN;
-    info->Flags |= WASM_SYMBOL_NO_STRIP;
-    info->ElementIndex = tableNumber++;
-    LLVM_DEBUG(dbgs() << "Synthesizing symbol for table definition: "
-                      << info->Name << "\n");
-    auto *wasmSym = make<WasmSymbol>(*info, globalType, &table->getType(),
-                                     eventType, signature);
-    symbols.push_back(createDefined(*wasmSym));
-    // Mark live, for the same reasons as for imported tables.
-    symbols.back()->markLive();
-  }
+
+  auto *info = make<WasmSymbolInfo>();
+  info->Name = tableImport->Field;
+  info->ImportModule = tableImport->Module;
+  info->ImportName = tableImport->Field;
+  info->Flags = WASM_SYMBOL_UNDEFINED;
+  info->ElementIndex = 0;
+  LLVM_DEBUG(dbgs() << "Synthesizing symbol for table import: " << info->Name
+                    << "\n");
+  const WasmGlobalType *globalType = nullptr;
+  const WasmEventType *eventType = nullptr;
+  const WasmSignature *signature = nullptr;
+  auto *wasmSym = make<WasmSymbol>(*info, globalType, &tableImport->Table,
+  Symbol *sym = createUndefined(*wasmSym, false);
+  // We're only sure it's a TableSymbol if the createUndefined succeeded.
+  if (errorCount())
+    return;
+  symbols.push_back(sym);
+  // Because there are no TABLE_NUMBER relocs, we can't compute accurate
+  // liveness info; instead, just mark the symbol as always live.
+  sym->markLive();
+
+  // We assume that this compilation unit has unrelocatable references to
+  // this table.
+  config->legacyFunctionTable = true;
 }
 
 void ObjFile::parse(bool ignoreComdats) {
   // Parse a memory buffer as a wasm file.
   LLVM_DEBUG(dbgs() << "Parsing object: " << toString(this) << "\n");
   std::unique_ptr<Binary> bin = CHECK(createBinary(mb), toString(this));
 
   auto *obj = dyn_cast<WasmObjectFile>(bin.get());
for (const WasmGlobal &g : wasmObj->globals())
     globals.emplace_back(make<InputGlobal>(g, this));
 
   // Populate `Events`.
   for (const WasmEvent &e : wasmObj->events())
     events.emplace_back(make<InputEvent>(types[e.Type.SigIndex], e, this));
 
   // Populate `Symbols` based on the symbols in the object.
   symbols.reserve(wasmObj->getNumberOfSymbols());
-  bool haveTableSymbol = false;
+  uint32_t tableSymbolCount = 0;
   for (const SymbolRef &sym : wasmObj->symbols()) {
     const WasmSymbol &wasmSym = wasmObj->getWasmSymbol(sym.getRawDataRefImpl());
     if (wasmSym.isTypeTable())
-      haveTableSymbol = true;
+      tableSymbolCount++;
     if (wasmSym.isDefined()) {
       // createDefined may fail if the symbol is comdat excluded in which case
       // we fall back to creating an undefined symbol
       if (Symbol *d = createDefined(wasmSym)) {
         symbols.push_back(d);
         continue;
       }
     }
     size_t idx = symbols.size();
     symbols.push_back(createUndefined(wasmSym, isCalledDirectly[idx]));
   }
 
-  // As a stopgap measure while implementing table support, if the object file
-  // has table definitions or imports but no table symbols, synthesize symbols
-  // for those tables.  Mark as NO_STRIP to ensure they reach the output file,
-  // even if there are no TABLE_NUMBER relocs against them.
-  if (!haveTableSymbol)
-    synthesizeTableSymbols();
+  addLegacyIndirectFunctionTableIfNeeded(tableSymbolCount);
 }
 
 bool ObjFile::isExcludedByComdat(InputChunk *chunk) const {
   uint32_t c = chunk->getComdat();
   if (c == UINT32_MAX)
     return false;
   return !keptComdats[c];
 }

lld/wasm/Symbols.cpp

 uint32_t TableSymbol::getTableNumber() const {
   if (const auto *t = dyn_cast<DefinedTable>(this))
     return t->table->getAssignedIndex();
   assert(tableNumber != INVALID_INDEX);
   return tableNumber;
 }
 
 void TableSymbol::setTableNumber(uint32_t number) {
+  if (const auto *t = dyn_cast<DefinedTable>(this))
+    return t->table->assignIndex(number);
   LLVM_DEBUG(dbgs() << "setTableNumber " << name << " -> " << number << "\n");
   assert(tableNumber == INVALID_INDEX);
   tableNumber = number;
 }
 
 bool TableSymbol::hasTableNumber() const {
   if (const auto *t = dyn_cast<DefinedTable>(this))
     return t->table->hasAssignedIndex();
   return tableNumber != INVALID_INDEX;
 }
 
 DefinedTable::DefinedTable(StringRef name, uint32_t flags, InputFile *file,
                            InputTable *table)
     : TableSymbol(name, DefinedTableKind, flags, file,
                   table ? &table->getType() : nullptr),
       table(table) {}
 
 const OutputSectionSymbol *SectionSymbol::getOutputSectionSymbol() const {
   assert(section->outputSec && section->outputSec->sectionSym);
   return section->outputSec->sectionSym;
 }
 
 void LazySymbol::fetch() { cast<ArchiveFile>(file)->addMember(&archiveSymbol); }
 
 void LazySymbol::setWeak() {
   flags |= (flags & ~WASM_SYMBOL_BINDING_MASK) | WASM_SYMBOL_BINDING_WEAK;
 }

lld/wasm/SyntheticSections.h

 protected:
 };
 
 class TableSection : public SyntheticSection {
 public:
   TableSection() : SyntheticSection(llvm::wasm::WASM_SEC_TABLE) {}
 
   bool isNeeded() const override { return inputTables.size() > 0; };
+  void assignIndexes() override;
   void writeBody() override;
   void addTable(InputTable *table);
 
   std::vector<InputTable *> inputTables;
 };
 
 class MemorySection : public SyntheticSection {
 public:

lld/wasm/SyntheticSections.cpp

 }
 
 void ImportSection::addImport(Symbol *sym) {
   assert(!isSealed);
   importedSymbols.emplace_back(sym);
   if (auto *f = dyn_cast<FunctionSymbol>(sym))
     f->setFunctionIndex(numImportedFunctions++);
   else if (auto *g = dyn_cast<GlobalSymbol>(sym))
     g->setGlobalIndex(numImportedGlobals++);
   else if (auto *e = dyn_cast<EventSymbol>(sym))
     e->setEventIndex(numImportedEvents++);
   else
     cast<TableSymbol>(sym)->setTableNumber(numImportedTables++);
 }
 
 void ImportSection::writeBody() {
   raw_ostream &os = bodyOutputStream;
 
   writeUleb128(os, getNumImports(), "import count");
 
void FunctionSection::addFunction(InputFunction *func) {
   if (!func->live)
     return;
   uint32_t functionIndex =
       out.importSec->getNumImportedFunctions() + inputFunctions.size();
   inputFunctions.emplace_back(func);
   func->setFunctionIndex(functionIndex);
 }
 
 void TableSection::writeBody() {
   raw_ostream &os = bodyOutputStream;
 
   writeUleb128(os, inputTables.size(), "table count");
   for (const InputTable *table : inputTables)
     writeTableType(os, table->getType());
 }
 
 void TableSection::addTable(InputTable *table) {
   if (!table->live)
     return;
-  uint32_t tableNumber =
-      out.importSec->getNumImportedTables() + inputTables.size();
+  // Some inputs require that the indirect function table be assigned to table
+  // number 0.
+  if (config->legacyFunctionTable &&
+      isa<DefinedTable>(WasmSym::indirectFunctionTable) &&
+      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()));
+          return;
+        }
+      }
+      llvm_unreachable("failed to find conflicting table import");
+    }
+    inputTables.insert(inputTables.begin(), table);
+    return;
+  }
   inputTables.push_back(table);
-  table->assignIndex(tableNumber);
+}
+
+void TableSection::assignIndexes() {
+  uint32_t tableNumber = out.importSec->getNumImportedTables();
+  for (InputTable *t : inputTables)
+    t->assignIndex(tableNumber++);
 }
 
 void MemorySection::writeBody() {
   raw_ostream &os = bodyOutputStream;
 
   bool hasMax = maxMemoryPages != 0 || config->sharedMemory;
   writeUleb128(os, 1, "memory count");
   unsigned flags = 0;
   if (hasMax)
     flags |= WASM_LIMITS_FLAG_HAS_MAX;
   if (config->sharedMemory)
     flags |= WASM_LIMITS_FLAG_IS_SHARED;
   if (config->is64.getValueOr(false))
     flags |= WASM_LIMITS_FLAG_IS_64;
   writeUleb128(os, flags, "memory limits flags");
   writeUleb128(os, numMemoryPages, "initial pages");
   if (hasMax)
     writeUleb128(os, maxMemoryPages, "max pages");
 }

lld/wasm/Writer.cpp

for (auto &feature : required.keys()) {
         error(Twine("Missing target feature '") + feature + "' in " + fileName +
               ", required by " + required[feature] +
               ". Use --no-check-features to suppress.");
     }
   }
 }
 
 static bool shouldImport(Symbol *sym) {
+  if (!sym->isUndefined())
+    return false;
+  if (sym->isWeak() && !config->relocatable)
+    return false;
+  if (!sym->isLive())
+    return false;
+  if (!sym->isUsedInRegularObj)
+    return false;
+
   // We don't generate imports for data symbols. They however can be imported
   // as GOT entries.
   if (isa<DataSymbol>(sym))
     return false;
 
   if (config->relocatable ||
       config->unresolvedSymbols == UnresolvedPolicy::ImportFuncs)
     return true;
   if (config->allowUndefinedSymbols.count(sym->getName()) != 0)
     return true;
   if (auto *g = dyn_cast<UndefinedGlobal>(sym))
     return g->importName.hasValue();
   if (auto *f = dyn_cast<UndefinedFunction>(sym))
     return f->importName.hasValue();
   if (auto *t = dyn_cast<UndefinedTable>(sym))
     return t->importName.hasValue();
 
   return false;
 }
 
 void Writer::calculateImports() {
+  // Some inputs require that the indirect function table be assigned to table
+  // number 0, so if it is present and is an import, allocate it before any
+  // other tables.
+  if (WasmSym::indirectFunctionTable &&
+      shouldImport(WasmSym::indirectFunctionTable))
+    out.importSec->addImport(WasmSym::indirectFunctionTable);
+
   for (Symbol *sym : symtab->getSymbols()) {
-    if (!sym->isUndefined())
-      continue;
-    if (sym->isWeak() && !config->relocatable)
+    if (!shouldImport(sym))
       continue;
-    if (!sym->isLive())
-      continue;
-    if (!sym->isUsedInRegularObj)
+    if (sym == WasmSym::indirectFunctionTable)
       continue;
-    if (shouldImport(sym)) {
-      LLVM_DEBUG(dbgs() << "import: " << sym->getName() << "\n");
-      out.importSec->addImport(sym);
-    }
-  }
-}
 
 void Writer::calculateExports() {
   if (config->relocatable)
     return;
 
   if (!config->relocatable && !config->importMemory)
     out.exportSec->exports.push_back(
         WasmExport{"memory", WASM_EXTERNAL_MEMORY, 0});
for (ObjFile *file : symtab->objectFiles) {
     for (InputTable *table : file->tables)
       out.tableSec->addTable(table);
   }
 
   for (InputTable *table : symtab->syntheticTables)
     out.tableSec->addTable(table);
 
   out.globalSec->assignIndexes();
+  out.tableSec->assignIndexes();
 }
 
 static StringRef getOutputDataSegmentName(StringRef name) {
   // We only support one thread-local segment, so we must merge the segments
   // despite --no-merge-data-segments.
   // We also need to merge .tbss into .tdata so they share the same offsets.
   if (name.startswith(".tdata") || name.startswith(".tbss"))
     return ".tdata";