Wasm entrypoint changes #3 (add --no-entry argument to LLD)

[ncw]

Summary

Split out from D40559 as requested.

This adds a --no-entry argument to LLD, used to suppress the default _start entrypoint.

[sbc100]

test/wasm/data-layout.ll

3

Looks like this is not based off of master. Can you rebase?

wasm/Driver.cpp

226

Empty string is not a valid symbol name. Do you have some reason to believe it is?

289

I don't think you need to change this. Treating empty as not present seems fine to me.

[sunfish]

wasm/Driver.cpp

226

There's a test for it: https://github.com/WebAssembly/testsuite/blob/master/names.wast#L20

[ncw]

test/wasm/data-layout.ll

3

It's based off D40559. I've got a pipeline of patches here!

In the title of the patches, I've tried to make it clear what order I'm expecting them to be merged in. That's solely to save work for whoever ends up committing them on my behalf, so they don't have to handle the merge conflicts.

Of course if D40559 is rejected, I can rebase this one, but I basically don't want to put extra admin on some else to manage my patches. Hope that's helpful!

wasm/Driver.cpp

289

I agree it wasn't an urgent change, but now it's done (at Dan's suggestion/request) I don't think it's doing any harm.

[ruiu]

wasm/Driver.cpp

226

You guys want to ban this by the wasm spec, no? Allowing the empty string as a symbol name isn't beneficial but just confusing, I think.

227

A newly created StringRef is considered to have an empty string. We don't usually distinguish uninitialized StringRef from a StringRef that happens to have an empty string. So this distinction is too subtle. If you really need to distinguish the two, a better way of doing it is to use Optional<StringRef>.

(But I believe disallowing the empty symbol is a better approach though.)

[ruiu]

wasm/Driver.cpp

226

On second thought, ELF might allow the empty string as a symbol name, but lld (and perhaps a lot of other tools) don't handle it as a valid symbol name because it doesn't worth it. I don't think you want to handle that super edge case.

[sbc100]

wasm/Driver.cpp

226

Agreed

289

If you want to represent an options stringref as distict from the empty string you can use llvm::Optional for that. Using .data() doesn't make sense here I don't think.

However, I agree with ruiu. I don't think want to support empty string as a value entry point, or even a symbol name.

[ncw]

Feedback addressed.

• Added check to disallow using empty string with --entry and --undefined
• Got rid of use of null StringRef for Entry
• Switched Entry to an Optional so code expresses intent more clearly by checking if (Config->Entry) rather than if (Config->Entry.empty())

[sbc100]

LGTM - with a couple more comments.

test/wasm/data-layout.ll

3

Is the --allow-undefined still needed? (I was thinking it might be there simply because this test doesn't have an entry point).

wasm/Config.h

38

I still think its simpler just to use empty string to mean no entry and continue to assume that empty strings cannot be symbols. I'm don't feel super strongly about it though. I guess maybe we should agree to spec in Linking.md that symbols cannot be empty strings?

wasm/Writer.cpp

264

I think you'll need to rebase this since the symbol is now stored in the Config itself.

[ruiu]

wasm/Config.h

38

I prefer empty string too. It is consistent with other lld ports.

[ncw]

Oops, OK, I've removed the use of Optional<StringRef> for Config::Entry.

The diff is shorter now, and should hopefully be good to go.

test/wasm/data-layout.ll

3

Good spot, but it actually still is needed. Inputs/hello.ll uses puts without defining it.

wasm/Writer.cpp

264

I have rebased, but I don't think there are any relevant changes to Config? The stack-pointer has moved in there, but the entrypoint's Symbol isn't in Config.

[ncw]

Updated with changes as requested (and rebased)

[ruiu]

wasm/Driver.cpp

228–231

Let's not too picky about "". In other ports, we don't check whether a given value is empty or not. If you pass --entry '', the linker should do whatever it is instructed to do. We allow users to shoot themselves in the foot. If they really want to use an empty string as an entry symbol name, they should be able to do that.

So, let's return Arg->getValue() without assigning it to Entry and check if it is empty string.

261–263

nit: I believe you can write

Config->Relocatable ? "" : "_start"

or at least

StringRef(Config->Relocatable ? "" : "_start")

304–305

Ditto -- if users want to force loading an empty symbol, we should do that.

327–331

You can now eliminate Sym

[ncw]

Updated with Rui's changes (all pretty minor).

Rebased, OK to apply directly to master.

Thanks for being patient reviewing all these patches!

wasm/Driver.cpp

228–231

Using empty string as the entrypoint name still won't work - because we just got rid of the code that handled unset/empty string differently.

I've made the change you requested to get rid of the empty-string check, but just bear in mind it won't actually make empty-string usable, it just makes --entry '' a legal synonym for --no-entry.

[ruiu]

LGTM

Thank you for making the change. As to the empty string handling, I wanted to make it consistent with other ports, and we in general don't write much code for edge cases to keep the code straightforward.

wasm/Driver.cpp

327–331

You can combine these two ifs.

[sbc100]

wasm/Driver.cpp

329

It looks like this change also means that --allow-undefined no longer applies to '_start'. I'm not necessarily against that but is there a reason for this change?

[ncw]

wasm/Driver.cpp

329

Ah good spot, I should have included that tweak in the --undefined review.

There is a reason: a) consistency with what we decided for --undefined, where --allow-undefined also doesn't apply, and b) it just makes no sense at all to import the entrypoint - if _start isn't found from libc. I can't imagine a single developer would want it to be silently added as an import instead!

[ncw]

Rebased again - with Rui's cosmetic tweak.

test/wasm/data-layout.ll

 ; RUN: llc -filetype=obj %p/Inputs/hello.ll -o %t.hello.o
 ; RUN: llc -filetype=obj %s -o %t.o
-; RUN: lld -flavor wasm --emit-relocs --allow-undefined --entry '' -o %t.wasm %t.o %t.hello.o
+; RUN: lld -flavor wasm --emit-relocs --allow-undefined --no-entry -o %t.wasm %t.o %t.hello.o
 ; RUN: obj2yaml %t.wasm | FileCheck %s
 
 target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
 target triple = "wasm32-unknown-unknown-wasm"
 
 @foo = hidden global i32 1, align 4
 @aligned_bar = hidden global i32 3, align 16
 

wasm/Config.h

 //===- Config.h -------------------------------------------------*- C++ -*-===//
 //
 //                             The LLVM Linker
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLD_WASM_CONFIG_H
 #define LLD_WASM_CONFIG_H
 
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/BinaryFormat/Wasm.h"
 
 #include "Symbols.h"
 
 using llvm::wasm::WasmGlobal;
 
 namespace lld {
 namespace wasm {
 
 struct Configuration {
   bool AllowUndefined;
   bool CheckSignatures;
   bool Demangle;
   bool EmitRelocs;
   bool ImportMemory;
   bool Relocatable;
   bool StripAll;
   bool StripDebug;
   uint32_t GlobalBase;
   uint32_t InitialMemory;
   uint32_t MaxMemory;
   uint32_t ZStackSize;
-  llvm::StringRef Entry;
+  llvm::Optional<llvm::StringRef> Entry;
   llvm::StringRef OutputFile;
   llvm::StringRef Sysroot;
 
   llvm::StringSet<> AllowUndefinedSymbols;
   std::vector<llvm::StringRef> SearchPaths;
   std::vector<std::pair<Symbol *, WasmGlobal>> SyntheticGlobals;
 };
 
 // The only instance of Configuration struct.
 extern Configuration *Config;
 
 } // namespace wasm
 } // namespace lld
 
 #endif

wasm/Driver.cpp

case OPT_INPUT:
       break;
     }
   }
 
   if (Files.empty())
     error("no input files");
 }
 
