Page MenuHomePhabricator

[lld][WebAssembly] Implement --unresolved-symbols
ClosedPublic

Authored by sbc100 on May 1 2020, 10:23 AM.

Details

Summary

This is a more full featured version of --allow-undefined.
The semantics of the different methods are as follows:

report-all:

                                                                      
Report all unresolved symbols.  This is the default.  Normally the linker
will generate an error message for each reported unresolved symbol but the 
option `--warn-unresolved-symbols` can change this to a warning.

ignore-all:

                                                                      
Resolve all undefined symbols to zero.  For data and function addresses
this is trivial.  For direct function calls, the linker will generate a
trapping stub function in place of the undefined function.

import-functions:

                                                                      
Generate WebAssembly imports for any undefined functions.  Undefined data
symbols are resolved to zero as in `ignore-all`.  This corresponds the

The plan is to followup with a new mode called import-all which allows
for statically linked binaries to refer to both data and functions
symbols from the embedder.

Diff Detail

Event Timeline

sbc100 created this revision.May 1 2020, 10:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2020, 10:23 AM
sbc100 retitled this revision from [WebAssembly][lld] Implement --unresolved-symbols This is a more full featured version of the existing wasm-specificed `--allow-undefined` flag that we probably should never have invented. to [WebAssembly][lld] Implement --unresolved-symbols.May 1 2020, 10:24 AM
sbc100 edited the summary of this revision. (Show Details)
sbc100 added reviewers: ruiu, kripken.
sbc100 retitled this revision from [WebAssembly][lld] Implement --unresolved-symbols to [lld][WebAssembly] Implement --unresolved-symbols.
sbc100 edited the summary of this revision. (Show Details)Sat, Nov 14, 8:28 AM
sbc100 updated this revision to Diff 305316.Sat, Nov 14, 8:31 AM
sbc100 edited the summary of this revision. (Show Details)
  • typos
sbc100 updated this revision to Diff 305318.Sat, Nov 14, 8:34 AM

remove some tracing

Harbormaster completed remote builds in B78855: Diff 305318.
sbc100 updated this revision to Diff 305362.Sun, Nov 15, 7:29 AM
  • cleanup

In ELF, --unresolved-symbols= sets --[no-]allow-shlib-undefined and -z (un)defs simultaneously.

  • --[no-]allow-shlib-undefined: whether an unresolved undefined symbol from a shared object should be errored
  • -z (un)defs: whether an unresolved undefined symbol from a regular object should be errored.

Does the wasm usage fit well in the existing practice?

In ELF, --unresolved-symbols= sets --[no-]allow-shlib-undefined and -z (un)defs simultaneously.

  • --[no-]allow-shlib-undefined: whether an unresolved undefined symbol from a shared object should be errored
  • -z (un)defs: whether an unresolved undefined symbol from a regular object should be errored.

Does the wasm usage fit well in the existing practice?

The support for shared libraries in wasm-ld, and indeed the entire PIC ABI, is experimental (in fact, it will warn until you also pass --experimental-pic).

In particular, support for linking against shared libraries does not yet

In ELF, --unresolved-symbols= sets --[no-]allow-shlib-undefined and -z (un)defs simultaneously.

  • --[no-]allow-shlib-undefined: whether an unresolved undefined symbol from a shared object should be errored
  • -z (un)defs: whether an unresolved undefined symbol from a regular object should be errored.

Does the wasm usage fit well in the existing practice?

Yes. While we don't currently support either of those flags I believe the implementation of ignore-all here will match -z defs, so I think that adding support for -z (un)defs would be trivial if decide do add that flag in the future.

Regarding --[no-]allow-shlib-undefined, our shared library support is still experimental is rather limited. Using it currently requires the --experimental-pic flag.

BTW, looking at the ELF code I don't see --unresolved-symbols currently effecting `--allow-shlib-undefined.

In ELF/Driver.cpp we have:

config->allowShlibUndefined =                                                  
      args.hasFlag(OPT_allow_shlib_undefined, OPT_no_allow_shlib_undefined,                        
                   args.hasArg(OPT_shared));

And this doesn't look like it gets updated based on --unresolved-symbols.

sbc100 updated this revision to Diff 305524.Mon, Nov 16, 8:20 AM

clang-tidy

