[ELF] Support copy relocation on non-default version symbols

[MaskRay]

Summary

Copy relocation on a non-default version symbol is unsupported and can crash at
runtime. Fortunately there is a one-line fix which works for most cases:
ensure getSymbolsAt unconditionally returns ss.

If two non-default version symbols are defined at the same place and both
are copy relocated, our implementation will copy relocated them into different
addresses. The pointer inequality is very unlikely an issue. In GNU ld, copy
relocating version aliases seems to create more pointer inequality problems than
us.

(
In glibc, sys_errlist@GLIBC_2.2.5 sys_errlist@GLIBC_2.3 sys_errlist@GLIBC_2.4
are defined at the same place, but it is unlikely they are all copy relocated in
one executable. Even if so, the variables are read-only and pointer inequality
should not be a problem.
)

[peter.smith]

I think this is worth doing as an improvement over where we are today. A quick empirical test with GNU ld by extending copy-rel-version.s with an extra non-default version and 2 separate copy-relocs to each non-default version also results in two separate copy relocations to two separate locations. So I don't think that there is likely to be a program today that relies on the addresses being identical, although there may be in the future.

IIUC if we were to fix the alias problem then we'd need to load the versym section and get the version info for each alias candidate so that we'd do find("sym@ver") rather than ("sym").

[MaskRay]

Thanks!

IIUC if we were to fix the alias problem then we'd need to load the versym section and get the version info for each alias candidate so that we'd do find("sym@ver") rather than ("sym").

Yes. I have actually implemented it but realized that it is an overkill.

diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index d5b9efbe18fc..e3cc3821ffad 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -1608,6 +1608,9 @@ template <class ELFT> void SharedFile::parse() {
                                    sym.st_other, sym.getType(), sym.st_value,
                                    sym.st_size, alignment, idx});
   }
