[lld][WebAssembly] Implement --unresolved-symbols

[sbc100]

Summary

This is a more full featured version of --allow-undefined.
The semantics 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 the

The plan is to followup with a new mode called import-all which allows
for statically linked binaries to refer to both data and functions
symbols from the embedder.

[sbc100]

rebase

[sbc100]

• typos

[sbc100]

remove some tracing

[sbc100]

• cleanup

[sbc100]

Downstream emscripten issue: https://github.com/emscripten-core/emscripten/issues/12682
Design doc: https://docs.google.com/document/d/1viN3qTS5QzeDP7NR0pGg9D5JuuOrQzsORumMswYGpsA/edit?resourcekey=0-2Rnysxch2EuXNT3cvoD1MA

[MaskRay]

In ELF, --unresolved-symbols= sets --[no-]allow-shlib-undefined and -z (un)defs simultaneously.

• --[no-]allow-shlib-undefined: whether an unresolved undefined symbol from a shared object should be errored
• -z (un)defs: whether an unresolved undefined symbol from a regular object should be errored.

Does the wasm usage fit well in the existing practice?

[sbc100]

In D79248#2396409, @MaskRay wrote:

In ELF, --unresolved-symbols= sets --[no-]allow-shlib-undefined and -z (un)defs simultaneously.

• --[no-]allow-shlib-undefined: whether an unresolved undefined symbol from a shared object should be errored
• -z (un)defs: whether an unresolved undefined symbol from a regular object should be errored.

Does the wasm usage fit well in the existing practice?

The support for shared libraries in wasm-ld, and indeed the entire PIC ABI, is experimental (in fact, it will warn until you also pass --experimental-pic).

In particular, support for linking against shared libraries does not yet

In D79248#2396409, @MaskRay wrote:

In ELF, --unresolved-symbols= sets --[no-]allow-shlib-undefined and -z (un)defs simultaneously.

• --[no-]allow-shlib-undefined: whether an unresolved undefined symbol from a shared object should be errored
• -z (un)defs: whether an unresolved undefined symbol from a regular object should be errored.

Does the wasm usage fit well in the existing practice?

Yes. While we don't currently support either of those flags I believe the implementation of ignore-all here will match -z defs, so I think that adding support for -z (un)defs would be trivial if decide do add that flag in the future.

Regarding --[no-]allow-shlib-undefined, our shared library support is still experimental is rather limited. Using it currently requires the --experimental-pic flag.

BTW, looking at the ELF code I don't see --unresolved-symbols currently effecting `--allow-shlib-undefined.

In ELF/Driver.cpp we have:

config->allowShlibUndefined =                                                  
      args.hasFlag(OPT_allow_shlib_undefined, OPT_no_allow_shlib_undefined,                        
                   args.hasArg(OPT_shared));

And this doesn't look like it gets updated based on --unresolved-symbols.

[sbc100]

clang-tidy

[MaskRay]

In D79248#2397526, @sbc100 wrote:

In D79248#2396409, @MaskRay wrote:

In ELF, --unresolved-symbols= sets --[no-]allow-shlib-undefined and -z (un)defs simultaneously.

• --[no-]allow-shlib-undefined: whether an unresolved undefined symbol from a shared object should be errored
• -z (un)defs: whether an unresolved undefined symbol from a regular object should be errored.

Does the wasm usage fit well in the existing practice?

The support for shared libraries in wasm-ld, and indeed the entire PIC ABI, is experimental (in fact, it will warn until you also pass --experimental-pic).

In particular, support for linking against shared libraries does not yet

In D79248#2396409, @MaskRay wrote:

In ELF, --unresolved-symbols= sets --[no-]allow-shlib-undefined and -z (un)defs simultaneously.

• --[no-]allow-shlib-undefined: whether an unresolved undefined symbol from a shared object should be errored
• -z (un)defs: whether an unresolved undefined symbol from a regular object should be errored.

Does the wasm usage fit well in the existing practice?

Yes. While we don't currently support either of those flags I believe the implementation of ignore-all here will match -z defs, so I think that adding support for -z (un)defs would be trivial if decide do add that flag in the future.

