[ELF] Set Out::TlsPhdr earlier for encoding packed reloc tables

[rprichard]

Summary

For --pack-dyn-relocs=android, finalizeSections calls
LinkerScript::assignAddresses and
AndroidPackedRelocationSection::updateAllocSize in a loop,
where assignAddresses lays out the ELF image, then updateAllocSize
determines the size of the Android packed relocation table by encoding it.
Encoding the table requires knowing the values of relocation addends.

To get the addend of a TLS relocation, updateAllocSize can call getSymVA
on a TLS symbol before setPhdrs has initialized Out::TlsPhdr, producing an
error:

<file> has an STT_TLS symbol but doesn't have an SHF_TLS section

Fix the problem by initializing Out::TlsPhdr immediately after the program
headers are created. The segment's p_vaddr field isn't initialized until
setPhdrs, so use FirstSec->Addr, which is what setPhdrs would use.
FirstSec will typically refer to the .tdata or .tbss output section, whose
(tentative) address was computed by assignAddresses.

Android currently avoids this problem because it uses emutls and doesn't
support ELF TLS. This problem doesn't apply to --pack-dyn-relocs=relr
because SHR_RELR only handles relative relocations without explicit addends
or info.

Fixes https://bugs.llvm.org/show_bug.cgi?id=37841.

[srhines]

Thanks for the detailed explanations. I only had one minor nitpick.

ELF/Writer.cpp

1654

const?

[rprichard]

ELF/Writer.cpp

1654

We'd also have to make Out::TlsPhdr point to a const PhdrEntry. Do we want to do that? The rest of the section fields in InX and Out aren't const.

[srhines]

ELF/Writer.cpp

1654

That should be decided by the main authors of LLD, but it seems like const would work fine on the declaration today for all of the existing uses.

[rprichard]

Make Out::TlsPhdr a pointer to const.

[srhines]

Thanks for the update. I'll let the project maintainers give a final LGTM, but I'm happy with it.

[rprichard]

Ping.

[ruiu]

ELF/OutputSections.h

132 ↗

(On Diff #164112)

Seems like an unrelated change.

ELF/Writer.cpp

1654

We normally don't add const; instead we keep variable scope as short as possible to make it obviously const. Please drop it.

1654

For consistency, add {}. We do

for (...)
  if (...)
    ...;

but don't do this:

for (...)
  if (...) {
    ...
  }

But perhaps the best way of writing this code is

for (...) {
  if (!condition)
    continue;
  ...;
  break;
}

1654

It needs comments. Essentially, all nontrivial code needs comments so that the code can be read and understood by a first-time reader.

[rprichard]

I removed const from Out::TlsPhdr and added comments.

I simplified the TLS segment search loop. It could break out of the loop once it finds the segment, which I suppose could be faster, but it doesn't worth the extra lines of code.

[ruiu]

LGTM

ELF/Symbols.cpp

case Symbol::DefinedKind: {
     // line (and how they get built), then you have a pretty good
     // understanding of the linker.
     uint64_t VA = IS->getVA(Offset);
 
     if (D.isTls() && !Config->Relocatable) {
       if (!Out::TlsPhdr)
         fatal(toString(D.File) +
               " has an STT_TLS symbol but doesn't have an SHF_TLS section");
-      return VA - Out::TlsPhdr->p_vaddr;
+      return VA - Out::TlsPhdr->FirstSec->Addr;
     }
     return VA;
   }
   case Symbol::SharedKind:
   case Symbol::UndefinedKind:
     return 0;
   case Symbol::LazyArchiveKind:
   case Symbol::LazyObjectKind:

ELF/Writer.cpp

template <class ELFT> void Writer<ELFT>::finalizeSections() {
 
   // Binary and relocatable output does not have PHDRS.
   // The headers have to be created before finalize as that can influence the
   // image base and the dynamic section on mips includes the image base.
   if (!Config->Relocatable && !Config->OFormatBinary) {
     Phdrs = Script->hasPhdrsCommands() ? Script->createPhdrs() : createPhdrs();
     addPtArmExid(Phdrs);
     Out::ProgramHeaders->Size = sizeof(Elf_Phdr) * Phdrs.size();
+
+    for (PhdrEntry *P : Phdrs)
+      if (P->p_type == PT_TLS && P->FirstSec) {
+        Out::TlsPhdr = P;
+        break;
+      }
   }
 
   // Some symbols are defined in term of program headers. Now that we
   // have the headers, we can find out which sections they point to.
   setReservedSymbolSections();
 
   // Dynamic section must be the last one in this list and dynamic
   // symbol table section (DynSymTab) must be the first one.
else if (P->p_type == PT_GNU_RELRO) {
       // The glibc dynamic loader rounds the size down, so we need to round up
       // to protect the last page. This is a no-op on FreeBSD which always
       // rounds up.
       P->p_memsz = alignTo(P->p_memsz, Target->PageSize);
     }
 
     // The TLS pointer goes after PT_TLS. At least glibc will align it,
     // so round up the size to make sure the offsets are correct.
-    if (P->p_type == PT_TLS) {
-      Out::TlsPhdr = P;
-      if (P->p_memsz)
-        P->p_memsz = alignTo(P->p_memsz, P->p_align);
-    }
-  }
-}
+    if (P->p_type == PT_TLS && P->p_memsz)
 
 // A helper struct for checkSectionOverlap.
 namespace {
 struct SectionOffset {
   OutputSection *Sec;
   uint64_t Offset;
 };
 } // namespace

