Wasm entrypoint changes #1 (add --undefined argument to LLD)

[ncw]

Summary

This is split out from https://reviews.llvm.org/D40559 as requested.

It adds a new argument to wasm-lld, --undefined, with similar semantics to the ELF linker. It pulls in symbols from files contained within a .a archive, forcing them to be included even if the translation unit would not otherwise be pulled in.

[sbc100]

test/wasm/load-undefined.ll

11

Could you add test for when the undefined symbol is not found? i.e. -u foo should cause a certain linker error? (unless we decide it doesn't, see below.)

(You can just add that case to within this file I think).

wasm/Driver.cpp

291

Hmm.. if we allow function symbols to have a null type I'm afraid we won't be able to represent them in the output object (since we can have an import or and export without a function type. Perhaps this is OK if we know these symbols will never appear in the output, but I'm worried that --allow-undefined will allow them to appear.

Also, I was experimenting with --undefined with GNU ld. The behaviour seems to be that if the symbol can't be found it simply results in an undefined symbol in the final output, not a link error. Which I find strange, but maybe we can to mimik that?

wasm/Symbols.h

72

I think its more clear if you explicitly compare with nullptr here.

[ruiu]

wasm/Driver.cpp

280

I'd change this to if (Config->Relocatable && Args.hasArgs(OPT_undefined)) so that we can remove Undefined variable.

[ncw]

Updated to address comments.

wasm/Driver.cpp

291

I should have added a test to make it clear, but what happens if the symbol can't be found, and --allow-undefined is specified, is that the symbol simply isn't exported or written out at all. The null type doesn't matter, and nothing crashes.

On second thoughts though, that might not be the right behaviour.

You make a good comparison with GNU ld. I'm not sure we can mimic that behaviour, sadly, since we can't import the symbol without knowing its type. And, I don't think we want to add some fancy parsing for types specified on the commandline, just so users can add unused imports!

I think these are the only two options:

• Ignore any undefined functions on the commandline (ie act as if --allow-undefined is always specified specifically for the -u symbols)
• Strictly check for any undefined functions on the commandline (ie act as if --allow-undefined was not specified, specifically for the -u symbols)

I prefer the strict check - if the user wants that symbol included by specifying its name, it's surely wrong if it can't be found.

I've added a test and updated the code to implement that.

[sbc100]

Thanks. LGTM

test/wasm/load-undefined.ll

13

Can you move these two tests down to the end of the file and then verify the errors they produce.

e.g:

; RUN: not lld -flavor wasm %t3.o -o %t.wasm -u symboldoesnotexist 2>&1 | FileCheck 
-check-prefix=CHECK-UNDEFINED %s
CHECK-UNDEFINED: .....

wasm/Driver.cpp

291

The second option sounds good to me. The ld behaviour doesn't make much sense to me to be honest. Out of interest what behavior does ELF lld have?

wasm/SymbolTable.cpp

122

How about:

"Skip the signature check if the existing function has no signature (e.g. if it is an undefined symbol generated by --undefined command line flag)."

[ncw]

Hopefully final update!

• Reworded comment
• Added check for error output from test

wasm/Driver.cpp

291

For completeness, it looks like ld.lld has similar behaviour to GNU ld. I tested and they are both lenient about not finding a symbol specified with --undefined.

[ncw]

Sorry for spam, yet another rebase! Every day brings fresh merge conflicts to LLD, at least it shows work is being done :)

test/wasm/load-undefined.ll

+; Verify that the -u / --undefined option is able to pull in symbols from
+; an archive, and doesn't error when uses to pull in a symbol already loaded.
+;
+; RUN: llc -filetype=obj -mtriple=wasm32-unknown-unknown-wasm %S/Inputs/ret64.ll -o %t.o
+; RUN: llc -filetype=obj -mtriple=wasm32-unknown-unknown-wasm %S/Inputs/ret32.ll -o %t2.o
+; RUN: llc -filetype=obj -mtriple=wasm32-unknown-unknown-wasm %s -o %t3.o
+; RUN: llvm-ar rcs %t2.a %t2.o
+; RUN: lld -flavor wasm %t3.o %t2.a %t.o -o %t.wasm -u ret32 --undefined ret64
+; RUN: obj2yaml %t.wasm | FileCheck %s
+
+; Verify that referencing a symbol that doesn't exist won't work
+; RUN: not lld -flavor wasm %t3.o -o %t.wasm -u symboldoesnotexist
+; RUN: not lld -flavor wasm %t3.o -o %t.wasm --undefined symboldoesnotexist --allow-undefined
+
+
+define i32 @_start() local_unnamed_addr {
+entry:
+  ret i32 1
+}
+
+; CHECK:        - Type:            EXPORT
+; CHECK-NEXT:     Exports:
+; CHECK-NEXT:       - Name:            memory
+; CHECK-NEXT:         Kind:            MEMORY
+; CHECK-NEXT:         Index:           0
+; CHECK-NEXT:       - Name:            _start
+; CHECK-NEXT:         Kind:            FUNCTION
+; CHECK-NEXT:         Index:           0
+; CHECK-NEXT:       - Name:            ret32
+; CHECK-NEXT:         Kind:            FUNCTION
+; CHECK-NEXT:         Index:           1
+; CHECK-NEXT:       - Name:            ret64
+; CHECK-NEXT:         Kind:            FUNCTION
+; CHECK-NEXT:         Index:           2
+; CHECK-NEXT:   - Type:

wasm/Driver.cpp

void LinkerDriver::link(ArrayRef<const char *> ArgsArr) {
   if (Config->OutputFile.empty())
     error("no output file specified");
 
   if (!Args.hasArg(OPT_INPUT))
     error("no input files");
 
   if (Config->Relocatable && !Config->Entry.empty())
     error("entry point specified for relocatable output file");
+  if (Config->Relocatable && Args.hasArg(OPT_undefined))
+    error("undefined symbols specified for relocatable output file");
 
   if (!Config->Relocatable) {
     if (Config->Entry.empty())
       Config->Entry = "_start";
     static WasmSignature Signature = {{}, WASM_TYPE_NORESULT};
     addSyntheticUndefinedFunction(Config->Entry, &Signature);
 
+    // Handle the `--undefined <sym>` options.
+    for (StringRef S : args::getStrings(Args, OPT_undefined))
+      addSyntheticUndefinedFunction(S, nullptr);
+
     addSyntheticGlobal("__stack_pointer", 0);
   }
 
   createFiles(Args);
   if (errorCount())
     return;
 
   // Add all files to the symbol table. This will add almost all
   // symbols that we need to the symbol table.
   for (InputFile *F : Files)
     Symtab->addFile(F);
 
   // Make sure we have resolved all symbols.
   if (!Config->Relocatable && !Config->AllowUndefined) {
     Symtab->reportRemainingUndefines();
     if (errorCount())
       return;
   }
 
   if (!Config->Entry.empty()) {
     Symbol *Sym = Symtab->find(Config->Entry);
     if (!Sym->isFunction())
       fatal("entry point is not a function: " + Sym->getName());
   }
+  for (StringRef S : args::getStrings(Args, OPT_undefined)) {
+    Symbol *Sym = Symtab->find(S);
+    if (!Sym->isDefined())
+      error("function forced with --undefined not found: " + Sym->getName());
+  }
+  if (errorCount())
+    return;
 
   // Write the result to the file.
   writeResult();
 }

wasm/Options.td

 include "llvm/Option/OptParser.td"
 
 // For options whose names are multiple letters, either one dash or
 // two can precede the option name except those that start with 'o'.
 class F<string name>: Flag<["--", "-"], name>;
 class J<string name>: Joined<["--", "-"], name>;
 class S<string name>: Separate<["--", "-"], name>;
 
+multiclass Eq<string name> {
+  def "": Separate<["--", "-"], name>;
+  def _eq: Joined<["--", "-"], name # "=">, Alias<!cast<Separate>(NAME)>;
+}
+
 def L: JoinedOrSeparate<["-"], "L">, MetaVarName<"<dir>">,
   HelpText<"Add a directory to the library search path">;
 
 def color_diagnostics: F<"color-diagnostics">,
   HelpText<"Use colors in diagnostics">;
 
 def color_diagnostics_eq: J<"color-diagnostics=">,
   HelpText<"Use colors in diagnostics">;

 def emit_relocs: F<"emit-relocs">, HelpText<"Generate relocations in output">;
 
 def strip_all: F<"strip-all">, HelpText<"Strip all symbols">;
 
 def strip_debug: F<"strip-debug">, HelpText<"Strip debugging information">;
 
 def sysroot: J<"sysroot=">, HelpText<"Set the system root">;
 
+defm undefined: Eq<"undefined">,
+  HelpText<"Force undefined symbol during linking">;
+
 def z: JoinedOrSeparate<["-"], "z">, MetaVarName<"<option>">,
   HelpText<"Linker option extensions">;
 
 def entry: S<"entry">, MetaVarName<"<entry>">,
   HelpText<"Name of entry point symbol">;
 
 def error_limit: J<"error-limit=">,
   HelpText<"Maximum number of errors to emit before stopping (0 = no limit)">;

 def allow_undefined_file_s: Separate<["-"], "allow-undefined-file">, Alias<allow_undefined_file>;
 
 // Aliases
 def alias_initial_memory_i: Flag<["-"], "i">, Alias<initial_memory>;
 def alias_max_memory_m: Flag<["-"], "m">, Alias<max_memory>;
 def alias_relocatable_r: Flag<["-"], "r">, Alias<relocatable>;
 def alias_entry_e: JoinedOrSeparate<["-"], "e">, Alias<entry>;
 def alias_entry_entry: J<"entry=">, Alias<entry>;
+def alias_undefined_u: JoinedOrSeparate<["-"], "u">, Alias<undefined>;

wasm/SymbolTable.cpp

error("symbol type mismatch: " + New.Name + "\n>>> defined as " +
           toString(Existing.getFile()) + "\n>>> defined as " +
           (NewIsFunction ? "Function" : "Global") + " in " + F.getName());
     return;
   }
 
   // For function symbols, optionally check the function signature matches too.
   if (!NewIsFunction || !Config->CheckSignatures)
     return;
+  // For undefined symbols added via --undefined we can't check the signature.
+  if (!Existing.hasFunctionType())
+    return;
 
   DEBUG(dbgs() << "checkSymbolTypes: " << New.Name << "\n");
   assert(NewSig);
 
   const WasmSignature &OldSig = Existing.getFunctionType();
   if (*NewSig == OldSig)
     return;
 

wasm/Symbols.h

public:
   StringRef getName() const { return Name; }
 
   // Returns the file from which this symbol was created.
   InputFile *getFile() const { return File; }
 
   uint32_t getGlobalIndex() const;
   uint32_t getFunctionIndex() const;
 
+  bool hasFunctionType() const { return FunctionType != nullptr; }
   const WasmSignature &getFunctionType() const;
   uint32_t getOutputIndex() const;
 
   // Returns the virtual address of a defined global.
   // Only works for globals, not functions.
   uint32_t getVirtualAddress() const;
 
   // Returns true if an output index has been set for this symbol