[ARM][AArch64] Revert Android Bionic PT_TLS overaligning hack

[MaskRay]

Summary

This reverts D53906.

D53906 increased p_align of PT_TLS on ARM/AArch64 to 32/64 to make the
static TLS layout compatible with Android Bionic's ELF TLS. However,
this may cause glibc ARM/AArch64 programs to crash (see PR41527).

The faulty PT_TLS in the executable satisfies p_vaddr%p_align != 0. The
remainder is normally 0 but may be non-zero with the hack in place. The
problem is that we increase PT_TLS's p_align after OutputSections'
addresses are fixed (assignAddress()). It is possible that
p_vaddr%old_p_align = 0 while p_vaddr%new_p_align != 0.

For the thread local variable defined in the executable, lld computed
TLS offsets (local exec) are different from glibc computed TLS offsets
from another module (initial exec/generic dynamic). Note: PR41527
reported the issue of initial exec but the bug affects generic dynamic
as well.

(glibc is correct in that it compute offsets that satisfy
offset%p_align == p_vaddr%p_align, which is a basic ELF requirement.
This hack appears to work on FreeBSD rtld, musl<=1.1.22, and Bionic, but
that is just because they (and lld) incorrectly compute offsets that
satisfy offset%p_align = 0 instead.)

Android developers are fine to revert this patch, carry this patch in
their tree before figuring out a long-term solution (e.g. a dummy .tdata
with sh_addralign=64 sh_size={0,1} in crtbegin*.o files. The overhead is
now insignificant after D62059).

[MaskRay]

.

[MaskRay]

This is correct now

[MaskRay]

Reword a bit

[rprichard]

I'm wondering if ruiu and/or srhines want to weigh in. I think I'm OK with reverting this change, at least for now, so that we can get the lld+glibc combination working again. It seems like a good idea to fix the https://bugs.llvm.org/show_bug.cgi?id=41527 regression in LLVM 8.0.1, which is coming up soon.

We could temporarily carry a revert of this revert in the Android tree, and I could look into doing TLS reservation using crtbegin*.o.

[MaskRay]

Explain the crash

[peter.smith]

I'm happy to go along with the Android folks decision here.

[MaskRay]

Clarify: it affects generic dynamic as well (which makes it even worse)

[MaskRay]

Rebase after D62059

[MaskRay]

Now that (a) we get .reloc support (D61973 D61992), (b) R_*_NONE on Elf*_Rel targets are compatible with ld.lld -r D62052 (c) p_memsz of PT_TLS is no longer aligned (D62059; Ryan had noticed the difference in https://reviews.llvm.org/D53906#1326683 but it had not been implemented... D62059 was tricky as it broke x86-{32,64} but I think they are good now), the workaround in crtbegin_{dynamic_static}.o should be feasible and it should have very little overhead.

Just waiting for @ruiu and/or @srhines to weigh in :) This revert should immediately unbreak glibc ARM/AArch64 users. I've describe the crash scenario in the description (PR41527 says it is for initial exec, but the bug affects the generic dynamic (most common; when you access an extern __thread in a DSO) model as well):

For a thread local variable defined in the executable, lld computed TLS offset (local exec) is different from glibc computed TLS offset from another module (initial exec/generic dynamic).

Since Ryan's patch affects only executables (if (!Config->Shared && (Config->EMachine == EM_ARM || Config->EMachine == EM_AARCH64)), I assume there is some code in Bionic that fires if the executable doesn't have PT_TLS but one of the shared objects at startup (part of static TLS) has. I think that piece of code can be deleted now.

# crtbegin_{dynamic,static}.o or some other object file that defines _start. I know little about Bionic
.section .tdata,"awT",@progbits
.p2align 6  # 5 for ARM
.zero 1 # delete if you don't care about ld.bfd (ld.bfd strips empty PT_TLS)

.text
.globl _start
_start:
  ...
  .reloc 0, R_AARCH64_NONE, .tdata  # prevent --gc-sections from discarding .tdata