In ELF, --unresolved-symbols= sets --[no-]allow-shlib-undefined and -z (un)defs simultaneously.

  • --[no-]allow-shlib-undefined: whether an unresolved undefined symbol from a shared object should be errored
  • -z (un)defs: whether an unresolved undefined symbol from a regular object should be errored.

Does the wasm usage fit well in the existing practice?

The support for shared libraries in wasm-ld, and indeed the entire PIC ABI, is experimental (in fact, it will warn until you also pass --experimental-pic).

In particular, support for linking against shared libraries does not yet

In ELF, --unresolved-symbols= sets --[no-]allow-shlib-undefined and -z (un)defs simultaneously.

  • --[no-]allow-shlib-undefined: whether an unresolved undefined symbol from a shared object should be errored
  • -z (un)defs: whether an unresolved undefined symbol from a regular object should be errored.

Does the wasm usage fit well in the existing practice?

Yes. While we don't currently support either of those flags I believe the implementation of ignore-all here will match -z defs, so I think that adding support for -z (un)defs would be trivial if decide do add that flag in the future.

Regarding --[no-]allow-shlib-undefined, our shared library support is still experimental is rather limited. Using it currently requires the --experimental-pic flag.

BTW, looking at the ELF code I don't see --unresolved-symbols currently effecting `--allow-shlib-undefined.

In ELF/Driver.cpp we have:

config->allowShlibUndefined =                                                  
      args.hasFlag(OPT_allow_shlib_undefined, OPT_no_allow_shlib_undefined,                        
                   args.hasArg(OPT_shared));

And this doesn't look like it gets updated based on --unresolved-symbols.

You are right. I knew this when investigating https://reviews.llvm.org/D67479#1667256
It is of very low priority because --unresolved-symbols= has too few use cases. Now wasm is adding this option I'll make the ELF side semantics clearer: D91510

  • -z (un)defs: whether an unresolved undefined symbol from a regular object should be errored.

I wish I had known about -z undefs when I first created wasm-ld.. I could have avoided inventing --allow-undefined

@MaskRay any objections to landing this?

BTW, how to you feel about being a reviewer on wasm-ld change in general.. if you are OK with it I could use a reviewer for my work here now that @Rui is less active.

@MaskRay any objections to landing this?

BTW, how to you feel about being a reviewer on wasm-ld change in general.. if you are OK with it I could use a reviewer for my work here now that @Rui is less active.

I don't mind but you'll throw out some learning resources on me.. (Persoanlly I do want to study more binary formats) For now I guess the best I can provide is how some options are used in ELF..

I just committed D91510 for ELF - personally I think the option is a bit awkward to use as it changes two bits simultaneously. There are more values in the wasm-lld version: do you think these values should all be mutually exclusive. If yes, this looks fine to me.

@MaskRay any objections to landing this?

BTW, how to you feel about being a reviewer on wasm-ld change in general.. if you are OK with it I could use a reviewer for my work here now that @Rui is less active.

I don't mind but you'll throw out some learning resources on me.. (Persoanlly I do want to study more binary formats) For now I guess the best I can provide is how some options are used in ELF..

That sounds good. I can include wasm-specific people too in all my changes, but its good to have somebody with deep knowledge on the other linker take a look at changes like this for sure.

I just committed D91510 for ELF - personally I think the option is a bit awkward to use as it changes two bits simultaneously. There are more values in the wasm-lld version: do you think these values should all be mutually exclusive. If yes, this looks fine to me.

That an interesting point. Yes I am certainly imagining the various modes being mutually exclusive. Unlike ELF we have a couple of different ways that we want to be able to handle undefined symbols accepting various value here makes sense to me. Just doing -z undefs would not be enough in general as I would also want to know *how* to handle those undefined symbols. Once the followup to this change lands there will be 3 different ways to deal with them once we know we want to allow them.

@dschuff can I get an final lgtm from you?

dschuff accepted this revision.Tue, Nov 17, 4:08 PM
dschuff added inline comments.
lld/docs/WebAssembly.rst
97

"corresponds the" -> "corresponds to the"

98

The copy of this text in the commit message is missing this line.

This revision is now accepted and ready to land.Tue, Nov 17, 4:08 PM
This revision was landed with ongoing or failed builds.Tue, Nov 17, 4:28 PM
This revision was automatically updated to reflect the committed changes.