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

220

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

282

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

[sunfish]

wasm/Driver.cpp

220

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

282

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

220

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.

221

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

220

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

220

Agreed

282

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 ↗

(On Diff #125553)

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

251

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

[ruiu]

wasm/Config.h

38 ↗

(On Diff #125553)

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

251

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

222–225

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.

253–255

nit: I believe you can write

Config->Relocatable ? "" : "_start"

or at least

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

295–296

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

326–328

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

222–225

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

326–327

You can combine these two ifs.

[sbc100]

wasm/Driver.cpp

328

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

328

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 -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/Driver.cpp

case OPT_INPUT:
       break;
     }
   }
 
   if (Files.empty())
     error("no input files");
 }
 
+static StringRef getEntry(opt::InputArgList &Args, StringRef def) {
+  auto *Arg = Args.getLastArg(OPT_entry, OPT_no_entry);
+  if (!Arg)
+    return def;
+  if (Arg->getOption().getID() == OPT_no_entry)
+    return StringRef();
+  return 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)) {
     outs() << getLLDVersion() << "\n";
     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->Entry = Args.getLastArgValue(OPT_entry);
+  Config->Relocatable = Args.hasArg(OPT_relocatable);
+  Config->Entry = getEntry(Args, Config->Relocatable ? "" : "_start");
   Config->ImportMemory = Args.hasArg(OPT_import_memory);
   Config->OutputFile = Args.getLastArgValue(OPT_o);
-  Config->Relocatable = Args.hasArg(OPT_relocatable);
   Config->SearchPaths = args::getStrings(Args, OPT_L);
   Config->StripAll = Args.hasArg(OPT_strip_all);
   Config->StripDebug = Args.hasArg(OPT_strip_debug);
   errorHandler().Verbose = Args.hasArg(OPT_verbose);
   ThreadsEnabled = Args.hasFlag(OPT_threads, OPT_no_threads, true);
   if (Config->Relocatable)
     Config->EmitRelocs = 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())
     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";
+    if (!Config->Entry.empty()) {
       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);
 
     Config->StackPointerSymbol = addSyntheticGlobal("__stack_pointer", 0);
   }
 
   createFiles(Args);
   if (errorCount())
     return;
 
   // Add all files to the symbol table. This will add almost all
if (!Config->Relocatable && !Config->AllowUndefined) {
     // -u/--undefined since these undefined symbols have only names and no
     // function signature, which means they cannot be written to the final
     // output.
     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;
   }
 
   if (!Config->Entry.empty()) {
-    Symbol *Sym = Symtab->find(Config->Entry);
-    if (!Sym->isFunction())
-      fatal("entry point is not a function: " + Sym->getName());
+    if (!Symtab->find(Config->Entry)->isDefined())
+      error("entry point not found: " + Config->Entry);
   }
+  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() {
   writeSleb128(OS, WASM_TYPE_ANYFUNC, "table type");
   writeUleb128(OS, WASM_LIMITS_FLAG_HAS_MAX, "table flags");
   writeUleb128(OS, NumTableElems, "table initial size");
   writeUleb128(OS, NumTableElems, "table max size");
 }
 
 void Writer::createExportSection() {
   bool ExportMemory = !Config->Relocatable && !Config->ImportMemory;
-  Symbol *EntrySym = Symtab->find(Config->Entry);
+  Symbol *EntrySym = !Config->Entry.empty() ? Symtab->find(Config->Entry) : nullptr;
   bool ExportEntry = !Config->Relocatable && EntrySym && EntrySym->isDefined();
   bool ExportHidden = Config->EmitRelocs;
 
   uint32_t NumExports = ExportMemory ? 1 : 0;
 
   std::vector<const Symbol *> SymbolExports;
   if (ExportEntry)
     SymbolExports.emplace_back(EntrySym);