If you don't care about ld.bfd, the zero-sized .tdata is nearly zero overhead :) If the program doesn't use TLS, p_memsz of PT_TLS will be zero: no allocation happens.

[srhines]

The revert is fine with me, although I still would like Ryan to follow up on verifying that the suggested fix for Bionic will indeed work for all cases. I think it is fine for you to merge now to unbreak things for everyone else though. Thanks for working through the problem.

[MaskRay]

Just re-confirmed that Ryan is ok to revert D53906! I'll go ahead.

[ruiu]

I'd vote for re-submitting the original patch with a larger alignment to avoid introducing Android-specific ABI to the output, as I said in a different review thread. Is there any problem to do that?

[MaskRay]

In D62055#1508038, @ruiu wrote:

I'd vote for re-submitting the original patch with a larger alignment to avoid introducing Android-specific ABI to the output, as I said in a different review thread. Is there any problem to do that?

@ruiu To fix the hack isn't impossible, but that would make the code more hacky (D61824 and D61825), and some ARM/AArch users (e.g. musl) are not happy. As I described in https://reviews.llvm.org/D62055#1507426, the relevant support is all in place, the nearly zero-overhead workaround in Bionic crtbegin_{dynamic,static}.o should be straightforward now (it is strictly better than the lld patch in all aspects). I don't know why we would like to re-introduce the hack and cause churn to the 8.0.1 release.