+
+  if (versymSec)
+    this->versyms = std::move(versyms);
 }
 
 static ELFKind getBitcodeELFKind(const Triple &t) {
diff --git a/lld/ELF/InputFiles.h b/lld/ELF/InputFiles.h
index bd72cfcdd050..bc01ad31e1cd 100644
--- a/lld/ELF/InputFiles.h
+++ b/lld/ELF/InputFiles.h
@@ -367,6 +367,8 @@ public:
   // This is actually a vector of Elf_Verdef pointers.
   std::vector<const void *> verdefs;
 
+  std::vector<uint16_t> versyms;
+
   // If the output file needs Elf_Verneed data structures for this file, this is
   // a vector of Elf_Vernaux version identifiers that map onto the entries in
   // Verdefs, otherwise it is empty.
diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp
index e3cc210972b2..778b2086581a 100644
--- a/lld/ELF/Relocations.cpp
+++ b/lld/ELF/Relocations.cpp
@@ -516,13 +516,27 @@ static SmallSet<SharedSymbol *, 4> getSymbolsAt(SharedSymbol &ss) {
   using Elf_Sym = typename ELFT::Sym;
 
   SharedFile &file = ss.getFile();
-
+  SmallString<128> buf;
   SmallSet<SharedSymbol *, 4> ret;
-  for (const Elf_Sym &s : file.template getGlobalELFSyms<ELFT>()) {
+  for (auto it : enumerate(file.template getGlobalELFSyms<ELFT>())) {
+    const Elf_Sym &s = it.value();
     if (s.st_shndx == SHN_UNDEF || s.st_shndx == SHN_ABS ||
         s.getType() == STT_TLS || s.st_value != ss.value)
       continue;
     StringRef name = check(s.getName(file.getStringTable()));
+    // Append @ suffix if the symbol has a non-default version.
+    const uint16_t idx =
+        file.versyms.empty() ? VER_NDX_GLOBAL : file.versyms[it.index()];
+    if ((idx & VERSYM_HIDDEN) && (idx & ~VERSYM_HIDDEN) > VER_NDX_GLOBAL) {
+      StringRef verName = file.getStringTable().data() +
+                          reinterpret_cast<const typename ELFT::Verdef *>(
+                              file.verdefs[idx & ~VERSYM_HIDDEN])
+                              ->getAux()
+                              ->vda_name;
+      buf.clear();
+      name = (name + "@" + verName).toStringRef(buf);
+    }
+
     Symbol *sym = symtab->find(name);
     if (auto *alias = dyn_cast_or_null<SharedSymbol>(sym))
       ret.insert(alias);

[MaskRay]

update comments

lld/ELF/Relocations.cpp

for (const Elf_Sym &s : file.template getGlobalELFSyms<ELFT>()) {
     if (s.st_shndx == SHN_UNDEF || s.st_shndx == SHN_ABS ||
         s.getType() == STT_TLS || s.st_value != ss.value)
       continue;
     StringRef name = check(s.getName(file.getStringTable()));
     Symbol *sym = symtab->find(name);
     if (auto *alias = dyn_cast_or_null<SharedSymbol>(sym))
       ret.insert(alias);
   }
+
+  // The loop does not check SHT_GNU_verneed, so ret does not contain
+  // non-default version symbols. If ss has a non-default version, ret won't
+  // contain ss. Just add ss unconditionally. If a non-default version alias is
+  // separately copy relocated, it and ss will have different addresses.
+  // Fortunately this case is impractical and fails with GNU ld as well.
+  ret.insert(&ss);
   return ret;
 }
 
 // When a symbol is copy relocated or we create a canonical plt entry, it is
 // effectively a defined symbol. In the case of copy relocation the symbol is
 // in .bss and in the case of a canonical plt entry it is in .plt. This function
 // replaces the existing symbol with a Defined pointing to the appropriate
 // location.

lld/test/ELF/Inputs/copy-rel-version.s

 .data
-.global foo@v1
-.type foo@v1, @object
-.size foo@v1, 4
-.global foo@@v2
-.type foo@@v2, @object
-.size foo@@v2, 8
-foo@v1:
-foo@@v2:
+.global foo_v1
+.symver foo_v1, foo@v1, remove
+.type foo_v1, @object
+.size foo_v1, 4
+
+.global foo_v2
+.symver foo_v2, foo@v2, remove
+.type foo_v2, @object
+.size foo_v2, 8
+
+.global foo
+.symver foo, foo@@@v3
+.type foo, @object
+.size foo, 12
+
+foo_v1:
+foo_v2:
+foo:
+.int 0
 .int 0
 .int 0

lld/test/ELF/copy-rel-version.s

-// REQUIRES: x86
-// RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
-// RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %p/Inputs/copy-rel-version.s -o %t1.o
-// RUN: echo "v1 {}; v2 {};" > %t.ver
-// RUN: ld.lld %t1.o -shared -soname t1.so --version-script=%t.ver -o %t1.so
-// RUN: ld.lld %t.o %t1.so -o %t
-// RUN: llvm-readobj --symbols %t | FileCheck %s
+# REQUIRES: x86
+## Copy relocate a versioned symbol which has a versioned alias.
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t1.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %p/Inputs/copy-rel-version.s -o %t.o
+# RUN: echo 'v1 {}; v2 {}; v3 {};' > %t.ver
+# RUN: ld.lld %t.o -shared -soname t.so --version-script=%t.ver -o %t.so
+
+## Copy relocate the default version symbol.
+# RUN: ld.lld %t1.o %t.so -o %t1
+# RUN: llvm-readelf --dyn-syms %t1 | FileCheck %s --check-prefix=CHECK1
+
+# CHECK1:       1: {{.+}}            12 OBJECT  GLOBAL DEFAULT [[#]] foo@v3
+# CHECK1-EMPTY:
+
+## Copy relocate the non-default version symbol.
+# RUN: llvm-objcopy --redefine-sym foo=foo@v1 %t1.o %t2.o
+# RUN: ld.lld %t2.o %t.so -o %t2
+# RUN: llvm-readelf --dyn-syms %t2 | FileCheck %s --check-prefix=CHECK2
+
+# CHECK2:       1: [[ADDR:[0-9a-f]+]] 4 OBJECT  GLOBAL DEFAULT [[#]] foo@v1
+# CHECK2-NEXT:  2: [[ADDR]]          12 OBJECT  GLOBAL DEFAULT [[#]] foo@v3
+# CHECK2-EMPTY:
 
 .global _start
 _start:
   leaq foo, %rax
-
-// CHECK:      Name: foo (
-// CHECK-NEXT: Value:
-// CHECK-NEXT: Size: 8