[lld][WebAssembly] Add new `--import-undefined` option

[sbc100]

Summary

This change revisits https://reviews.llvm.org/D79248 which originally
added support for the --unresolved-symbols flag.

At the time I thought it would make sense to add a third option to this
flag called import-functions but it turns out (as was suspects by one
the reviewers IIRC) that this option can be orthogonal.

Instead I've added a new option called --import-undefined that only
operates on symbols that can be imported (for example, function symbols
can always be imported as opposed to data symbols we can only be
imported when compiling with PIC).

This option gives us the full expresivitiy that emscripten needs to be
able allow reporting of undefined data symbols as well as the option to
disable that.

This change does remove the --unresolved-symbols=import-functions
option, which is been in the codebase now for about a year but I would
be extremely surprised if anyone was using it.

[sbc100]

• typos

[dschuff]

lld/docs/WebAssembly.rst

76

is this missing a ` character or phab's markdown rendering just confused?

lld/docs/WebAssembly.rst

.. option:: --compress-relocations
   potentially accommodate the largest LEB128 value.  This option will cause the
   linker to shrink the code section to remove any padding from the final
   output.  However because it affects code offset, this option is not
   compatible with outputting debug information.
 
 .. option:: --allow-undefined
 
   Allow undefined symbols in linked binary.  This is the legacy
-  flag which corresponds to ``--unresolved-symbols=import-functions``.
+  flag which corresponds to ``--unresolve-symbols=ignore`` +
+  --import-undefined``.
 
 .. option:: --unresolved-symbols=<method>
 
   This is a more full featured version of ``--allow-undefined``.
   The semanatics of the different methods are as follows:
 
   report-all:
 
      Report all unresolved symbols.  This is the default.  Normally the linker
      will generate an error message for each reported unresolved symbol but the
      option ``--warn-unresolved-symbols`` can change this to a warning.
 
   ignore-all:
 
      Resolve all undefined symbols to zero.  For data and function addresses
      this is trivial.  For direct function calls, the linker will generate a
      trapping stub function in place of the undefined function.
 
-  import-functions:
-
-     Generate WebAssembly imports for any undefined functions.  Undefined data
-     symbols are resolved to zero as in ``ignore-all``.  This corresponds to
-     the legacy ``--allow-undefined`` flag.
-
 .. option:: --import-memory
 
   Import memory from the environment.
 
+.. option:: --import-undefined
+
+   Generate WebAssembly imports for undefined symbols, where possible.  For
+   example, for function symbols this is always possible, but in general this
+   is not possible for undefined data symbols.  Undefined data symbols will
+   still be reported as normal (in accordance with ``--unresolved-symbols``).
+
 .. option:: --initial-memory=<value>
 
   Initial size of the linear memory. Default: static data size.
 
 .. option:: --max-memory=<value>
 
   Maximum size of the linear memory. Default: unlimited.
 

lld/test/wasm/unresolved-symbols.s

 # RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown %s -o %t1.o
 
-## Check that %t1.o contains undefined symbol undef.
+## Check that %t1.o contains undefined symbol undef_func.
 # RUN: not wasm-ld %t1.o -o /dev/null 2>&1 | \
 # RUN:   FileCheck -check-prefix=ERRUND %s
-# ERRUND: error: {{.*}}1.o: undefined symbol: undef
+# ERRUND: error: {{.*}}1.o: undefined symbol: undef_func
 
 ## report-all is the default one. Check that we get the same error
 # RUN: not wasm-ld %t1.o -o /dev/null --unresolved-symbols=report-all 2>&1 | \
 # RUN:   FileCheck -check-prefix=ERRUND %s
 
 ## Error out if unknown option value was set.
 # RUN: not wasm-ld %t1.o -o /dev/null --unresolved-symbols=xxx 2>&1 | \
 # RUN:   FileCheck -check-prefix=ERR1 %s

 # IGNORE-NEXT:        Name:            undefined
 # IGNORE-NEXT:      - Index:           1
 # IGNORE-NEXT:        Name:            _start
 # IGNORE-NEXT:      - Index:           2
 # IGNORE-NEXT:        Name:            get_data_addr
 # IGNORE-NEXT:      - Index:           3
 # IGNORE-NEXT:        Name:            get_func_addr
 
-## import-functions should not produce errors and should resolve in
-# imports for the missing functions but not the missing data symbols.
-# `--allow-undefined` should behave exactly the same.
-# RUN: wasm-ld %t1.o -o %t3.wasm --unresolved-symbols=import-functions
+## --import-undefined should handle unresolved funtions symbols
+# by importing them but still report errors/warning for missing data symbols.
+# `--allow-undefined` should behave like `--import-undefined` +
+# `--unresolve-symbols=ignore`
+# RUN: wasm-ld %t1.o -o %t3.wasm --import-undefined --unresolved-symbols=ignore-all
 # RUN: obj2yaml %t3.wasm | FileCheck -check-prefix=IMPORT %s
 #      IMPORT:  - Type:            IMPORT
 # IMPORT-NEXT:    Imports:
 # IMPORT-NEXT:      - Module:          env
-# IMPORT-NEXT:        Field:           undef
+# IMPORT-NEXT:        Field:           undef_func
 # IMPORT-NEXT:        Kind:            FUNCTION
 # IMPORT-NEXT:        SigIndex:        0
 # IMPORT-NEXT:  - Type:            FUNCTION
 
+## Check that --import-undefined reports unresolved data symbols.
+# RUN: not wasm-ld %t1.o -o %t3.wasm --import-undefined --unresolved-symbols=report-all 2>&1 | FileCheck -check-prefix=IMPORTUNDEFINED %s
+# IMPORTUNDEFINED-NOT: error: {{.*}}1.o: undefined symbol: undef_func
+# IMPORTUNDEFINED: error: {{.*}}1.o: undefined symbol: undef_data
+
 ## Do not report undefines if linking relocatable.
 # RUN: wasm-ld -r %t1.o -o %t4.wasm --unresolved-symbols=report-all
 # RUN: llvm-readobj %t4.wasm > /dev/null 2>&1
 
 .globl _start
 _start:
     .functype _start () -> ()
-    call undef
+    call undef_func
     call get_data_addr
     call get_func_addr
     end_function
 
 .globl get_data_addr
 get_data_addr:
     .functype get_data_addr () -> (i32)
     i32.const undef_data
     return
     end_function
 
 .globl get_func_addr
 get_func_addr:
     .functype get_func_addr () -> (i32)
-    i32.const undef
+    i32.const undef_func
     return
     end_function
 
-.functype undef () -> ()
+.functype undef_func () -> ()

lld/wasm/Config.h

 #include "llvm/ADT/StringSet.h"
 #include "llvm/BinaryFormat/Wasm.h"
 #include "llvm/Support/CachePruning.h"
 
 namespace lld {
 namespace wasm {
 
 // For --unresolved-symbols.
-// The `ImportFuncs` mode is an additional mode that corresponds to the
-// --allow-undefined flag which turns undefined functions in imports
-// as opposed ed to Ignore or Warn which turn them into unreachables.
-enum class UnresolvedPolicy { ReportError, Warn, Ignore, ImportFuncs };
+enum class UnresolvedPolicy { ReportError, Warn, Ignore };
 
 // This struct contains the global configuration for the linker.
 // Most fields are direct mapping from the command line options
 // and such fields have the same name as the corresponding options.
 // Most fields are initialized by the driver.
 struct Configuration {
   bool bsymbolic;
   bool checkFeatures;
   bool compressRelocations;
   bool demangle;
   bool disableVerify;
   bool experimentalPic;
   bool emitRelocs;
   bool exportAll;
   bool exportDynamic;
   bool exportTable;
   bool growableTable;
   bool gcSections;
   bool importMemory;
   bool sharedMemory;
   bool importTable;
+  bool importUndefined;
   llvm::Optional<bool> is64;
   bool mergeDataSegments;
   bool pie;
   bool printGcSections;
   bool relocatable;
   bool saveTemps;
   bool shared;
   bool stripAll;

lld/wasm/Driver.cpp

UnresolvedPolicy errorOrWarn = args.hasFlag(OPT_error_unresolved_symbols,
                                               OPT_warn_unresolved_symbols, true)
                                      ? UnresolvedPolicy::ReportError
                                      : UnresolvedPolicy::Warn;
 
   if (auto *arg = args.getLastArg(OPT_unresolved_symbols)) {
     StringRef s = arg->getValue();
     if (s == "ignore-all")
       return UnresolvedPolicy::Ignore;
-    if (s == "import-functions")
-      return UnresolvedPolicy::ImportFuncs;
     if (s == "report-all")
       return errorOrWarn;
     error("unknown --unresolved-symbols value: " + s);
   }
 
-  // Legacy --allow-undefined flag which is equivalent to
-  // --unresolve-symbols=ignore-all
-  if (args.hasArg(OPT_allow_undefined))
-    return UnresolvedPolicy::ImportFuncs;
-
   return errorOrWarn;
 }
 
 // Initializes Config members by the command line options.
 static void readConfigs(opt::InputArgList &args) {
   config->bsymbolic = args.hasArg(OPT_Bsymbolic);
   config->checkFeatures =
       args.hasFlag(OPT_check_features, OPT_no_check_features, true);
   config->compressRelocations = args.hasArg(OPT_compress_relocations);
   config->demangle = args.hasFlag(OPT_demangle, OPT_no_demangle, true);
   config->disableVerify = args.hasArg(OPT_disable_verify);
   config->emitRelocs = args.hasArg(OPT_emit_relocs);
   config->experimentalPic = args.hasArg(OPT_experimental_pic);
   config->entry = getEntry(args);
   config->exportAll = args.hasArg(OPT_export_all);
   config->exportTable = args.hasArg(OPT_export_table);
   config->growableTable = args.hasArg(OPT_growable_table);
   errorHandler().fatalWarnings =
       args.hasFlag(OPT_fatal_warnings, OPT_no_fatal_warnings, false);
   config->importMemory = args.hasArg(OPT_import_memory);
   config->sharedMemory = args.hasArg(OPT_shared_memory);
   config->importTable = args.hasArg(OPT_import_table);
+  config->importUndefined = args.hasArg(OPT_import_undefined);
   config->ltoo = args::getInteger(args, OPT_lto_O, 2);
   config->ltoPartitions = args::getInteger(args, OPT_lto_partitions, 1);
   config->ltoNewPassManager =
       args.hasFlag(OPT_no_lto_legacy_pass_manager, OPT_lto_legacy_pass_manager,
                    LLVM_ENABLE_NEW_PASS_MANAGER);
   config->ltoDebugPassManager = args.hasArg(OPT_lto_debug_pass_manager);
   config->mapFile = args.getLastArgValue(OPT_Map);
   config->optimize = args::getInteger(args, OPT_O, 1);
static void readConfigs(opt::InputArgList &args) {
 
   if (auto *arg = args.getLastArg(OPT_features)) {
     config->features =
         llvm::Optional<std::vector<std::string>>(std::vector<std::string>());
     for (StringRef s : arg->getValues())
       config->features->push_back(std::string(s));
   }
 
+  // Legacy --allow-undefined flag which is equivalent to
+  // --unresolve-symbols=ignore + --import-undefined
+  if (args.hasArg(OPT_allow_undefined)) {
+    config->importUndefined = true;
+    config->unresolvedSymbols = UnresolvedPolicy::Ignore;
+  }
+
   if (args.hasArg(OPT_print_map))
     config->mapFile = "-";
 }
 
 // Some Config members do not directly correspond to any particular
 // command line options, but computed based on other Config values.
 // This function initialize such members. See Config.h for the details
 // of these values.
if (config->relocatable) {
     if (config->growableTable)
       error("--relocatable is incompatible with --growable-table");
     // Ignore any --import-table, as it's redundant.
     config->importTable = true;
   }
 
   if (config->shared) {
     config->importMemory = true;
-    config->unresolvedSymbols = UnresolvedPolicy::ImportFuncs;
+    config->importUndefined = true;
+    config->unresolvedSymbols = UnresolvedPolicy::Ignore;
   }
 }
 
 // Some command line options or some combinations of them are not allowed.
 // This function checks for such errors.
 static void checkOptions(opt::InputArgList &args) {
   if (!config->stripDebug && !config->stripAll && config->compressRelocations)
     error("--compress-relocations is incompatible with output debug"

lld/wasm/Options.td

defm wrap: Eq<"wrap", "Use wrapper functions for symbol">,
   MetaVarName<"<symbol>=<symbol>">;
 
 def z: JoinedOrSeparate<["-"], "z">, MetaVarName<"<option>">,
   HelpText<"Linker option extensions">;
 
 // The follow flags are unique to wasm
 
 def allow_undefined: F<"allow-undefined">,
-  HelpText<"Allow undefined symbols in linked binary">;
+  HelpText<"Allow undefined symbols in linked binary. This options is equivelant "
+           "to --import-undefined and --unresolved-symbols=ignore-all">;
+
+def import_undefined: F<"import-undefined">,
+  HelpText<"Turn undefined symbols into imports where possible">;
 
 def allow_undefined_file: J<"allow-undefined-file=">,
   HelpText<"Allow symbols listed in <file> to be undefined in linked binary">;
 
 def allow_undefined_file_s: Separate<["-"], "allow-undefined-file">,
   Alias<allow_undefined_file>;
 
 defm export: Eq<"export", "Force a symbol to be exported">;

lld/wasm/Relocations.cpp

if (sym->isDefined() && (!config->shared || config->bsymbolic))
     return false;
   return true;
 }
 
 static bool allowUndefined(const Symbol* sym) {
   // Undefined functions and globals with explicit import name are allowed to be
   // undefined at link time.
   if (auto *f = dyn_cast<UndefinedFunction>(sym))
-    if (f->importName)
+    if (f->importName || config->importUndefined)
       return true;
   if (auto *g = dyn_cast<UndefinedGlobal>(sym))
     if (g->importName)
       return true;
   if (auto *g = dyn_cast<UndefinedGlobal>(sym))
     if (g->importName)
       return true;
   return config->allowUndefinedSymbols.count(sym->getName()) != 0;
 }
 
 static void reportUndefined(Symbol *sym) {
   if (!allowUndefined(sym)) {
     switch (config->unresolvedSymbols) {
     case UnresolvedPolicy::ReportError:
       error(toString(sym->getFile()) + ": undefined symbol: " + toString(*sym));
       break;
     case UnresolvedPolicy::Warn:
       warn(toString(sym->getFile()) + ": undefined symbol: " + toString(*sym));
       break;
     case UnresolvedPolicy::Ignore:
+      LLVM_DEBUG(dbgs() << "ignoring undefined symbol: " + toString(*sym) +
+                               "\n");
+      if (!config->importUndefined) {
         if (auto *f = dyn_cast<UndefinedFunction>(sym)) {
           if (!f->stubFunction) {
-          LLVM_DEBUG(dbgs()
-                     << "ignoring undefined symbol: " + toString(*sym) + "\n");
             f->stubFunction = symtab->createUndefinedStub(*f->getSignature());
             f->stubFunction->markLive();
             // Mark the function itself as a stub which prevents it from being
             // assigned a table entry.
             f->isStub = true;
           }
         }
-      break;
-    case UnresolvedPolicy::ImportFuncs:
+      }
       break;
     }
   }
 }
 
 static void addGOTEntry(Symbol *sym) {
   if (requiresGOTAccess(sym))
     out.importSec->addGOTEntry(sym);

lld/wasm/Writer.cpp

static bool shouldImport(Symbol *sym) {
   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->isPic || config->relocatable) ||
-      config->unresolvedSymbols == UnresolvedPolicy::ImportFuncs)
+  if (config->isPic || config->relocatable || config->importUndefined)
     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))