test/ELF/pack-dyn-relocs-tls-aarch64.s

+// REQUIRES: aarch64
+
+// RUN: llvm-mc -filetype=obj -triple=aarch64-unknown-linux %s -o %t.o
+// RUN: ld.lld -shared --pack-dyn-relocs=android %t.o -o %t.so
+// RUN: llvm-readobj -relocations %t.so | FileCheck %s
+
+// Bug 37841: Symbol::getVA must work on TLS symbols during the layout loop in
+// finalizeSections.
+
+    .global foo
+foo:
+    adrp    x0, :tlsdesc:tlsvar1
+    ldr     x1, [x0, :tlsdesc_lo12:tlsvar1]
+    add     x0, x0, :tlsdesc_lo12:tlsvar1
+    .tlsdesccall tlsvar1
+
+// Also test an atypical IE access from a shared object to a local TLS symbol.
+
+    .global bar
+bar:
+    adrp    x0, :gottprel:tlsvar2
+    ldr     x0, [x0, #:gottprel_lo12:tlsvar2]
+
+    .section    .tdata,"awT",@progbits
+    .space  0x1234
+tlsvar1:
+    .word   42
+tlsvar2:
+    .word   17
+
+// CHECK:          Section ({{.+}}) .rela.dyn {
+// CHECK-NEXT:     R_AARCH64_TLSDESC - 0x1234
+// CHECK-NEXT:     R_AARCH64_TLS_TPREL64 - 0x1238
+// CHECK-NEXT:     }

test/ELF/pack-dyn-relocs-tls-x86-64.s

+// REQUIRES: x86
+
+// RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+// RUN: ld.lld -shared --pack-dyn-relocs=android %t.o -o %t.so
+// RUN: llvm-readobj -relocations %t.so | FileCheck %s
+
+// Bug 37841: Symbol::getVA must work on TLS symbols during the layout loop in
+// finalizeSections. This test uses an atypical IE access in a shared object to
+// access a local TLS symbol, because a more typical access would avoid the
+// bug.
+
+    .globl  foo
+foo:
+    movq    tlsvar@GOTTPOFF(%rip), %rcx
+
+    .section    .tdata,"awT",@progbits
+    .space 0x1234
+tlsvar:
+    .word   42
+
+// CHECK:          Section ({{.+}}) .rela.dyn {
+// CHECK-NEXT:     R_X86_64_TPOFF64 - 0x1234
+// CHECK-NEXT:     }