ELF/InputSection.cpp

 // target-specific adjustment to produce a thread-pointer-relative offset.
 static int64_t getTlsTpOffset() {
   switch (Config->EMachine) {
   case EM_ARM:
   case EM_AARCH64:
     // Variant 1. The thread pointer points to a TCB with a fixed 2-word size,
     // followed by a variable amount of alignment padding, followed by the TLS
     // segment.
-    //
-    // 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);
   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);
   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

ELF/Writer.cpp

if (P->p_type == PT_LOAD) {
       P->p_align = std::max<uint64_t>(P->p_align, Config->MaxPageSize);
     } else if (P->p_type == PT_GNU_RELRO) {
       P->p_align = 1;
       // 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, Config->CommonPageSize);
     }
-
-    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, which allocates several slots after the thread pointer (e.g.
-        // TLS_SLOT_STACK_GUARD==5). For simplicity, this overalignment is also
-        // done on other operating systems.
-        P->p_align = std::max<uint64_t>(P->p_align, Config->Wordsize * 8);
-      }
-    }
   }
 }
 
 // A helper struct for checkSectionOverlap.
 namespace {
 struct SectionOffset {
   OutputSection *Sec;
   uint64_t Offset;

test/ELF/aarch64-cortex-a53-843419-tlsrelax.s

  ldr    x1, [x0, #:gottprel_lo12:v]
  adrp   x0, :gottprel:v
  ldr    x1, [x0, #:gottprel_lo12:v]
  ret
 
 // CHECK: _start:
 // CHECK-NEXT:   210ff8:        41 d0 3b d5     mrs     x1, TPIDR_EL0
 // CHECK-NEXT:   210ffc:        00 00 a0 d2     movz    x0, #0, lsl #16
-// CHECK-NEXT:   211000:        01 08 80 f2     movk    x1, #64
+// CHECK-NEXT:   211000:        01 02 80 f2     movk    x1, #16
 // CHECK-NEXT:   211004:        00 00 a0 d2     movz    x0, #0, lsl #16
-// CHECK-NEXT:   211008:        01 08 80 f2     movk    x1, #64
+// CHECK-NEXT:   211008:        01 02 80 f2     movk    x1, #16
 // CHECK-NEXT:   21100c:        c0 03 5f d6     ret
 
  .type  v,@object
  .section       .tbss,"awT",@nobits
  .globl v
 v:
  .word 0

test/ELF/aarch64-tls-gdle.s

 # REQUIRES: aarch64
 # RUN: llvm-mc -filetype=obj -triple=aarch64-unknown-linux %p/Inputs/aarch64-tls-ie.s -o %ttlsie.o
 # RUN: llvm-mc -filetype=obj -triple=aarch64-unknown-linux %s -o %tmain.o
 # RUN: ld.lld %tmain.o %ttlsie.o -o %tout
 # RUN: llvm-objdump -d %tout | FileCheck %s
 # RUN: llvm-readobj -S -r %tout | FileCheck -check-prefix=RELOC %s
 
 #Local-Dynamic to Local-Exec relax creates no
 #RELOC:      Relocations [
 #RELOC-NEXT: ]
 
-# TCB size = 64 and foo is first element from TLS register.
+# TCB size = 0x16 and foo is first element from TLS register.
 # CHECK: Disassembly of section .text:
 # CHECK-EMPTY:
 # CHECK: _start:
 # CHECK:  210000: 00 00 a0 d2 movz  x0, #0, lsl #16
-# CHECK:  210004: 00 08 80 f2   movk  x0, #64
+# CHECK:  210004: 00 02 80 f2   movk  x0, #16
 # CHECK:  210008: 1f 20 03 d5   nop
 # CHECK:  21000c: 1f 20 03 d5   nop
 
 .globl _start
 _start:
  adrp    x0, :tlsdesc:foo
  ldr     x1, [x0, :tlsdesc_lo12:foo]
  add     x0, x0, :tlsdesc_lo12:foo
  .tlsdesccall foo
  blr     x1

test/ELF/aarch64-tls-iele.s

 # REQUIRES: aarch64
 # RUN: llvm-mc -filetype=obj -triple=aarch64-unknown-linux %p/Inputs/aarch64-tls-ie.s -o %ttlsie.o
 # RUN: llvm-mc -filetype=obj -triple=aarch64-unknown-linux %s -o %tmain.o
 # RUN: ld.lld %tmain.o %ttlsie.o -o %tout
 # RUN: llvm-objdump -d %tout | FileCheck %s
 # RUN: llvm-readobj -S -r %tout | FileCheck -check-prefix=RELOC %s
 
 # Initial-Exec to Local-Exec relax creates no dynamic relocations.
 # RELOC:      Relocations [
 # RELOC-NEXT: ]
 
-# TCB size = 64 and foo is first element from TLS register.
+# TCB size = 0x16 and foo is first element from TLS register.
 # CHECK: Disassembly of section .text:
 # CHECK-EMPTY:
 # CHECK: _start:
 # CHECK-NEXT: 210000:  00 00 a0 d2   movz   x0, #0, lsl #16
-# CHECK-NEXT: 210004:  80 08 80 f2   movk   x0, #68
+# CHECK-NEXT: 210004:  80 02 80 f2   movk   x0, #20
 # CHECK-NEXT: 210008:  00 00 a0 d2   movz   x0, #0, lsl #16
-# CHECK-NEXT: 21000c:  00 08 80 f2   movk   x0, #64
+# CHECK-NEXT: 21000c:  00 02 80 f2   movk   x0, #16
 
 .section .tdata
 .align 2
 .type foo_local, %object
 .size foo_local, 4
 foo_local:
 .word 5
 .text
 
 .globl _start
 _start:
  adrp    x0, :gottprel:foo
  ldr     x0, [x0, :gottprel_lo12:foo]
  adrp    x0, :gottprel:foo_local
  ldr     x0, [x0, :gottprel_lo12:foo_local]

test/ELF/aarch64-tls-le.s

 _start:
  mrs x0, TPIDR_EL0
  add x0, x0, :tprel_hi12:v1
  add x0, x0, :tprel_lo12_nc:v1
  mrs x0, TPIDR_EL0
  add x0, x0, :tprel_hi12:v2
  add x0, x0, :tprel_lo12_nc:v2
 
-# TCB size = 64 and foo is first element from TLS register.
+# TCB size = 0x16 and foo is first element from TLS register.
 #CHECK: Disassembly of section .text:
 #CHECK: _start:
 #CHECK:  210000: 40 d0 3b d5     mrs     x0, TPIDR_EL0
 #CHECK:  210004: 00 00 40 91     add     x0, x0, #0, lsl #12
-#CHECK:  210008: 00 00 01 91     add     x0, x0, #64
+#CHECK:  210008: 00 40 00 91     add     x0, x0, #16
 #CHECK:  21000c: 40 d0 3b d5     mrs     x0, TPIDR_EL0
 #CHECK:  210010: 00 fc 7f 91     add     x0, x0, #4095, lsl #12
 #CHECK:  210014: 00 e0 3f 91     add     x0, x0, #4088
 
 .section        .tbss,"awT",@nobits
 
 .type   v1,@object
 .globl  v1
 .p2align 2
 v1:
 .word  0
 .size  v1, 4
 
-# The current offset from the thread pointer is 68. Raise it to just below the
+# The current offset from the thread pointer is 20. Raise it to just below the
 # 24-bit limit.
-.space (0xfffff8 - 68)
+.space (0xfffff8 - 20)
 
 .type   v2,@object
 .globl  v2
 .p2align 2
 v2:
 .word  0
 .size  v2, 4

test/ELF/aarch64-tlsld-ldst.s

_start:  mrs x8, TPIDR_EL0
         add x8, x8, :tprel_hi12:var3
         ldrh w0, [x8, :tprel_lo12_nc:var3]
 
         add x8, x8, :tprel_hi12:var4
         ldrb w0, [x8, :tprel_lo12_nc:var4]
 
 // CHECK: _start:
 // CHECK-NEXT:    210000:       48 d0 3b d5     mrs     x8, TPIDR_EL0
-// 0x0 + c40 = 0xc40       = tcb (64-bytes) + var0
-// CHECK-NEXT:    210004:       08 01 40 91     add x8, x8, #0, lsl #12
-// CHECK-NEXT:    210008:       14 11 c3 3d     ldr q20, [x8, #3136]
-// 0x1000 + 0x850 = 0x1850 = tcb + var1
-// CHECK-NEXT:    21000c:       08 05 40 91     add x8, x8, #1, lsl #12
-// CHECK-NEXT:    210010:       00 29 44 f9     ldr x0, [x8, #2128]
-// 0x2000 + 0x458 = 0x2458 = tcb + var2
-// CHECK-NEXT:    210014:       08 09 40 91     add x8, x8, #2, lsl #12
-// CHECK-NEXT:    210018:       00 59 44 b9     ldr w0, [x8, #1112]
-// 0x3000 + 0x5c  = 0x305c = tcb + var3
-// CHECK-NEXT:    21001c:       08 0d 40 91     add x8, x8, #3, lsl #12
-// CHECK-NEXT:    210020:       00 b9 40 79     ldrh  w0, [x8, #92]
-// 0x3000 + 0xc5e = 0x3c5e = tcb + var4
-// CHECK-NEXT:    210024:       08 0d 40 91     add x8, x8, #3, lsl #12
-// CHECK-NEXT:    210028:       00 79 71 39     ldrb  w0, [x8, #3166]
+// 0x0 + c10 = 0xc10       = tcb (16-bytes) + var0
+// CHECK-NEXT:    210004:       08 01 40 91     add     x8, x8, #0, lsl #12
+// CHECK-NEXT:    210008:       14 05 c3 3d     ldr     q20, [x8, #3088]
+// 0x1000 + 0x820 = 0x1820 = tcb + var1
+// CHECK-NEXT:    21000c:       08 05 40 91     add     x8, x8, #1, lsl #12
+// CHECK-NEXT:    210010:       00 11 44 f9     ldr     x0, [x8, #2080]
+// 0x2000 + 0x428 = 0x2428 = tcb + var2
+// CHECK-NEXT:    210014:       08 09 40 91     add     x8, x8, #2, lsl #12
+// CHECK-NEXT:    210018:       00 29 44 b9     ldr     w0, [x8, #1064]
+// 0x3000 + 0x2c  = 0x302c = tcb + var3
+// CHECK-NEXT:    21001c:       08 0d 40 91     add     x8, x8, #3, lsl #12
+// CHECK-NEXT:    210020:       00 59 40 79     ldrh    w0, [x8, #44]
+// 0x3000 + 0xc2e = 0x32ce = tcb + var4
+// CHECK-NEXT:    210024:       08 0d 40 91     add     x8, x8, #3, lsl #12
+// CHECK-NEXT:    210028:       00 b9 70 39     ldrb    w0, [x8, #3118]
 
-// CHECK-SYMS:      0000000000000c00    16 TLS     GLOBAL DEFAULT    2 var0
-// CHECK-SYMS-NEXT: 0000000000001810     8 TLS     GLOBAL DEFAULT    2 var1
-// CHECK-SYMS-NEXT: 0000000000002418     4 TLS     GLOBAL DEFAULT    2 var2
-// CHECK-SYMS-NEXT: 000000000000301c     2 TLS     GLOBAL DEFAULT    2 var3
-// CHECK-SYMS-NEXT: 0000000000003c1e     1 TLS     GLOBAL DEFAULT    2 var4
+// CHECK-SYMS:      0000000000000c00     0 TLS     GLOBAL DEFAULT    2 var0
+// CHECK-SYMS-NEXT: 0000000000001810     4 TLS     GLOBAL DEFAULT    2 var1
+// CHECK-SYMS-NEXT: 0000000000002418     2 TLS     GLOBAL DEFAULT    2 var2
+// CHECK-SYMS-NEXT: 000000000000301c     1 TLS     GLOBAL DEFAULT    2 var3
+// CHECK-SYMS-NEXT: 0000000000003c1e     0 TLS     GLOBAL DEFAULT    2 var4
 
         .globl var0
         .globl var1
         .globl var2
         .globl var3
         .globl var4
         .type var0,@object
         .type var1,@object
         .type var2,@object
         .type var3,@object
 
 .section .tbss,"awT",@nobits
-        .balign 64
+        .balign 16
         .space 1024 * 3
 var0:
         .quad 0
         .quad 0
-        .size var0, 16
+        .size var1, 16
         .space 1024 * 3
 var1:
         .quad 0
         .size var1, 8
         .space 1024 * 3
 var2:
         .word 0
-        .size var2, 4
+        .size var1, 4
 
         .space 1024 * 3
 var3:
         .hword 0
-        .size var3, 2
+        .size var2, 2
         .space 1024 * 3
 var4:
         .byte 0
-        .size var4, 1
+        .size var3, 1
         .space 1024 * 3

test/ELF/arm-tls-le32.s

 // SEC:      Size: 8
 
 // SEC: Dynamic Relocations {
 // SEC-NEXT: }
 
 // CHECK: Disassembly of section .text:
 // CHECK-EMPTY:
 // CHECK-NEXT: _start:
-// offset of x from Thread pointer = (TcbSize + 0x0 = 0x20)
-// CHECK-NEXT:   11000:         20 00 00 00
-// offset of z from Thread pointer = (TcbSize + 0x8 = 0x28)
-// CHECK-NEXT:   11004:         28 00 00 00
-// offset of y from Thread pointer = (TcbSize + 0x4 = 0x24)
-// CHECK-NEXT:   11008:         24 00 00 00
+// offset of x from Thread pointer = (TcbSize + 0x0 = 0x8)
+// CHECK-NEXT:   11000:         08 00 00 00
+// offset of z from Thread pointer = (TcbSize + 0x8 = 0x10)
+// CHECK-NEXT:   11004:         10 00 00 00
+// offset of y from Thread pointer = (TcbSize + 0x4 = 0xc)
+// CHECK-NEXT:   11008:         0c 00 00 00

test/ELF/arm-tls-norelax-ie-le.s

  .globl x2
  .section       .tbss,"awT",%nobits
  .type  x1, %object
 x2:
  .space 4
  .type x2, %object
 
 // CHECK: Contents of section .got:
-// x1 at offset 0x20 from TP, x2 at offset 0x24 from TP. Offsets include TCB size of 0x20
-// CHECK-NEXT: 12064 20000000 24000000
+// x1 at offset 8 from TP, x2 at offset 0xc from TP. Offsets include TCB size of 8
+// CHECK-NEXT: 12064 08000000 0c000000