+static Optional<StringRef> getEntry(opt::InputArgList &Args,
+                                    Optional<StringRef> def) {
+  auto *Arg = Args.getLastArg(OPT_entry, OPT_no_entry);
+  if (!Arg)
+    return def;
+  if (Arg->getOption().getID() == OPT_no_entry)
+    return Optional<StringRef>();
+  return Optional<StringRef>(Arg->getValue());
+}
+
 void LinkerDriver::link(ArrayRef<const char *> ArgsArr) {
   WasmOptTable Parser;
   opt::InputArgList Args = Parser.parse(ArgsArr.slice(1));
 
   // Handle --help
   if (Args.hasArg(OPT_help)) {
     printHelp(ArgsArr[0]);
     return;
if (Args.hasArg(OPT_version) || Args.hasArg(OPT_v)) {
     return;
   }
 
   Config->AllowUndefined = Args.hasArg(OPT_allow_undefined);
   Config->CheckSignatures =
       Args.hasFlag(OPT_check_signatures, OPT_no_check_signatures, false);
   Config->EmitRelocs = Args.hasArg(OPT_emit_relocs);
   Config->Relocatable = Args.hasArg(OPT_relocatable);
-  Config->Entry = Args.getLastArgValue(OPT_entry,
-                                       Config->Relocatable ? "" : "_start");
+  Config->Entry = getEntry(Args, Config->Relocatable ? Optional<StringRef>()
+                               : Optional<StringRef>(StringRef("_start")));
   Config->ImportMemory = Args.hasArg(OPT_import_memory);
   Config->OutputFile = Args.getLastArgValue(OPT_o);
   Config->SearchPaths = args::getStrings(Args, OPT_L);
   Config->StripAll = Args.hasArg(OPT_strip_all);
   Config->StripDebug = Args.hasArg(OPT_strip_debug);
   Config->Sysroot = Args.getLastArgValue(OPT_sysroot);
   errorHandler().Verbose = Args.hasArg(OPT_verbose);
   ThreadsEnabled = Args.hasFlag(OPT_threads, OPT_no_threads, true);
 
if (Optional<MemoryBufferRef> Buf = readFile(Arg->getValue()))
         Config->AllowUndefinedSymbols.insert(Sym);
 
   if (Config->OutputFile.empty())
     error("no output file specified");
 
   if (!Args.hasArg(OPT_INPUT))
     error("no input files");
 
-  if (Config->Relocatable && !Config->Entry.empty())
+  if (Config->Relocatable && Config->Entry)
     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()) {
+    if (Config->Entry) {
+      if (Config->Entry.getValue().empty())
+        error("empty string invalid for entry point");
       static WasmSignature Signature = {{}, WASM_TYPE_NORESULT};
-      addSyntheticUndefinedFunction(Config->Entry, &Signature);
+      addSyntheticUndefinedFunction(Config->Entry.getValue(), &Signature);
     }
 
     // Handle the `--undefined <sym>` options.
-    for (StringRef S : args::getStrings(Args, OPT_undefined))
+    for (StringRef S : args::getStrings(Args, OPT_undefined)) {
+      if (S.empty())
+        error("empty string invalid for undefined symbol");
       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());
+  if (Config->Entry) {
+    Symbol *Sym = Symtab->find(Config->Entry.getValue());
+    if (!Sym->isDefined())
+      error("entry point not found: " + Config->Entry.getValue());
   }
   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

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 no_entry: F<"no-entry">,
+  HelpText<"Do not output any entry point">;
+
 def error_limit: J<"error-limit=">,
   HelpText<"Maximum number of errors to emit before stopping (0 = no limit)">;
 
 // The follow flags are unique to wasm
 
 def global_base: J<"global-base=">,
   HelpText<"Where to start to place global data">;
 

wasm/Writer.cpp

void Writer::createTableSection() {
   writeUleb128(OS, NumTableElems, "table max size");
 }
 
 void Writer::createExportSection() {
   // Memory is and main function are exported for executables.
   bool ExportMemory = !Config->Relocatable && !Config->ImportMemory;
   bool ExportOther = true; // ??? TODO Config->Relocatable;
   bool ExportHidden = Config->Relocatable;
-  Symbol *EntrySym = Symtab->find(Config->Entry);
+  Symbol *EntrySym = Config->Entry ? Symtab->find(Config->Entry.getValue())
+                                   : nullptr;
   bool ExportEntry = !Config->Relocatable && EntrySym && EntrySym->isDefined();
 
   uint32_t NumExports = 0;
 
   if (ExportMemory)
     ++NumExports;
 
   if (ExportEntry)
if (ExportMemory) {
     MemoryExport.Name = "memory";
     MemoryExport.Kind = WASM_EXTERNAL_MEMORY;
     MemoryExport.Index = 0;
     writeExport(OS, MemoryExport);
   }
 
   if (ExportEntry) {
     WasmExport EntryExport;
-    EntryExport.Name = Config->Entry;
+    EntryExport.Name = Config->Entry.getValue();
     EntryExport.Kind = WASM_EXTERNAL_FUNCTION;
     EntryExport.Index = EntrySym->getOutputIndex();
     writeExport(OS, EntryExport);
   }
 
   if (ExportOther) {
     for (ObjFile *File : Symtab->ObjectFiles) {
       for (Symbol *Sym : File->getSymbols()) {
for (ObjFile *File : Symtab->ObjectFiles) {
           Export.Kind = WASM_EXTERNAL_GLOBAL;
         writeExport(OS, Export);
       }
     }
   }
 }
 
 void Writer::createStartSection() {
-  if (Config->Entry.empty())
+  if (!Config->Entry)
     return;
 
-  const Symbol *Sym = Symtab->find(Config->Entry);
+  const Symbol *Sym = Symtab->find(Config->Entry.getValue());
   assert(Sym && Sym->isDefined() && Sym->isFunction());
 
   SyntheticSection *Section = createSyntheticSection(WASM_SEC_START);
   raw_ostream &OS = Section->getStream();
 
   log("Start: " + Sym->getName());
   writeUleb128(OS, Sym->getOutputIndex(), "start index");
 }