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 the link has both foo and foo@ver.
+    // The reason this happens is because given
+    // .symver foo, bar@@VER
+    // foo has to be global for bar@@VER to be global. Since we don't want to export
+    // two symbols, it normally looks like
+    // .symver foo, foo@@VER
+    Sym->body()->setName(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 isShared() const { return SymbolKind == SharedKind; }
   bool isInCurrentDSO() const {
     return !isUndefined() && !isShared() && !isLazy();
   }
   bool isLocal() const { return IsLocal; }
   bool isPreemptible() const;
   StringRef getName() const { return Name; }
+  void setName(StringRef Val) { Name = Val; }
   uint8_t getVisibility() const { return StOther & 0x3; }
   void parseSymbolVersion();
   void copy(SymbolBody *Other);
 
   bool isInGot() const { return GotIndex != -1U; }
   bool isInPlt() const { return PltIndex != -1U; }
 
   uint64_t getVA(int64_t Addend = 0) const;
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