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 ↗

(On Diff #125137)

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

wasm/Driver.cpp

226 ↗

(On Diff #125137)

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

291 ↗

(On Diff #125137)

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

[sunfish]

wasm/Driver.cpp

226 ↗

(On Diff #125137)

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

[ncw]

test/wasm/data-layout.ll

3 ↗

(On Diff #125137)

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

291 ↗

(On Diff #125137)

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 ↗

(On Diff #125137)

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 ↗

(On Diff #125137)

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 Diff #125137)

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 ↗

(On Diff #125137)

Agreed

291 ↗

(On Diff #125137)

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 ↗

(On Diff #125553)

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

264 ↗

(On Diff #125553)

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 ↗

(On Diff #125553)

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

wasm/Writer.cpp

264 ↗

(On Diff #125553)

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

224–227 ↗

(On Diff #125701)

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.

259–260 ↗

(On Diff #125701)

nit: I believe you can write

Config->Relocatable ? "" : "_start"

or at least

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

301–302 ↗

(On Diff #125701)

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

326–327 ↗

(On Diff #125701)

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

224–227 ↗

(On Diff #125701)

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 ↗

(On Diff #125945)

You can combine these two ifs.

[sbc100]

wasm/Driver.cpp

328 ↗

(On Diff #125945)

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 ↗

(On Diff #125945)

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.

lld/trunk/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
 

lld/trunk/wasm/Driver.cpp

case OPT_INPUT:
       break;
     }
   }
 
   if (Files.empty())
     error("no input files");
 }
 
+static const char *getEntry(opt::InputArgList &Args, const char *def) {
+  auto *Arg = Args.getLastArg(OPT_entry, OPT_no_entry);
+  if (!Arg)
+    return def;
+  if (Arg->getOption().getID() == OPT_no_entry)
+    return "";
+  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 (!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 = Symtab->addDefinedGlobal("__stack_pointer");
   }
 
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 (!Config->Entry.empty() && !Symtab->find(Config->Entry)->isDefined())
+    error("entry point not found: " + Config->Entry);
+  if (errorCount())
+    return;
 
   // Write the result to the file.
   writeResult();
 }

lld/trunk/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">;