Wasm entrypoint changes #1 (add --undefined argument to LLD)
ClosedPublic

Authored by ncw on Fri, Dec 1, 6:57 AM.

Details

Summary

This is split out from https://reviews.llvm.org/D40559 as requested.

It adds a new argument to wasm-lld, --undefined, with similar semantics to the ELF linker. It pulls in symbols from files contained within a .a archive, forcing them to be included even if the translation unit would not otherwise be pulled in.

Diff Detail

Repository
rL LLVM
ncw created this revision.Fri, Dec 1, 6:57 AM
sbc100 added inline comments.Fri, Dec 1, 11:18 AM
test/wasm/load-undefined.ll
10 ↗(On Diff #125132)

Could you add test for when the undefined symbol is not found? i.e. -u foo should cause a certain linker error? (unless we decide it doesn't, see below.)

(You can just add that case to within this file I think).

wasm/Driver.cpp
292 ↗(On Diff #125132)

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?

wasm/Symbols.h
72 ↗(On Diff #125132)

I think its more clear if you explicitly compare with nullptr here.

ruiu added a subscriber: ruiu.Fri, Dec 1, 1:00 PM
ruiu added inline comments.
wasm/Driver.cpp
281 ↗(On Diff #125132)

I'd change this to if (Config->Relocatable && Args.hasArgs(OPT_undefined)) so that we can remove Undefined variable.

ncw updated this revision to Diff 125550.Tue, Dec 5, 9:21 AM
ncw marked 3 inline comments as done.
ncw retitled this revision from Wasm entrypoint changes #1 (add --undefined argument to LLD) APPLY AFTER D40690 to Wasm entrypoint changes #1 (add --undefined argument to LLD).
ncw edited the summary of this revision. (Show Details)

Updated to address comments.

wasm/Driver.cpp
292 ↗(On Diff #125132)

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.

sbc100 accepted this revision.Tue, Dec 5, 11:54 AM

Thanks. LGTM

test/wasm/load-undefined.ll
13 ↗(On Diff #125550)

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: .....
wasm/Driver.cpp
292 ↗(On Diff #125132)

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?

wasm/SymbolTable.cpp
122 ↗(On Diff #125550)

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)."

This revision is now accepted and ready to land.Tue, Dec 5, 11:54 AM
ncw updated this revision to Diff 125688.Wed, Dec 6, 3:09 AM
ncw marked 2 inline comments as done.

Hopefully final update!

  • Reworded comment
  • Added check for error output from test
wasm/Driver.cpp
292 ↗(On Diff #125132)

For completeness, it looks like ld.lld has similar behaviour to GNU ld. I tested and they are both lenient about not finding a symbol specified with --undefined.

ncw updated this revision to Diff 125698.Wed, Dec 6, 4:24 AM

Sorry for spam, yet another rebase! Every day brings fresh merge conflicts to LLD, at least it shows work is being done :)

This revision was automatically updated to reflect the committed changes.