[ELF] - fixed not properly handled @GOTTPOFF relocation against local symbols.

[grimar]

Summary

This patch changes sequence of applying relocations, moving tls optimized relocation handling code before code for other locals.
Without that change relocation @GOTTPOFF against local symbol caused runtime error ("unrecognized reloc ...").
That change also should fix other tls optimized relocations, but I did not check them, thats a field for another patch.

R_X86_64_GOTTPOFF relocations agains locals can be found when linking against libc.a(malloc.o):
000000000036 000600000016 R_X86_64_GOTTPOFF 0000000000000000 libc_tsd_MALLOC - 4
000000000131 000600000016 R_X86_64_GOTTPOFF 0000000000000000 libc_tsd_MALLOC - 4
...

[ruiu]

ELF/InputSection.cpp

187 ↗

(On Diff #43024)

This is an unrelated change to the reordering of TLS optimization code, no? If so, please split this patch into two patches.

[grimar]

ELF/InputSection.cpp

187 ↗

(On Diff #43024)

Actually it is related. For local symbols getLocalRelTarget() is used to obtain SymVA. Previously it calculated addend inside and always used it for result. But for this relocation addend should not be taken in account as it is optimized in relocateTlsOptimize().
Thats why I had to add addend as argument here.

[ruiu]

LGTM

ELF/InputSection.cpp

193 ↗

(On Diff #43024)

Can you remove variable Body and use *PBody instead? It now feels a bit odd to define a new variable here just to dereference PBody.

[grimar]

ELF/InputSection.cpp

193 ↗

(On Diff #43024)

Ok, I did it because of 2 reasons:

1. Code that used Body assumed that it is not null, therefore reference object is the best for it.
2. Using of *PBody introduced lot of changes, almost each line was affected what was not very reasonable because of first reason for me.

But having another variable for the same object also not very good, here I agree. I renamed PBody to Body then, since PBody name appeared because Body was busy, and its also consistent with other code which uses Body name for pointers to SymbolBody.

lld/trunk/ELF/InputSection.cpp

for (size_t I = 0; I < Num; ++I) {
     if (Target->isTlsLocalDynamicReloc(Type) &&
         !Target->isTlsOptimized(Type, nullptr)) {
       Target->relocateOne(BufLoc, BufEnd, Type, AddrLoc,
                           Out<ELFT>::Got->getLocalTlsIndexVA() +
                               getAddend<ELFT>(RI));
       continue;
     }
 
+    const Elf_Shdr *SymTab = File->getSymbolTable();
+    SymbolBody *Body = nullptr;
+    if (SymIndex >= SymTab->sh_info)
+      Body = File->getSymbolBody(SymIndex)->repl();
+
+    if (Target->isTlsOptimized(Type, Body)) {
+      uintX_t SymVA;
+      if (!Body)
+        SymVA = getLocalRelTarget(*File, RI, 0);
+      else if (Target->relocNeedsGot(Type, *Body))
+        SymVA = Out<ELFT>::Got->getEntryAddr(*Body);
+      else
+        SymVA = getSymVA<ELFT>(*Body);
+      // By optimizing TLS relocations, it is sometimes needed to skip
+      // relocations that immediately follow TLS relocations. This function
+      // knows how many slots we need to skip.
+      I += Target->relocateTlsOptimize(BufLoc, BufEnd, Type, AddrLoc, SymVA,
+                                       *Body);
+      continue;
+    }
+
     // Handle relocations for local symbols -- they never get
     // resolved so we don't allocate a SymbolBody.
-    const Elf_Shdr *SymTab = File->getSymbolTable();
-    if (SymIndex < SymTab->sh_info) {
-      uintX_t SymVA = getLocalRelTarget(*File, RI);
+    uintX_t A = getAddend<ELFT>(RI);
+    if (!Body) {
+      uintX_t SymVA = getLocalRelTarget(*File, RI, A);
       Target->relocateOne(BufLoc, BufEnd, Type, AddrLoc, SymVA, 0,
                           findMipsPairedReloc(Buf, SymIndex, Type, NextRelocs));
       continue;
     }
 
-    SymbolBody &Body = *File->getSymbolBody(SymIndex)->repl();
-
     if (Target->isTlsGlobalDynamicReloc(Type) &&
-        !Target->isTlsOptimized(Type, &Body)) {
+        !Target->isTlsOptimized(Type, Body)) {
       Target->relocateOne(BufLoc, BufEnd, Type, AddrLoc,
-                          Out<ELFT>::Got->getGlobalDynAddr(Body) +
+                          Out<ELFT>::Got->getGlobalDynAddr(*Body) +
                               getAddend<ELFT>(RI));
       continue;
     }
 
-    if (Target->isTlsOptimized(Type, &Body)) {
-      uintX_t SymVA = Target->relocNeedsGot(Type, Body)
-                          ? Out<ELFT>::Got->getEntryAddr(Body)
-                          : getSymVA<ELFT>(Body);
-      // By optimizing TLS relocations, it is sometimes needed to skip
-      // relocations that immediately follow TLS relocations. This function
-      // knows how many slots we need to skip.
-      I += Target->relocateTlsOptimize(BufLoc, BufEnd, Type, AddrLoc, SymVA,
-                                       Body);
-      continue;
-    }
-
-    uintX_t SymVA = getSymVA<ELFT>(Body);
-    if (Target->relocNeedsPlt(Type, Body)) {
-      SymVA = Out<ELFT>::Plt->getEntryAddr(Body);
+    uintX_t SymVA = getSymVA<ELFT>(*Body);
+    if (Target->relocNeedsPlt(Type, *Body)) {
+      SymVA = Out<ELFT>::Plt->getEntryAddr(*Body);
       Type = Target->getPltRefReloc(Type);
-    } else if (Target->relocNeedsGot(Type, Body)) {
-      SymVA = Out<ELFT>::Got->getEntryAddr(Body);
-      if (Body.isTls())
+    } else if (Target->relocNeedsGot(Type, *Body)) {
+      SymVA = Out<ELFT>::Got->getEntryAddr(*Body);
+      if (Body->isTls())
         Type = Target->getTlsGotReloc(Type);
-    } else if (!Target->needsCopyRel(Type, Body) &&
-               isa<SharedSymbol<ELFT>>(Body)) {
+    } else if (!Target->needsCopyRel(Type, *Body) &&
+               isa<SharedSymbol<ELFT>>(*Body)) {
       continue;
-    } else if (Target->isTlsDynReloc(Type, Body) ||
-               Target->isSizeDynReloc(Type, Body)) {
+    } else if (Target->isTlsDynReloc(Type, *Body) ||
+               Target->isSizeDynReloc(Type, *Body)) {
       continue;
     } else if (Config->EMachine == EM_MIPS) {
-      if (Type == R_MIPS_HI16 && &Body == Config->MipsGpDisp)
+      if (Type == R_MIPS_HI16 && Body == Config->MipsGpDisp)
         SymVA = getMipsGpAddr<ELFT>() - AddrLoc;
-      else if (Type == R_MIPS_LO16 && &Body == Config->MipsGpDisp)
+      else if (Type == R_MIPS_LO16 && Body == Config->MipsGpDisp)
         SymVA = getMipsGpAddr<ELFT>() - AddrLoc + 4;
     }
-    uintX_t A = getAddend<ELFT>(RI);
-    uintX_t Size = getSymSize<ELFT>(Body);
+    uintX_t Size = getSymSize<ELFT>(*Body);
     Target->relocateOne(BufLoc, BufEnd, Type, AddrLoc, SymVA + A, Size + A,
                         findMipsPairedReloc(Buf, SymIndex, Type, NextRelocs));
   }
 }
 
 template <class ELFT> void InputSection<ELFT>::writeTo(uint8_t *Buf) {
   if (this->Header->sh_type == SHT_NOBITS)
     return;

lld/trunk/ELF/OutputSections.h

 }
 
 template <class ELFT>
 typename llvm::object::ELFFile<ELFT>::uintX_t getSymVA(const SymbolBody &S);
 
 template <class ELFT, bool IsRela>
 typename llvm::object::ELFFile<ELFT>::uintX_t
 getLocalRelTarget(const ObjectFile<ELFT> &File,
-                  const llvm::object::Elf_Rel_Impl<ELFT, IsRela> &Rel);
+                  const llvm::object::Elf_Rel_Impl<ELFT, IsRela> &Rel,
+                  typename llvm::object::ELFFile<ELFT>::uintX_t Addend);
 bool canBePreempted(const SymbolBody *Body, bool NeedsGot);
 template <class ELFT> bool includeInSymtab(const SymbolBody &B);
 
 bool includeInDynamicSymtab(const SymbolBody &B);
 
 template <class ELFT>
 bool shouldKeepInSymtab(
     const ObjectFile<ELFT> &File, StringRef Name,

lld/trunk/ELF/OutputSections.cpp

for (const DynamicReloc<ELFT> &Rel : Relocs) {
     uintX_t Addend;
     if (NeedsCopy)
       Addend = 0;
     else if (CanBePreempted || IsDynRelative)
       Addend = OrigAddend;
     else if (Body)
       Addend = getSymVA<ELFT>(cast<ELFSymbolBody<ELFT>>(*Body)) + OrigAddend;
     else if (IsRela)
-      Addend = getLocalRelTarget(File, static_cast<const Elf_Rela &>(RI));
+      Addend =
+          getLocalRelTarget(File, static_cast<const Elf_Rela &>(RI),
+                            getAddend<ELFT>(static_cast<const Elf_Rela &>(RI)));
     else
-      Addend = getLocalRelTarget(File, RI);
+      Addend = getLocalRelTarget(File, RI, 0);
 
     if (IsRela)
       static_cast<Elf_Rela *>(P)->r_addend = Addend;
   }
 }
 
 template <class ELFT> unsigned RelocationSection<ELFT>::getRelocOffset() {
   const unsigned EntrySize = IsRela ? sizeof(Elf_Rela) : sizeof(Elf_Rel);
typename ELFFile<ELFT>::uintX_t lld::elf2::getSymVA(const SymbolBody &S) {
   llvm_unreachable("Invalid symbol kind");
 }
 
 // Returns a VA which a relocatin RI refers to. Used only for local symbols.
 // For non-local symbols, use getSymVA instead.
 template <class ELFT, bool IsRela>
 typename ELFFile<ELFT>::uintX_t
 lld::elf2::getLocalRelTarget(const ObjectFile<ELFT> &File,
-                             const Elf_Rel_Impl<ELFT, IsRela> &RI) {
+                             const Elf_Rel_Impl<ELFT, IsRela> &RI,
+                             typename ELFFile<ELFT>::uintX_t Addend) {
   typedef typename ELFFile<ELFT>::Elf_Sym Elf_Sym;
   typedef typename ELFFile<ELFT>::uintX_t uintX_t;
 
-  uintX_t Addend = getAddend<ELFT>(RI);
-
   // PPC64 has a special relocation representing the TOC base pointer
   // that does not have a corresponding symbol.
   if (Config->EMachine == EM_PPC64 && RI.getType(false) == R_PPC64_TOC)
     return getPPC64TocBase() + Addend;
 
   const Elf_Sym *Sym =
       File.getObj().getRelocationSymbol(&RI, File.getSymbolTable());
 

 
 template ELFFile<ELF32LE>::uintX_t getSymVA<ELF32LE>(const SymbolBody &);
 template ELFFile<ELF32BE>::uintX_t getSymVA<ELF32BE>(const SymbolBody &);
 template ELFFile<ELF64LE>::uintX_t getSymVA<ELF64LE>(const SymbolBody &);
 template ELFFile<ELF64BE>::uintX_t getSymVA<ELF64BE>(const SymbolBody &);
 
 template ELFFile<ELF32LE>::uintX_t
 getLocalRelTarget(const ObjectFile<ELF32LE> &,
-                  const ELFFile<ELF32LE>::Elf_Rel &);
-template ELFFile<ELF32BE>::uintX_t
-getLocalRelTarget(const ObjectFile<ELF32BE> &,
-                  const ELFFile<ELF32BE>::Elf_Rel &);
-template ELFFile<ELF64LE>::uintX_t
-getLocalRelTarget(const ObjectFile<ELF64LE> &,
-                  const ELFFile<ELF64LE>::Elf_Rel &);
-template ELFFile<ELF64BE>::uintX_t
-getLocalRelTarget(const ObjectFile<ELF64BE> &,
-                  const ELFFile<ELF64BE>::Elf_Rel &);
-
-template ELFFile<ELF32LE>::uintX_t
-getLocalRelTarget(const ObjectFile<ELF32LE> &,
-                  const ELFFile<ELF32LE>::Elf_Rela &);
+                  const ELFFile<ELF32LE>::Elf_Rel &,
+                  ELFFile<ELF32LE>::uintX_t Addend);
 template ELFFile<ELF32BE>::uintX_t
 getLocalRelTarget(const ObjectFile<ELF32BE> &,
-                  const ELFFile<ELF32BE>::Elf_Rela &);
+                  const ELFFile<ELF32BE>::Elf_Rel &,
+                  ELFFile<ELF32BE>::uintX_t Addend);
 template ELFFile<ELF64LE>::uintX_t
 getLocalRelTarget(const ObjectFile<ELF64LE> &,
-                  const ELFFile<ELF64LE>::Elf_Rela &);
+                  const ELFFile<ELF64LE>::Elf_Rel &,
+                  ELFFile<ELF64LE>::uintX_t Addend);
 template ELFFile<ELF64BE>::uintX_t
 getLocalRelTarget(const ObjectFile<ELF64BE> &,
-                  const ELFFile<ELF64BE>::Elf_Rela &);
+                  const ELFFile<ELF64BE>::Elf_Rel &,
+                  ELFFile<ELF64BE>::uintX_t Addend);
 
 template bool includeInSymtab<ELF32LE>(const SymbolBody &);
 template bool includeInSymtab<ELF32BE>(const SymbolBody &);
 template bool includeInSymtab<ELF64LE>(const SymbolBody &);
 template bool includeInSymtab<ELF64BE>(const SymbolBody &);
 
 template bool shouldKeepInSymtab<ELF32LE>(const ObjectFile<ELF32LE> &,
                                           StringRef,

lld/trunk/test/ELF/tls-opt-local.s

+// RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+// RUN: ld.lld %t.o -o %t1
+// RUN: llvm-readobj -r %t1 | FileCheck --check-prefix=NORELOC %s
+// RUN: llvm-objdump -d %t1 | FileCheck --check-prefix=DISASM %s
+
+// NORELOC:      Relocations [
+// NORELOC-NEXT: ]
+
+// DISASM:      Disassembly of section .text:
+// DISASM-NEXT: _start:
+// DISASM-NEXT: 11000: 48 c7 c0 f8 ff ff ff movq $-8, %rax
+// DISASM-NEXT: 11007: 49 c7 c7 f8 ff ff ff movq $-8, %r15
+// DISASM-NEXT: 1100e: 48 8d 80 f8 ff ff ff leaq -8(%rax), %rax
+// DISASM-NEXT: 11015: 4d 8d bf f8 ff ff ff leaq -8(%r15), %r15
+// DISASM-NEXT: 1101c: 48 81 c4 f8 ff ff ff addq $-8, %rsp
+// DISASM-NEXT: 11023: 49 81 c4 f8 ff ff ff addq $-8, %r12
+// DISASM-NEXT: 1102a: 48 c7 c0 fc ff ff ff movq $-4, %rax
+// DISASM-NEXT: 11031: 49 c7 c7 fc ff ff ff movq $-4, %r15
+// DISASM-NEXT: 11038: 48 8d 80 fc ff ff ff leaq -4(%rax), %rax
+// DISASM-NEXT: 1103f: 4d 8d bf fc ff ff ff leaq -4(%r15), %r15
+// DISASM-NEXT: 11046: 48 81 c4 fc ff ff ff addq $-4, %rsp
+// DISASM-NEXT: 1104d: 49 81 c4 fc ff ff ff addq $-4, %r12
+
+.section .tbss,"awT",@nobits
+
+.type tls0,@object
+.align 4
+tls0:
+ .long 0
+ .size tls0, 4
+
+.type  tls1,@object
+.align 4
+tls1:
+ .long 0
+ .size tls1, 4
+
+.section .text
+.globl _start
+_start:
+ movq tls0@GOTTPOFF(%rip), %rax
+ movq tls0@GOTTPOFF(%rip), %r15
+ addq tls0@GOTTPOFF(%rip), %rax
+ addq tls0@GOTTPOFF(%rip), %r15
+ addq tls0@GOTTPOFF(%rip), %rsp
+ addq tls0@GOTTPOFF(%rip), %r12
+ movq tls1@GOTTPOFF(%rip), %rax
+ movq tls1@GOTTPOFF(%rip), %r15
+ addq tls1@GOTTPOFF(%rip), %rax
+ addq tls1@GOTTPOFF(%rip), %r15
+ addq tls1@GOTTPOFF(%rip), %rsp
+ addq tls1@GOTTPOFF(%rip), %r12