Regarding --[no-]allow-shlib-undefined, our shared library support is still experimental is rather limited. Using it currently requires the --experimental-pic flag.

BTW, looking at the ELF code I don't see --unresolved-symbols currently effecting `--allow-shlib-undefined.

In ELF/Driver.cpp we have:

config->allowShlibUndefined =                                                  
      args.hasFlag(OPT_allow_shlib_undefined, OPT_no_allow_shlib_undefined,                        
                   args.hasArg(OPT_shared));

And this doesn't look like it gets updated based on --unresolved-symbols.

You are right. I knew this when investigating https://reviews.llvm.org/D67479#1667256
It is of very low priority because --unresolved-symbols= has too few use cases. Now wasm is adding this option I'll make the ELF side semantics clearer: D91510

[sbc100]

• -z (un)defs: whether an unresolved undefined symbol from a regular object should be errored.

I wish I had known about -z undefs when I first created wasm-ld.. I could have avoided inventing --allow-undefined

[sbc100]

@dschuff ptal.

[sbc100]

@MaskRay any objections to landing this?

BTW, how to you feel about being a reviewer on wasm-ld change in general.. if you are OK with it I could use a reviewer for my work here now that @Rui is less active.

[MaskRay]

In D79248#2400523, @sbc100 wrote:

@MaskRay any objections to landing this?

BTW, how to you feel about being a reviewer on wasm-ld change in general.. if you are OK with it I could use a reviewer for my work here now that @Rui is less active.

I don't mind but you'll throw out some learning resources on me.. (Persoanlly I do want to study more binary formats) For now I guess the best I can provide is how some options are used in ELF..

I just committed D91510 for ELF - personally I think the option is a bit awkward to use as it changes two bits simultaneously. There are more values in the wasm-lld version: do you think these values should all be mutually exclusive. If yes, this looks fine to me.

[sbc100]

In D79248#2400768, @MaskRay wrote:

In D79248#2400523, @sbc100 wrote:

@MaskRay any objections to landing this?

BTW, how to you feel about being a reviewer on wasm-ld change in general.. if you are OK with it I could use a reviewer for my work here now that @Rui is less active.

I don't mind but you'll throw out some learning resources on me.. (Persoanlly I do want to study more binary formats) For now I guess the best I can provide is how some options are used in ELF..

That sounds good. I can include wasm-specific people too in all my changes, but its good to have somebody with deep knowledge on the other linker take a look at changes like this for sure.

I just committed D91510 for ELF - personally I think the option is a bit awkward to use as it changes two bits simultaneously. There are more values in the wasm-lld version: do you think these values should all be mutually exclusive. If yes, this looks fine to me.

That an interesting point. Yes I am certainly imagining the various modes being mutually exclusive. Unlike ELF we have a couple of different ways that we want to be able to handle undefined symbols accepting various value here makes sense to me. Just doing -z undefs would not be enough in general as I would also want to know *how* to handle those undefined symbols. Once the followup to this change lands there will be 3 different ways to deal with them once we know we want to allow them.

[sbc100]

@dschuff can I get an final lgtm from you?

[dschuff]

lld/docs/WebAssembly.rst

97 ↗

