[PATCH] D62059: [ELF] Don't align PT_TLS's p_memsz

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D62059#1507362 <https://reviews.llvm.org/D62059#1507362>, @MaskRay wrote:

> In D62059#1507017 <https://reviews.llvm.org/D62059#1507017>, @rprichard wrote:
>
> > I had suggested doing something like this at https://reviews.llvm.org/D53906#1326683. I like the idea, but I think we need to fix up the variant 2 case in getTlsTpOffset:
> >
> >   case EM_386:
> >   case EM_X86_64:
> >     // Variant 2. The TLS segment is located just before the thread pointer.
> >     return -Out::TlsPhdr->p_memsz;
> >   
> >
> > e.g. If the TLS segment is 4-bytes in size with 8-byte alignment (and assuming p_vaddr % p_align == 0), then the segment will be located at TP-8 on variant 2 targets, not TP-4.
> >
> > I haven't tested this, but something like this would probably work:
> >
> >   return -alignTo(Out::TlsPhdr->p_memsz, Out::TlsPhdr->p_align);
> >   
> >
> > Otherwise, lld and the loader will disagree about the location of TLS symbols.
>
>
> Thanks for pointing this out! I pushed a quick fix. To be more precise, it should be:
>
>   diff --git c/ELF/InputSection.cpp w/ELF/InputSection.cpp
>   index 72a2c298d..2eb9a0452 100644
>   --- c/ELF/InputSection.cpp
>   +++ w/ELF/InputSection.cpp
>   @@ -594,11 +594,13 @@ static int64_t getTlsTpOffset() {
>        // NB: While the ARM/AArch64 ABI formally has a 2-word TCB size, lld
>        // effectively increases the TCB size to 8 words for Android compatibility.
>        // It accomplishes this by increasing the segment's alignment.
>   -    return alignTo(Config->Wordsize * 2, Out::TlsPhdr->p_align);
>   +    return alignTo(Config->Wordsize * 2, Out::TlsPhdr->p_align, Out::TlsPhdr->FirstSec->Addr);
>      case EM_386:
>      case EM_X86_64:
>        // Variant 2. The TLS segment is located just before the thread pointer.
>   -    return -alignTo(Out::TlsPhdr->p_memsz, Out::TlsPhdr->p_align);
>   +    return -(Out::TlsPhdr->p_memsz +
>   +             (-Out::TlsPhdr->p_memsz - Out::TlsPhdr->FirstSec->Addr &
>   +              Out::TlsPhdr->p_align - 1));
>      case EM_PPC64:
>        // The thread pointer points to a fixed offset from the start of the
>        // executable's TLS segment. An offset of 0x7000 allows a signed 16-bit
>
>
> The formulae are complex because they take `p_vaddr%p_align!=0` into account... I didn't do that in the commit because I cannot think of a way to make `p_vaddr%p_align!=0` (after we remove the ARM/AArch64 overalignment hack).


@rprichard @ruiu I missed another two cases for x86-32: gd to le relaxation and `@tpoff`... rLLD361088 <https://reviews.llvm.org/rLLD361088> now.

Anyway, I think with recent llvm/lld changes, it is low overhead to implement the workaround in crtbegin*.o now.

  .section .tdata,"awT", at progbits
  .p2align 8
  .zero 1 # delete if you don't care about ld.bfd (ld.bfd strips empty PT_TLS)
  
  .text
  _start:
    .reloc 0, R_AARCH64_NONE, .tdata


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62059/new/

https://reviews.llvm.org/D62059