[lld][WebAssembly] Fix resolveIndirectFunctionTable for relocatable output

[wingo]

Summary

For relocatable output that needs the indirect function table, identify
the well-known function table. This allows us to properly fix the
limits on the imported table, and in a followup will allow the element
section to reference the indirect function table even if it's not
assigned to table number 0. Adapt tests for import reordering.

[wingo]

Fix typo

[sbc100]

Does this mean the --relocatable always produces output with an entry in the symbol table for __indirect_function_table? i.e. does the linker always produce non-MVP object files?

lld/wasm/Driver.cpp

824

Would it be better to ensure that importTable is always set when config->relocatable. Anything else would be nonsensical I think.

We should also error out if -r is used with any of the table control options (--export-table, --import-table, --growable-table all make no sense with --relocatable.

Also why the extra existing here? In --relocatable mode we never want to end up calling createDefinedIndirectFunctionTable so we need to make sure that this branch of the if I think, no?

[wingo]

In D96770#2565766, @sbc100 wrote:

Does this mean the --relocatable always produces output with an entry in the symbol table for __indirect_function_table? i.e. does the linker always produce non-MVP object files?

Yeah apparently so. Though if no input needs an indirect function table, specifying --relocatable won't cause one to be produced. Otherwise yes a symtab entry for __indirect_function_table is residualized. Note however that the file is still conforming to the MVP spec (i.e no 5-byte relocs for call_indirect).

[wingo]

lld/wasm/Driver.cpp

824

I can make this change, I just thought you wanted a precise mapping between command-line args and the importTable config flag.

The extra "existing" is because both importTable and exportTable force the table to be present, even if not needed. That's not what we want with relocatable.

[wingo]

lld/wasm/Driver.cpp

824

Sorry, following up here -- can't just set importTable when relocatable because that could cause the function table to be created when not needed. Advice welcome; as is I don't see an obvious change to make.

[sbc100]

lld/wasm/Driver.cpp

824

I think if fine for --import-table to mean "import table if you need a table at all".

Export table is different because the embedded may want access to the exported table. For the import, there no point in imported a table one does not user.

[sbc100]

lld/wasm/Driver.cpp

824

I think if fine for --import-table to mean "import table if you need a table at all".

Export table is different because the embedded may want access to the exported table. For the import, there no point in imported a table one does not user.

So this would become:

// No point in importing a table if we don't have any references to one.
if (config->importTable && existing)

And --relocatable could set config->importTable internally?

826

Can an error when --export-table is passed in --relocatable mode so we know we never get here.
Perhaps even an assert on line 838 too. In fact, perhaps we can land a separate change that no table
flags should be passed in --relocatable mode?

[wingo]

Rebase on top of D96872

[wingo]

Thanks for feedback :) Fixed nits, I think. Tests pass here.

[sbc100]

lgtm with a test for the new error(s).

lld/wasm/Driver.cpp

475

Can you do the same for growableTable? Probably good to have a test too.

[wingo]

Add error tests, and mark -r incompatible with --growable-table

lld/test/wasm/locals-duplicate.test

 ; RELOC-NEXT:    Imports:
 ; RELOC-NEXT:      - Module:          env
 ; RELOC-NEXT:        Field:           __indirect_function_table
 ; RELOC-NEXT:        Kind:            TABLE
 ; RELOC-NEXT:        Table:
 ; RELOC-NEXT:          Index:           0
 ; RELOC-NEXT:          ElemType:        FUNCREF
 ; RELOC-NEXT:          Limits:
-; RELOC-NEXT:            Initial:         0x3
+; RELOC-NEXT:            Initial:         0x7
 ; RELOC-NEXT:   - Type:            FUNCTION
 ; RELOC-NEXT:     FunctionTypes:   [ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
 ; RELOC-NEXT:                        0, 0 ]
 ; RELOC-NEXT:   - Type:            MEMORY
 ; RELOC-NEXT:     Memories:
 ; RELOC-NEXT:       - Initial:         0x1
 ; RELOC-NEXT:   - Type:            ELEM
 ; RELOC-NEXT:     Segments:

lld/test/wasm/relocatable.ll

 ; CHECK-NEXT:         ReturnTypes:
 ; CHECK-NEXT:           - I32
 ; CHECK-NEXT:       - Index:           2
 ; CHECK-NEXT:         ParamTypes:
 ; CHECK-NEXT:         ReturnTypes:     []
 ; CHECK-NEXT:   - Type:            IMPORT
 ; CHECK-NEXT:     Imports:
 ; 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:         0x4
