This is an archive of the discontinued LLVM Phabricator instance.

[LLD][ELF] Do not error for missing version when symbol has local version.
ClosedPublic

Authored by peter.smith on Feb 9 2018, 6:32 AM.

Details

Summary

If a symbol with an undefined version in a DSO is not going to be exported into the dynamic symbol table then do not give an error message for the missing version. This can happen with the --exclude-libs option which implicitly gives all symbols in a static library the local version. This matches the behavior of ld.gold and is exploited by the Bionic dynamic linker on Arm.

This was found when trying to link the bionic dynamic linker on Arm and got link errors with LLD. I've reported to Bionic under https://issuetracker.google.com/issues/73020933
as ld.bfd was also giving the same error message, but ld.gold was succeeding. I think on balance ld.gold is more sensible and we should match it.

Fixes PR36295

Diff Detail

Event Timeline

peter.smith created this revision.Feb 9 2018, 6:32 AM

Peter Smith via Phabricator <reviews@reviews.llvm.org> writes:
Index: ELF/Symbols.cpp
===================================================================

    • ELF/Symbols.cpp +++ ELF/Symbols.cpp @@ -206,8 +206,10 @@ It is an error if the specified version is not defined. Usually version script is not provided when linking executable, // but we may still want to override a versioned symbol from DSO,
  • // so we do not report error in this case.
  • if (Config->Shared) + so we do not report error in this case. We also do not error + if the symbol has a local version as it won't be in the dynamic + // symbol table. + if (Config->Shared && VersionId != VER_NDX_LOCAL)

Is the Config->Shared part still required?

At the moment it is; I get the error messages I remove it. If we could somehow arrange the default VersionId for symbols in executables to have VersionId == VER_NDX_LOCAL then the Config->Shared could be dropped. I can investigate that if you'd prefer it?

Should an error be reported when linking an executable with
--export-dynamic?

A good question. In theory it should as the symbol is exported, in practice I fear that the likelihood of anyone using export-dynamic with a @version definition is lower than the likelihood of using export-dynamic in a large project with a lurking @version definition that they did not know about (like the Bionic case). It seems like there isn't much consistency with how this is handled, and gold doesn't seem to handle the case sensibly. Table renders best in phab:

LinkerWith version scriptWithout version script
bfdsymbol given version from scriptsymbol not given version
goldsymbol given global versionversion defined, but symbol not given version
lldsymbol given version from scriptsymbol not given version

In summary, I don't have a strong opinion either way but I'm leaning towards not giving an error due to the risk of breaking existing programs.

espindola edited reviewers, added: espindola; removed: rafael.Mar 15 2018, 8:37 AM

Rebased against master. I'd like to see if I can progress this a bit further as there is now interest in Android to switch to LLD and this issue is preventing some of Bionic from being linked with LLD.

As it stands this change replicates the behaviour of gold. It must be said that the specification symbol versioning information is not clear on what the behaviour should be in this case. The symbol version is indeed missing from the executable, but exclude-libs has removed all symbols that use the version from the dynamic symbol table so the symbol version doesn't need to exist.

Rafael had a comment:

Should an error be reported when linking an executable with --export-dynamic?

My response was that I thought the risk of breaking existing programs was too high as symbol version scripts were not intended to be used with executables.

Does anyone have any input on how to move this forward?

grimar accepted this revision.May 11 2018, 4:57 AM

LGTM. Let's try this at least.

This revision is now accepted and ready to land.May 11 2018, 4:57 AM

Thanks, I'll wait till next Monday before committing.

This revision was automatically updated to reflect the committed changes.