[PATCH] D40725: Wasm entrypoint changes #3 (add --no-entry argument to LLD) APPLY AFTER D40559

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: wasm/Driver.cpp:224-227
+  StringRef Entry(Arg->getValue());
+  if (Entry.empty())
+    error("empty string invalid for entry point");
+  return Entry;
----------------
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.


================
Comment at: wasm/Driver.cpp:259-260
   Config->Relocatable = Args.hasArg(OPT_relocatable);
-  Config->Entry = Args.getLastArgValue(OPT_entry,
-                                       Config->Relocatable ? "" : "_start");
+  Config->Entry = getEntry(Args, Config->Relocatable ? StringRef()
+                                                     : StringRef("_start"));
   Config->ImportMemory = Args.hasArg(OPT_import_memory);
----------------
nit: I believe you can write

  Config->Relocatable ? "" : "_start"

or at least

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


================
Comment at: wasm/Driver.cpp:301-302
+    for (StringRef S : args::getStrings(Args, OPT_undefined)) {
+      if (S.empty())
+        error("empty string invalid for undefined symbol");
       addSyntheticUndefinedFunction(S, nullptr);
----------------
Ditto -- if users want to force loading an empty symbol, we should do that.


================
Comment at: wasm/Driver.cpp:326-327
   if (!Config->Entry.empty()) {
     Symbol *Sym = Symtab->find(Config->Entry);
-    if (!Sym->isFunction())
-      fatal("entry point is not a function: " + Sym->getName());
+    if (!Sym->isDefined())
+      error("entry point not found: " + Config->Entry);
----------------
You can now eliminate `Sym`


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D40725