+; CHECK-NEXT:       - Module:          env
 ; CHECK-NEXT:         Field:           puts
 ; CHECK-NEXT:         Kind:            FUNCTION
 ; CHECK-NEXT:         SigIndex:        0
 ; CHECK-NEXT:       - Module:          env
 ; CHECK-NEXT:         Field:           foo_import
 ; CHECK-NEXT:         Kind:            FUNCTION
 ; CHECK-NEXT:         SigIndex:        1
 ; CHECK-NEXT:       - Module:          env
 ; CHECK-NEXT:         Field:           bar_import
 ; 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:         0x3
 ; CHECK-NEXT:   - Type:            FUNCTION
 ; CHECK-NEXT:     FunctionTypes:   [ 2, 1, 1, 2 ]
 ; CHECK-NEXT:   - Type:            MEMORY
 ; CHECK-NEXT:     Memories:
 ; CHECK-NEXT:      - Initial:         0x1
 ; CHECK-NEXT:   - Type:            ELEM
 ; CHECK-NEXT:     Segments:
 ; CHECK-NEXT:       - Offset:

lld/test/wasm/weak-alias.ll

 ; RELOC-NEXT:         ReturnTypes:     []
 ; RELOC-NEXT:       - Index:           1
 ; RELOC-NEXT:         ParamTypes:      []
 ; RELOC-NEXT:         ReturnTypes:
 ; RELOC-NEXT:           - I32
 ; RELOC-NEXT:   - Type:            IMPORT
 ; RELOC-NEXT:     Imports:
 ; RELOC-NEXT:       - Module:          env
-; RELOC-NEXT:         Field:           __stack_pointer
-; RELOC-NEXT:         Kind:            GLOBAL
-; RELOC-NEXT:         GlobalType:      I32
-; RELOC-NEXT:         GlobalMutable:   true
-; RELOC-NEXT:       - Module:          env
 ; RELOC-NEXT:         Field:           __indirect_function_table
 ; RELOC-NEXT:         Kind:            TABLE
 ; RELOC-NEXT:         Table:
 ; RELOC-NEXT:           Index:           0
 ; RELOC-NEXT:           ElemType:        FUNCREF
 ; RELOC-NEXT:           Limits:
-; RELOC-NEXT:             Initial:         0x1
+; RELOC-NEXT:             Initial:         0x2
+; RELOC-NEXT:       - Module:          env
+; RELOC-NEXT:         Field:           __stack_pointer
+; RELOC-NEXT:         Kind:            GLOBAL
+; RELOC-NEXT:         GlobalType:      I32
+; RELOC-NEXT:         GlobalMutable:   true
 ; RELOC-NEXT:   - Type:            FUNCTION
 ; RELOC-NEXT:     FunctionTypes:   [ 0, 1, 1, 1, 1, 1 ]
 ; RELOC-NEXT:   - Type:            MEMORY
 ; RELOC-NEXT:     Memories:
 ; RELOC-NEXT:       - Initial:         0x0
 ; RELOC-NEXT:   - Type:            ELEM
 ; RELOC-NEXT:     Segments:
 ; RELOC-NEXT:       - Offset:

lld/wasm/Driver.cpp

static void setConfigs() {
   config->isPic = config->pie || config->shared;
 
   if (config->isPic) {
     if (config->exportTable)
       error("-shared/-pie is incompatible with --export-table");
     config->importTable = true;
   }
 
+  if (config->relocatable) {
+    if (config->exportTable)
+      error("--relocatable is incompatible with --export-table");
+    // Ignore any --import-table, as it's redundant.
+    config->importTable = true;
+  }
+
   if (config->shared) {
     config->importMemory = true;
     config->unresolvedSymbols = UnresolvedPolicy::ImportFuncs;
   }
 }
 
 // Some command line options or some combinations of them are not allowed.
 // This function checks for such errors.
if (existing) {
     }
     if (existing->isDefined()) {
       error(Twine("reserved symbol must not be defined in input files: `") +
             functionTableName + "`");
       return nullptr;
     }
   }
 
   if (config->importTable) {
     return cast_or_null<TableSymbol>(existing);
   } 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.
if (entrySym)
     entrySym->setHidden(false);
 
   if (errorCount())
     return;
 
   // Do size optimizations: garbage collection
   markLive();
 
-  if (!config->relocatable) {
-    // Provide the indirect funciton table if needed.
-    WasmSym::indirectFunctionTable = resolveIndirectFunctionTable();
 
-    if (errorCount())
-      return;
-  }
 
   // Write the result to the file.
   writeResult();
 }
 
 } // namespace wasm
 } // namespace lld