[PATCH] D35059: [ELF] - Handle symbols with default version early.

[Rafael Avila de Espindola via llvm-commits]

LGTM.

Thanks!


George Rimar via Phabricator <reviews at reviews.llvm.org> writes:

> grimar created this revision.
> Herald added a subscriber: emaste.
>
> This fixes last testcase provided in PR28414.
> In short issue is next: when we had X@@Version symbol in object A,
> we did not resolve it to X early. Then when in another object B
> we had reference to undefined X, symbol X from archive was fetched.
> Since both archive and object A contains another symbol Z, duplicate
> symbol definition was triggered as a result.
>
> Correct behavior is to use X@@Version from object A instead and do not fetch
> any symbols from archive.
>
>
> https://reviews.llvm.org/D35059
>
> Files:
>   ELF/SymbolTable.cpp
>   test/ELF/Inputs/symver-archive1.s
>   test/ELF/Inputs/symver-archive2.s
>   test/ELF/symver-archive.s
>
>
> Index: test/ELF/symver-archive.s
> ===================================================================
> --- test/ELF/symver-archive.s
> +++ test/ELF/symver-archive.s
> @@ -0,0 +1,15 @@
> +# REQUIRES: x86
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t1
> +# RUN: rm -f %t.a
> +# RUN: llvm-ar rcs %t.a %t1
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %S/Inputs/symver-archive1.s -o %t2.o
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %S/Inputs/symver-archive2.s -o %t3.o
> +# RUN: ld.lld -o %t.out %t2.o %t3.o %t.a
> +
> +.text
> +.globl x
> +.type x, @function
> +x:
> +
> +.globl xx
> +xx = x
> Index: test/ELF/Inputs/symver-archive2.s
> ===================================================================
> --- test/ELF/Inputs/symver-archive2.s
> +++ test/ELF/Inputs/symver-archive2.s
> @@ -0,0 +1 @@
> +call xx at PLT
> Index: test/ELF/Inputs/symver-archive1.s
> ===================================================================
> --- test/ELF/Inputs/symver-archive1.s
> +++ test/ELF/Inputs/symver-archive1.s
> @@ -0,0 +1,6 @@
> +.text
> +.globl x
> +.type x, @function
> +x:
> +
> +.symver x, xx@@VER
> Index: ELF/SymbolTable.cpp
> ===================================================================
> --- ELF/SymbolTable.cpp
> +++ ELF/SymbolTable.cpp
> @@ -211,6 +211,13 @@
>  // Find an existing symbol or create and insert a new one.
>  template <class ELFT>
>  std::pair<Symbol *, bool> SymbolTable<ELFT>::insert(StringRef Name) {
> +  // <name>@@<version> means the symbol is the default version. In that
> +  // case symbol <name> must exist and <name>@@<version> will be used to
> +  // resolve references to <name>.
> +  size_t Pos = Name.find("@@");
> +  if (Pos != StringRef::npos)
> +    Name = Name.take_front(Pos);
> +
>    auto P = Symtab.insert(
>        {CachedHashStringRef(Name), SymIndex((int)SymVector.size(), false)});
>    SymIndex &V = P.first->second;
> @@ -712,31 +719,14 @@
>        B->symbol()->VersionId = VersionId;
>  }
>  
> -static bool isDefaultVersion(SymbolBody *B) {
> -  return B->isInCurrentDSO() && B->getName().find("@@") != StringRef::npos;
> -}
> -
>  // This function processes version scripts by updating VersionId
>  // member of symbols.
>  template <class ELFT> void SymbolTable<ELFT>::scanVersionScript() {
>    // Symbol themselves might know their versions because symbols
>    // can contain versions in the form of <name>@<version>.
>    // Let them parse and update their names to exclude version suffix.
> -  for (Symbol *Sym : SymVector) {
> -    SymbolBody *Body = Sym->body();
> -    bool IsDefault = isDefaultVersion(Body);
> -    Body->parseSymbolVersion();
> -
> -    if (!IsDefault)
> -      continue;
> -
> -    // <name>@@<version> means the symbol is the default version. If that's the
> -    // case, the symbol is not used only to resolve <name> of version <version>
> -    // but also undefined unversioned symbols with name <name>.
> -    SymbolBody *S = find(Body->getName());
> -    if (S && S->isUndefined())
> -      S->copy(Body);
> -  }
> +  for (Symbol *Sym : SymVector)
> +    Sym->body()->parseSymbolVersion();
>  
>    // Handle edge cases first.
>    handleAnonymousVersion();
>
>
> Index: test/ELF/symver-archive.s
> ===================================================================
> --- test/ELF/symver-archive.s
> +++ test/ELF/symver-archive.s
> @@ -0,0 +1,15 @@
> +# REQUIRES: x86
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t1
> +# RUN: rm -f %t.a
> +# RUN: llvm-ar rcs %t.a %t1
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %S/Inputs/symver-archive1.s -o %t2.o
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %S/Inputs/symver-archive2.s -o %t3.o
> +# RUN: ld.lld -o %t.out %t2.o %t3.o %t.a
> +
> +.text
> +.globl x
> +.type x, @function
> +x:
> +
> +.globl xx
> +xx = x
> Index: test/ELF/Inputs/symver-archive2.s
> ===================================================================
> --- test/ELF/Inputs/symver-archive2.s
> +++ test/ELF/Inputs/symver-archive2.s
> @@ -0,0 +1 @@
> +call xx at PLT
> Index: test/ELF/Inputs/symver-archive1.s
> ===================================================================
> --- test/ELF/Inputs/symver-archive1.s
> +++ test/ELF/Inputs/symver-archive1.s
> @@ -0,0 +1,6 @@
> +.text
> +.globl x
> +.type x, @function
> +x:
> +
> +.symver x, xx@@VER
> Index: ELF/SymbolTable.cpp
> ===================================================================
> --- ELF/SymbolTable.cpp
> +++ ELF/SymbolTable.cpp
> @@ -211,6 +211,13 @@
>  // Find an existing symbol or create and insert a new one.
>  template <class ELFT>
>  std::pair<Symbol *, bool> SymbolTable<ELFT>::insert(StringRef Name) {
> +  // <name>@@<version> means the symbol is the default version. In that
> +  // case symbol <name> must exist and <name>@@<version> will be used to
> +  // resolve references to <name>.
> +  size_t Pos = Name.find("@@");
> +  if (Pos != StringRef::npos)
> +    Name = Name.take_front(Pos);
> +
>    auto P = Symtab.insert(
>        {CachedHashStringRef(Name), SymIndex((int)SymVector.size(), false)});
>    SymIndex &V = P.first->second;
> @@ -712,31 +719,14 @@
>        B->symbol()->VersionId = VersionId;
>  }
>  
> -static bool isDefaultVersion(SymbolBody *B) {
> -  return B->isInCurrentDSO() && B->getName().find("@@") != StringRef::npos;
> -}
> -
>  // This function processes version scripts by updating VersionId
>  // member of symbols.
>  template <class ELFT> void SymbolTable<ELFT>::scanVersionScript() {
>    // Symbol themselves might know their versions because symbols
>    // can contain versions in the form of <name>@<version>.
>    // Let them parse and update their names to exclude version suffix.
> -  for (Symbol *Sym : SymVector) {
> -    SymbolBody *Body = Sym->body();
> -    bool IsDefault = isDefaultVersion(Body);
> -    Body->parseSymbolVersion();
> -
> -    if (!IsDefault)
> -      continue;
> -
> -    // <name>@@<version> means the symbol is the default version. If that's the
> -    // case, the symbol is not used only to resolve <name> of version <version>
> -    // but also undefined unversioned symbols with name <name>.
> -    SymbolBody *S = find(Body->getName());
> -    if (S && S->isUndefined())
> -      S->copy(Body);
> -  }
> +  for (Symbol *Sym : SymVector)
> +    Sym->body()->parseSymbolVersion();
>  
>    // Handle edge cases first.
>    handleAnonymousVersion();