This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Make foo@@v1 resolve undefined foo@v1
ClosedPublic

Authored by MaskRay on Nov 27 2020, 9:31 PM.

Details

Summary

The symbol resolution rules for versioned symbols are:

  • foo@@v1 (default version) resolves both undefined foo and foo@v1
  • foo@v1 (non-default version) resolves undefined foo@v1

Note, foo@@v1 must be defined (the assembler errors if attempting to
create an undefined foo@@v1).

For defined foo@@v1 in a shared object, we call SymbolTable::addSymbol twice,
one for foo and the other for foo@v1. We don't do the same for object files, so
foo@@v1 defined in one object file incorrectly does not resolve a foo@v1
reference in another object file.

This patch fixes the issue by reusing the --wrap code to redirect symbols in
object files. This has to be done after processing input files because
foo and foo@v1 are two separate symbols if we haven't seen foo@@v1.

Add a helper Symbol::getVersionSuffix to retrieve the optional trailing
@... or @@... from the possibly truncated symbol name.

Depends on D92258

Diff Detail

Event Timeline

MaskRay created this revision.Nov 27 2020, 9:31 PM
MaskRay requested review of this revision.Nov 27 2020, 9:31 PM

(FWIW I have made some notes about symbol versioning at https://translate.google.com/translate?sl=zh-CN&tl=en&u=https%3A%2F%2Fmaskray.me%2Fblog%2F2020-11-26-all-about-symbol-versioning)
(Apparently people are annoyed by lack of symbol versioning documentation https://invisible-island.net/ncurses/ncurses-mapsyms.html :) )
(We may need one document under docs/ELF/ describing the LLD behavior)

MaskRay retitled this revision from [ELF] Make foo@@v1 resolved undefined foo@v1 to [ELF] Make foo@@v1 resolve undefined foo@v1.Nov 28 2020, 1:02 AM
MaskRay updated this revision to Diff 308239.Nov 29 2020, 3:53 PM

Report duplicate symbol error even if foo@@v1 is weak

MaskRay updated this revision to Diff 308243.Nov 29 2020, 4:57 PM

Test visibility

grimar added inline comments.Nov 30 2020, 12:19 AM
lld/ELF/Driver.cpp
1937

Should we add something like Symbol::getFullName or Symbol::getVersionedName?
The code like const char *end1 = name.data() + name.size() looks hacky. I'd isolate it.

1939

I'd suggest this change here and below for conisistency.

I'm not much of a symbol versioning expert (see my inline comment), but this approach seems reasonable to me at least.

(FWIW I have made some notes about symbol versioning at https://translate.google.com/translate?sl=zh-CN&tl=en&u=https%3A%2F%2Fmaskray.me%2Fblog%2F2020-11-26-all-about-symbol-versioning)
(Apparently people are annoyed by lack of symbol versioning documentation https://invisible-island.net/ncurses/ncurses-mapsyms.html :) )
(We may need one document under docs/ELF/ describing the LLD behavior)

In my opinion, you can never have too much documentation.

lld/ELF/Driver.cpp
1927–1928

Given the possibility for a significant link time cost to this section of code where there are lots of (versioned) symbols, perhaps it is worth adding a time scope to this function with this change?

1934

Seems to me like iterating through all symbols could be quite expensive, based on an assumption that most symbols aren't versioned. At least for our platform, we don't even support symbol versioning, so there shouldn't be ANY versioned symbols, meaning an unnecessary trawl through the symbols. Would we be better off recording earlier and then using here a list of versioned symbols?

1935–1936

There's a slight inconsistency in your grammar here - the first sentence refers to multiple symbols, but "Its name" implies a singular symbol. Probably best to rewrite as suggested.

1942
1952
lld/test/ELF/symver.s
18–20
43–44

Optional improvement to this check, as it will a) still pass if there are more than 10 section headers, and b) is shorter. (It will start matching against 0 too, but that should be printed as UND so there is no issue here).

You can use the same technique for the protected case too, if you want.

MaskRay updated this revision to Diff 308409.Nov 30 2020, 10:07 AM
MaskRay marked 9 inline comments as done.
MaskRay retitled this revision from [ELF] Make foo@@v1 resolve undefined foo@v1 to [ELF] Make foo@@v1 resolved undefined foo@v1.
MaskRay edited the summary of this revision. (Show Details)

Comments

MaskRay retitled this revision from [ELF] Make foo@@v1 resolved undefined foo@v1 to [ELF] Make foo@@v1 resolve undefined foo@v1.Nov 30 2020, 10:07 AM
MaskRay added inline comments.
lld/ELF/Driver.cpp
1934

Added a TimeScope.

In my testing,
for a 4.9GiB output, redirectSymbols takes 49ms in a 31s link, so the cost seems acceptable.
For a 161MiB clang, redirectSymbols takes 14ms in the 923ms link, so the cost (1.5%) is a bit significant.

I added an early return below, then it just takes 2ms for clang. I think the cost is acceptable.

(On an unrelated thing, 048b16f7fbb745635b48d31ee957bb8865597606 reclaimed a 1%~1.7% performance loss.)

1937

Added Symbol::getVersionSuffix

MaskRay updated this revision to Diff 308410.Nov 30 2020, 10:10 AM

Fix comments

Minor nit. The rest looks probably good to me.

lld/ELF/Symbols.h
181

NUl->NUL

jhenderson accepted this revision.Dec 1 2020, 12:36 AM

LGTM, once the nit is fixed.

This revision is now accepted and ready to land.Dec 1 2020, 12:36 AM
MaskRay updated this revision to Diff 308674.Dec 1 2020, 8:50 AM
MaskRay marked an inline comment as done.

NUl -> NUL

This revision was landed with ongoing or failed builds.Dec 1 2020, 8:54 AM
This revision was automatically updated to reflect the committed changes.