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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: wasm/Driver.cpp:226
+    return def;
+  // Apparently the empty string is a valid symbol name in Wasm, so we use a
+  // null string to mean "no entrypoint", *not* the empty string.
----------------
ruiu wrote:
> ruiu wrote:
> > sunfish wrote:
> > > sbc100 wrote:
> > > > Empty string is not a valid symbol name.   Do you have some reason to believe it is?
> > > There's a test for it: https://github.com/WebAssembly/testsuite/blob/master/names.wast#L20
> > 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.
> 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.
Agreed


================
Comment at: wasm/Driver.cpp:291
 
-  if (Config->Relocatable && !Config->Entry.empty())
+  if (Config->Relocatable && Config->Entry.data())
     error("entry point specified for relocatable output file");
----------------
ncw wrote:
> sbc100 wrote:
> > I don't think you need to change this.  Treating empty as not present seems fine to me.
> 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.
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.  


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D40725