[PATCH] D53906: [ARM][AArch64] Increase TLS alignment to reserve space for Android's TCB

[Ryan Prichard via Phabricator via llvm-commits]

rprichard marked an inline comment as done.
rprichard added inline comments.


================
Comment at: ELF/Writer.cpp:2183
+    if (P->p_type == PT_TLS && P->p_memsz) {
+      // Reserve extra space (8 words) between the thread pointer and an
+      // executable's TLS segment by overaligning the segment. This reservation
----------------
peter.smith wrote:
> May I suggest moving the added code after the // The TLS pointer goes after PT_TLS for variant 2 targets comment? I think the comment here would be easier to understand if it came sequentially afterwards. It may also be worth moving the comment inside the if statement, or clarifying that it is Arm/AArch64 only at the moment.  
The code itself can't be reordered because the first statement modifies P->p_align and the second one reads P->p_align. Maybe something like this?

```
    if (P->p_type == PT_TLS && P->p_memsz) {
      if (!Config->Shared &&
          (Config->EMachine == EM_ARM || Config->EMachine == EM_AARCH64)) {
        // On ARM/AArch64, reserve extra space (8 words) between the thread
        // pointer and an executable's TLS segment by overaligning the segment.
        // This reservation is needed for backwards compatibility with Android's
        // TCB, but for simplicity it is also done on other operating systems.
        P->p_align = std::max<uint64_t>(P->p_align, Config->Wordsize * 8);
      }

      // The TLS pointer goes after PT_TLS for variant 2 targets. At least glibc
      // will align it, so round up the size to make sure the offsets are
      // correct.
      P->p_memsz = alignTo(P->p_memsz, P->p_align);
    }
```



Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D53906