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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D62059#2436681 <https://reviews.llvm.org/D62059#2436681>, @jhenderson wrote:

> 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).
>
> I'm just working on some internal testing in relation to this. It's quite possible to trigger this behaviour using a linker script. All you need is for the PT_TLS segment to appear somewhere other than at the front of its PT_LOAD segment, with data before it. In my experiment, I had an 8-byte section first, followed by an 8-byte sized, 8-byte aligned .tdata and 16-byte aligned .tbss. This resulted in a PT_TLS appearing at a `p_vaddr % p_align != 0` position.
>
> I'm posting this because I wasn't sure if a test case has since been added, or whether I should just try posting my example as a test case. @MaskRay, what do you think?

Adding back the code does not break any test, so there is no. If you are going to add a test, probably reuse an existing tls test and check PT_TLS.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62059