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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 accepted this revision.
sbc100 added a comment.
This revision is now accepted and ready to land.

Thanks. LGTM



================
Comment at: test/wasm/load-undefined.ll:13
+; RUN: not lld -flavor wasm %t3.o -o %t.wasm -u symboldoesnotexist
+; RUN: not lld -flavor wasm %t3.o -o %t.wasm --undefined symboldoesnotexist --allow-undefined
+
----------------
Can you move these two tests down to the end of the file and then verify the errors they produce.

e.g:

```
; RUN: not lld -flavor wasm %t3.o -o %t.wasm -u symboldoesnotexist 2>&1 | FileCheck 
-check-prefix=CHECK-UNDEFINED %s
CHECK-UNDEFINED: .....
```


================
Comment at: wasm/Driver.cpp:292
+    for (StringRef S : Undefined)
+      addSyntheticUndefinedFunction(S, nullptr);
+
----------------
ncw wrote:
> 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.
The second option sounds good to me.  The ld behaviour doesn't make much sense to me to be honest.  Out of interest what behavior does ELF lld have?


================
Comment at: wasm/SymbolTable.cpp:122
     return;
+  // For undefined symbols added via --undefined we can't check the signature.
+  if (!Existing.hasFunctionType())
----------------
How about:

"Skip the signature check if the existing function has no signature (e.g. if it is an undefined symbol generated by --undefined command line flag)."


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D40724