Bring back r307364

[• espindola]

Summary

This includes an extra hack to handle a file having both foo and foo@@ver.

Another option is checking with the user if they can juts change their code. For what it is worth it, gold produces the same result as the original patch.

With this patch we produce the same result as bfd.

[ruiu]

ELF/SymbolTable.cpp

246

We also reach here if foo and foo@ver come from different input file, no?

ELF/Symbols.h

148

I wouldn't remove this because I want to hide the fact that the name is represented as StringRefZ instead of StringRef from outside of this class.

[• rafael]

Actually, given how symver works I am pretty much convinced that having foo and foo@@ver in the same .o file is the only way to handle versioning without having to copy symbol names to a version scirpt.

[emaste]

Handy Phabricator link: rL307364

So this case just doesn't work with gold?

[• rafael]

This should address the review comments.

Given that this effectively requires foo to be weak in

.symver foo, foo@@bar

I will try an alternative solution too.

[smeenai]

I'm probably just misunderstanding the discussion here, but bfd complains about multiple definitions if you do .symver x, x@@VER: https://ghostbin.com/paste/c9b4t. Granted, I'm on an old version of binutils, so maybe that's changed?

[smeenai]

(from @rafael's email) Correct, bfd complains. That is why you have to make foo weak, which is annoying.

Okay, that makes sense. Are we not necessarily trying to mimic bfd's behavior then?

I'm slightly confused in general (and I apologize for jumping in without having the full context, but I've had to deal a bunch of symbol versioning issues internally, and I'm also working on switching things over to LLD, so I want to make sure I understand LLD's intended behavior for versioning). From what I understand, this patch is meant to restore https://reviews.llvm.org/D35059 while fixing the case in https://bugs.llvm.org/show_bug.cgi?id=33820. Shouldn't there be an additional test case for the issue in that bug? As far as I can see the test cases here are the same as the ones from https://reviews.llvm.org/D35059.

ELF/SymbolTable.cpp

246

This is meant to address the case from https://bugs.llvm.org/show_bug.cgi?id=33820, right?

[smeenai]

Thanks for the explanations @rafael. Everything makes sense now.

As someone not super familiar with LLD's code, I find D35612 a lot easier to understand. Both work fine for all the cases I was concerned with, however. Thanks.

ELF/SymbolTable.cpp

if (VA == STV_DEFAULT)
     return VB;
   if (VB == STV_DEFAULT)
     return VA;
   return std::min(VA, VB);
 }
 
 // Find an existing symbol or create and insert a new one.
 template <class ELFT>
-std::pair<Symbol *, bool> SymbolTable<ELFT>::insert(StringRef Name) {
+std::pair<Symbol *, bool> SymbolTable<ELFT>::insert(StringRef OrigName) {
+  // <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>.
+  StringRef Name = OrigName;
+  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;
   bool IsNew = P.second;
 
   if (V.Idx == -1) {
     IsNew = true;
     V = SymIndex((int)SymVector.size(), true);
   }
 
   Symbol *Sym;
   if (IsNew) {
     Sym = make<Symbol>();
     Sym->InVersionScript = false;
     Sym->Binding = STB_WEAK;
     Sym->Visibility = STV_DEFAULT;
     Sym->IsUsedInRegularObj = false;
     Sym->ExportDynamic = false;
     Sym->Traced = V.Traced;
     Sym->VersionId = Config->DefaultSymbolVersion;
     SymVector.push_back(Sym);
   } else {
     Sym = SymVector[V.Idx];
+
+    // This is for the case where an input file has both foo and
+    // foo@ver. This is a really odd case, but has been observed in
+    // the wild and the expected behavior is to give precedence to
+    // keeping the version info.
+    Sym->body()->Name = OrigName;
   }
   return {Sym, IsNew};
 }
 
 // Find an existing symbol or create and insert a new one, then apply the given
 // attributes.
 template <class ELFT>
 std::pair<Symbol *, bool>
void SymbolTable<ELFT>::assignWildcardVersion(SymbolVersion Ver,
   // Exact matching takes precendence over fuzzy matching,
   // so we set a version to a symbol only if no version has been assigned
   // to the symbol. This behavior is compatible with GNU.
   for (SymbolBody *B : findAllByVersion(Ver))
     if (B->symbol()->VersionId == Config->DefaultSymbolVersion)
       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);
-  }
-
   // Handle edge cases first.
   handleAnonymousVersion();
 
   // Now we have version definitions, so we need to set version ids to symbols.
   // Each version definition has a glob pattern, and all symbols that match
   // with the pattern get that version.
 
   // First, we assign versions to exact matching symbols,

ELF/Symbols.h

public:
 
   bool isSection() const { return Type == llvm::ELF::STT_SECTION; }
   bool isTls() const { return Type == llvm::ELF::STT_TLS; }
   bool isFunc() const { return Type == llvm::ELF::STT_FUNC; }
   bool isGnuIFunc() const { return Type == llvm::ELF::STT_GNU_IFUNC; }
   bool isObject() const { return Type == llvm::ELF::STT_OBJECT; }
   bool isFile() const { return Type == llvm::ELF::STT_FILE; }
 
-protected:
   StringRefZ Name;
 };
 
 // The base class for any defined symbols.
 class Defined : public SymbolBody {
 public:
   Defined(Kind K, StringRefZ Name, bool IsLocal, uint8_t StOther, uint8_t Type);
   static bool classof(const SymbolBody *S) { return S->isDefined(); }

test/ELF/Inputs/symver-archive1.s

+.text
+.globl x
+.type x, @function
+x:
+
+.symver x, xx@@VER

test/ELF/Inputs/symver-archive2.s

+call xx@PLT

test/ELF/symver-archive.s

+# 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