[ELF] Don't copy STT_TLS in copy relocation

[MaskRay]

Summary

During copy relocation of a variable defined in a DSO, if a TLS variable in that DSO happens to have the same st_value, it would also be copied. This was incorrect because the addresses of TLS variables are relative to TLS segment. They don't interfere with non-TLS variables.

This copying behavior can be harmful in the following scenario:

For function-scope thread-local variables with non-trivial constructors,
they have guard variables. In the case of x86_64 general-dynamic model:

thread_local std::string a;

GOT[n]   R_X86_64_DTPMOD64 guard variable for a
GOT[n+1] R_X86_64_DTPOFF64 guard variable for a
GOT[n+2] R_X86_64_DTPMOD64 a
GOT[n+3] R_X86_64_DTPOFF64 a

a and its guard variable are both represented as TLS variables, which
should be within the same module. If one is copy relocated to the main
module while the other is not, their module ID will mismatch and can
cause access without prior construction.

[ruiu]

Why?

[ruiu]

LGTM

ELF/Relocations.cpp

469

nit: I'd check the value equality at the last of these boolean expressions. (i.e. instead of adding your new expression at the end, insert it to the place where it feels more natural, which is after the comparison against SHN_ABS).

[MaskRay]

Address ruiu's comment

[echristo]

ELF/Relocations.cpp

468

Drive by comment: It'd be great to get a comment about everything in this conditional :)

[MaskRay]

ELF/Relocations.cpp

468

Agree. I didn't comment them because I still don't understand the stuff enough and feel comfortable to document it :( Hope the conditions are comprehensive now.

ELF/Relocations.cpp

 template <class ELFT>
 static SmallSet<SharedSymbol *, 4> getSymbolsAt(SharedSymbol &SS) {
   typedef typename ELFT::Sym Elf_Sym;
 
   SharedFile<ELFT> &File = SS.getFile<ELFT>();
 
   SmallSet<SharedSymbol *, 4> Ret;
   for (const Elf_Sym &S : File.getGlobalELFSyms()) {
     if (S.st_shndx == SHN_UNDEF || S.st_shndx == SHN_ABS ||
-        S.st_value != SS.Value)
+        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);
   }
   return Ret;
 }

test/ELF/Inputs/copy-rel-tls.s

+.bss
+.global foo
+.type foo, @object
+.size foo, 4
+foo:
+
+.section .tbss,"awT",@nobits
+.global tfoo
+.skip 0x2000
+.type tfoo,@object
+.size tfoo, 4
+tfoo:

test/ELF/copy-rel-tls.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-tls.s -o %t1.o
+// RUN: ld.lld %t1.o -shared -soname t1.so -o %t1.so
+// RUN: ld.lld %t.o %t1.so -o %t
+// RUN: llvm-nm %t1.so | FileCheck %s
+// RUN: llvm-nm %t | FileCheck --check-prefix=TLS %s
+// foo and tfoo have the same st_value but we should not copy tfoo.
+// CHECK: 2000 B foo
+// CHECK: 2000 B tfoo
+// TLS-NOT: tfoo
+
+.global _start
+_start:
+  leaq foo, %rax