[PATCH] D40724: Wasm entrypoint changes #1 (add --undefined argument to LLD)

[Nicholas Wilson via Phabricator via llvm-commits]

ncw added inline comments.


================
Comment at: wasm/Driver.cpp:292
+    for (StringRef S : Undefined)
+      addSyntheticUndefinedFunction(S, nullptr);
+
----------------
sbc100 wrote:
> Hmm.. if we allow function symbols to have a null type I'm afraid we won't be able to represent them in the output object (since we can have an import or and export without a function type.  Perhaps this is OK if we know these symbols will never appear in the output, but I'm worried that --allow-undefined will allow them to appear.
> 
> Also, I was experimenting with --undefined with GNU ld.  The behaviour seems to be that if the symbol can't be found it simply results in an undefined symbol in the final output, not a link error.  Which I find strange, but maybe we can to mimik that?
I should have added a test to make it clear, but what happens if the symbol can't be found, and `--allow-undefined` is specified, is that the symbol simply isn't exported or written out at all. The null type doesn't matter, and nothing crashes.

On second thoughts though, that might not be the right behaviour.

You make a good comparison with GNU ld. I'm not sure we can mimic that behaviour, sadly, since we can't import the symbol without knowing its type. And, I don't think we want to add some fancy parsing for types specified on the commandline, just so users can add unused imports!

I think these are the only two options:
* Ignore any undefined functions on the commandline (ie act as if `--allow-undefined` is always specified specifically for the `-u` symbols)
* Strictly check for any undefined functions on the commandline (ie act as if `--allow-undefined` was not specified, specifically for the `-u` symbols)

I prefer the strict check - if the user wants that symbol included by specifying its name, it's surely wrong if it can't be found.

I've added a test and updated the code to implement that.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D40724