(On Diff #305524)

"corresponds the" -> "corresponds to the"

98 ↗

(On Diff #305524)

The copy of this text in the commit message is missing this line.

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.
+# RUN: not wasm-ld %t1.o -o /dev/null 2>&1 | \
+# RUN:   FileCheck -check-prefix=UNDCHECK %s
+# UNDCHECK: error: {{.*}}1.o: undefined symbol: undef
+
+## 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
+# ERR1: unknown --unresolved-symbols value: xxx
+## Check alias.
+# RUN: not wasm-ld %t1.o -o /dev/null --unresolved-symbols xxx 2>&1 | \
+# RUN:   FileCheck -check-prefix=ERR1 %s
+
+## Ignore all should not produce error for symbols from object except
+## case when --no-undefined specified.
+# RUN: wasm-ld %t1.o -o %t1_1 --unresolved-symbols=ignore-all
+# RUN: llvm-readobj %t1_1 > /dev/null 2>&1
+
+# RXUN: not wasm-ld %t2.o -o /dev/null --unresolved-symbols=ignore-all --no-undefined 2>&1 | \
+# RXUN:   FileCheck -check-prefix=ERRUND %s
+# ERRUND: error: undefined symbol: undef
+# ERRUND: >>> referenced by {{.*}}:(.text+0x1)
+
+## Also ignore all should not produce error for symbols from DSOs.
+# RXUN: wasm-ld %t1.o %t.so -o %t1_3 --allow-shlib-undefined --unresolved-symbols=ignore-all
+# RXUN: llvm-readobj %t1_3 > /dev/null 2>&1
+
+## Do not report undefines if linking relocatable.
+# RXUN: wasm-ld -r %t1.o %t2.o -o %t5 --unresolved-symbols=report-all
+# RXUN: llvm-readobj %t5 > /dev/null 2>&1
+
+.globl _start
+_start:
+    .functype _start () -> ()
+    call undef
+    end_function
+
+.functype undef () -> ()

lld/wasm/Config.h

 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/BinaryFormat/Wasm.h"
 #include "llvm/Support/CachePruning.h"
 
 namespace lld {
 namespace wasm {
 
+// For --unresolved-symbols.
+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 allowUndefined;
   bool checkFeatures;
   bool compressRelocations;
   bool demangle;
   bool disableVerify;
   bool emitRelocs;
   bool exportAll;
   bool exportDynamic;
   bool exportTable;
struct Configuration {
   uint64_t globalBase;
   uint64_t initialMemory;
   uint64_t maxMemory;
   uint64_t zStackSize;
   unsigned ltoPartitions;
   unsigned ltoo;
   unsigned optimize;
   llvm::StringRef thinLTOJobs;
+  UnresolvedPolicy unresolvedSymbols;
 
   llvm::StringRef entry;
   llvm::StringRef outputFile;
   llvm::StringRef thinLTOCacheDir;
 
   llvm::StringSet<> allowUndefinedSymbols;
   llvm::StringSet<> exportedSymbols;
   std::vector<llvm::StringRef> searchPaths;

lld/wasm/Driver.cpp

if (args.hasArg(OPT_shared))
       return "__wasm_call_ctors";
     return "_start";
   }
   if (arg->getOption().getID() == OPT_no_entry)
     return "";
   return arg->getValue();
 }
 
+// Determines what we should do if there are remaining unresolved
+// symbols after the name resolution.
+static UnresolvedPolicy getUnresolvedSymbolPolicy(opt::InputArgList &args) {
+  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 == "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::Ignore;
+
+  return errorOrWarn;
+}
+
 // Initializes Config members by the command line options.
 static void readConfigs(opt::InputArgList &args) {
-  config->allowUndefined = args.hasArg(OPT_allow_undefined);
   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->entry = getEntry(args);
   config->exportAll = args.hasArg(OPT_export_all);
static void readConfigs(opt::InputArgList &args) {
   config->stripAll = args.hasArg(OPT_strip_all);
   config->stripDebug = args.hasArg(OPT_strip_debug);
   config->stackFirst = args.hasArg(OPT_stack_first);
   config->trace = args.hasArg(OPT_trace);
   config->thinLTOCacheDir = args.getLastArgValue(OPT_thinlto_cache_dir);
   config->thinLTOCachePolicy = CHECK(
       parseCachePruningPolicy(args.getLastArgValue(OPT_thinlto_cache_policy)),
       "--thinlto-cache-policy: invalid cache policy");
+  config->unresolvedSymbols = getUnresolvedSymbolPolicy(args);
   errorHandler().verbose = args.hasArg(OPT_verbose);
   LLVM_DEBUG(errorHandler().verbose = true);
 
   config->initialMemory = args::getInteger(args, OPT_initial_memory, 0);
   config->globalBase = args::getInteger(args, OPT_global_base, 1024);
   config->maxMemory = args::getInteger(args, OPT_max_memory, 0);
   config->zStackSize =
       args::getZOptionValue(args, OPT_z, "stack-size", WasmPageSize);
static void setConfigs() {
   if (config->isPic) {
     if (config->exportTable)
       error("-shared/-pie is incompatible with --export-table");
     config->importTable = true;
   }
 
   if (config->shared) {
     config->importMemory = true;
-    config->allowUndefined = 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"
void LinkerDriver::link(ArrayRef<const char *> argsArr) {
   // Apply symbol renames for -wrap.
   if (!wrapped.empty())
     wrapSymbols(wrapped);
 
   for (auto *arg : args.filtered(OPT_export)) {
     Symbol *sym = symtab->find(arg->getValue());
     if (sym && sym->isDefined())
       sym->forceExport = true;
-    else if (!config->allowUndefined)
+    else if (config->unresolvedSymbols == UnresolvedPolicy::ReportError)
       error(Twine("symbol exported via --export not found: ") +
             arg->getValue());
+    else if (config->unresolvedSymbols == UnresolvedPolicy::Warn)
+      warn(Twine("symbol exported via --export not found: ") + arg->getValue());
   }
 
   if (!config->relocatable) {
     // Add synthetic dummies for weak undefined functions.  Must happen
     // after LTO otherwise functions may not yet have signatures.
     symtab->handleWeakUndefines();
   }
 

lld/wasm/Options.td

def compress_relocations: F<"compress-relocations">,
   HelpText<"Compress the relocation targets in the code section.">;
 
 defm demangle: B<"demangle",
     "Demangle symbol names",
     "Do not demangle symbol names">;
 
 def emit_relocs: F<"emit-relocs">, HelpText<"Generate relocations in output">;
 
+def error_unresolved_symbols: F<"error-unresolved-symbols">,
+  HelpText<"Report unresolved symbols as errors">;
+
 defm export_dynamic: B<"export-dynamic",
     "Put symbols in the dynamic symbol table",
     "Do not put symbols in the dynamic symbol table (default)">;
 
 def entry: S<"entry">, MetaVarName<"<entry>">,
   HelpText<"Name of entry point symbol">;
 
 def error_limit: J<"error-limit=">,
: Eq<"threads", "Number of threads. '1' disables multi-threading. By "
                     "default all available hardware threads are used">;
 
 def trace: F<"trace">, HelpText<"Print the names of the input files">;
 
 defm trace_symbol: Eq<"trace-symbol", "Trace references to symbols">;
 
 defm undefined: Eq<"undefined", "Force undefined symbol during linking">;
 
+defm unresolved_symbols:
+  Eq<"unresolved-symbols", "Determine how to handle unresolved symbols">;
+
 def v: Flag<["-"], "v">, HelpText<"Display the version number">;
 
 def verbose: F<"verbose">, HelpText<"Verbose mode">;
 
 def version: F<"version">, HelpText<"Display the version number and exit">;
 
-def z: JoinedOrSeparate<["-"], "z">, MetaVarName<"<option>">,
-  HelpText<"Linker option extensions">;
+def warn_unresolved_symbols: F<"warn-unresolved-symbols">,
+  HelpText<"Report unresolved symbols as warnings">;
 
 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">;
 
 def allow_undefined_file: J<"allow-undefined-file=">,
   HelpText<"Allow symbols listed in <file> to be undefined in linked binary">;
 

lld/wasm/Relocations.cpp

 }
 
 static bool allowUndefined(const Symbol* sym) {
   // Undefined functions with explicit import name are allowed to be undefined
   // at link time.
   if (auto *F = dyn_cast<UndefinedFunction>(sym))
     if (F->importName)
       return true;
-  return (config->allowUndefined ||
-          config->allowUndefinedSymbols.count(sym->getName()) != 0);
+  return config->allowUndefinedSymbols.count(sym->getName()) != 0;
 }
 
 static void reportUndefined(const Symbol* sym) {
   assert(sym->isUndefined());
   assert(!sym->isWeak());
-  if (!allowUndefined(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:
+      break;
+    }
+  }
 }
 
 static void addGOTEntry(Symbol *sym) {
   // In PIC mode a GOT entry is an imported global that the dynamic linker
   // will assign.
   // In non-PIC mode (i.e. when code compiled as fPIC is linked into a static
   // binary) we create an internal wasm global with a fixed value that takes the
   // place of th GOT entry and effectivly acts as an i32 const. This can