I tested this patch, and it appears to fix the overflow problem.

The idea seems good to me.

Aside: This change is also tracked in Android's Gerrit review tool: https://android-review.googlesource.com/c/platform/external/libunwind_llvm/+/1142642

(I also tried running the libunwind tests but didn't get them to run.)

FWIW, Android's official toolchain currently only uses LLVM's libunwind for arm32, so this patch doesn't affect anything yet in the Android platform or NDK. I tested it with NDK r20 by:

• building libunwind.a with -DLIBUNWIND_TARGET_TRIPLE=i686-linux-android16, then
• building a PIE executable, linking libunwind.a in front of the libgcc.a unwinder that NDK r20 normally uses.

FWIW, I tested the "extest" benchmark from https://github.com/android/ndk/issues/1062 on arm32. Using dl_unwind_find_exidx, the benchmark throws 10,000 exceptions in about 860ms. Using dl_iterate_phdr, the run-time varies, and I saw run-times from 3000-6000ms. (The unwinder has an optimization where it compares dlpi_addr to the target PC, and that makes EH overhead depend on the layout of shared libraries in memory.)

Exclude an unrelated change from the base revision.

Its aarch64 port is correct (https://sourceware.org/bugzilla/show_bug.cgi?id=24606).

Looks good to me.

In D64930#1606359, @peter.smith wrote:

I had a quick look at the PR to see if I could make it fail (with D64930 applied) with glibc without hacking the binary with the trim-pt-tls program with this change. I wasn't able to do so although that doesn't mean it is impossible. Having said that I agree that it would be safer to be conservative in our demands on the capabilities of dynamic loaders.

I'm not sure about breaking the (p_vaddr % p_align == 0) invariant. ld.bfd, gold, and (for the most part) lld currently provide this invariant for TLS segments in output files.

Android's ELF TLS requires lld, but the NDK doesn't default to lld yet. (I believe the NDK uses bfd on arm64 and gold everywhere else.) When the NDK Clang driver targets 29/Q+, the NDK could generate executables or shared objects using ELF TLS that don't load on 29/Q.

In D61824#1506772, @dalias wrote:

The waste is not just 48 bytes in the ELF image on disk, but up to 63 bytes in each thread. Depending on how that falls, the waste is likely actually either 0 bytes or 4096 bytes per thread, since the memory is allocated with page granularity.

Forcing the alignment will also force musl to *always* mmap space (on page granularity, so A LOT of waste) for the main thread's TLS at startup, rather than using the built-in bss space intended to be used when the application's TLS needs are small, since the built-in space does not have excess alignment. This imposes a syscall as well, and a new failure mode for static-linked binaries that may have been intended not to be able to fail after exec.

Please, drop this ridiculous and costly hack for Bionic or put it under the --android-tls option or whatever.

In D61824#1499997, @nsz wrote:

i think glibc is right (comes up with a tlsoffset%p_align == p_vaddr%p_align for the module) and musl and other implementations are wrong (they may not get the tlsoffset right in this case: it seems musl always computes tlsoffset%p_align==0 on tls variant 1 targets).

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:

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.

FWIW: I noticed that the binutils assembler's .reloc directive can reference an arbitrary symbol whereas with this patch, a .reloc directive referencing a label turns into a section reference. e.g.:

The commit message only mentions AArch64, but the commit appears to also add support for R_ARM_NONE. I tried using R_ARM_NONE, though, and was unsuccessful:

In D61824#1502750, @nsz wrote:

In D61824#1502690, @rprichard wrote:

I think that basically works. It would add a TLS segment to every Android executable, even those that aren't using ELF TLS:

• If the placeholder symbol has size 1 and 64-byte alignment, then every Android executable (targeting Q(29) and up) would have a PT_TLS segment of 64-byte size and alignment with gold or lld.
• If the placeholder symbol in .tdata has size 0 instead, then ld.bfd omits the .tdata section from the output if there aren't any other non-empty .tdata sections. This is a problem if there are other TLS sections (.tbss). Android is switching to lld, though, so maybe it's OK. (The NDK still defaults to bfd and gold.)
• If the placeholder symbol has a size of 48 bytes and 1-byte alignment, then libc.so can't use alignment to detect whether space was reserved. I can think of a couple ways for space to not be reserved:
  • Using an NDK that's too old
  • Using a too-old NDK API level, assuming this placeholder symbol is omitted when targeting old versions of Bionic (because Q(29) is currently rejecting an underaligned TLS segment...)

i think the placeholder tls symbol size and alignment should
be exactly such that it covers the bionic internal tls slots.

In D61824#1500063, @MaskRay wrote:

@rprichard Rich Felker briefly mentioned this can be worked around in crt1.o. My understanding is that:

1. create a dummy .tdata ((a) with placeholder TLS variables of 8*sizeof(void*) bytes (b) sh_addralign=8*sizeof(void*)) in crt1.o (and its variants)
2. create a R_{ARM,AARCH64}_NONE from .text offset 0 to the .tdata (this prevents --gc-sections stripping)
3. no hack is needed in ld.bfd/gold/lld. crt1.o is linked into executables. The linker applies its usual Rules for Linking Unrecognized Sections and concatenates .tdata and .tbss. The .tdata from crt1.o will be the first input section in the TLS segment. Don't use the dummy part of the TLS block (they may collide with Bionic's reserved TLS slots)
   
   Does this approach work?

This change isn't needed anymore -- Bionic instead uses the standard TLS layout on x86-{32,64} and arm{32,64}, but with a minimum alignment of 8 words on arm{32,64}.

Update comment again.

Remove the blank line.

Add an explanation of how overaligning the executable TLS segment reserves the extra space Android needs for its TCB slots.

Move comment into if-statement and clarify that it's only for ARM and AArch64.

I think this CL is ready for review now.

Thanks for looking at the buildbot failure. Let me know if I need to revert my two commits.

I switched HWAddressSanitizer::getHwasanThreadSlotPtr over:
https://reviews.llvm.org/D55592

Looks good to me.

For context:

• https://reviews.llvm.org/D53906
• https://android-review.googlesource.com/c/platform/bionic/+/847554
• https://android-review.googlesource.com/c/platform/bionic/+/825741/3#message-cce1a13518d9c314080be3decb05d784aa13a0cc

I implemented the proposed fix for ARM/AArch64 (i.e. set a minimum alignment of 8 words on an executable's TLS segment). I think this will work for Android -- i.e. we'll leave the x86 layout alone. Even if we *did* decide to change the x86 TLS layout later, it'd probably make sense to use this approach on ARM.

FWIW: I've published a draft of a document summarizing ELF TLS with emphasis on issues affecting Bionic. It's at https://android.googlesource.com/platform/bionic/+/master/docs/elf-tls.md. These sections are relevant to this LLVM patch:

• https://android.googlesource.com/platform/bionic/+/master/docs/elf-tls.md#Thread_Specific-Memory-Layout
• https://android.googlesource.com/platform/bionic/+/master/docs/elf-tls.md#Bionic-Memory-Layout-Conflicts-with-Common-TLS-Layout

On non-Android Linux, it looks like hwasan and msan instrumentation use an IE-model TLS variable (via the GOT), which is more of a load-time constant than a compile-time one.

I'm wondering whether moving the TLS_SLOT_TSAN slot is feasible. e.g. This sanitizer code would stop working, but it could presumably be fixed in a newer sanitizer library:

// The Android Bionic team has allocated a TLS slot for TSan starting with N,
// given that Android currently doesn't support ELF TLS. It is used to store
// Sanitizers thread specific data.
static const int TLS_SLOT_TSAN = 8;

In D53906#1284881, @ruiu wrote:

What I'm currently seeking is a simple way to create executables/DSOs that are compatible on non-Android and Android, without introducing any new command line flag or a target-dependent logic. I believe it is OK to waste a few words on non-Android if we can achieve that goal. Another idea that is in line with that is to always set a >8 word alignment (you suggested 16, but I don't know why it has to be 16) to a TLS segment if ARM or AArch64. Maybe that could produce a binary that is compatible both Android and non-Android?

In D53906#1284788, @ruiu wrote:

Setting a large alignment to the TLS segment to make room from TP and the TLS segment is an interesting idea. It is tempting, but at the same time it feels a bit too subtle and perhaps fragile. Looks like it is a very indirect way to do what we actually want to do.

If I understand correctly, what we want to do is boiled down to this:

 // A TLS symbol's virtual address is relative to the TLS segment. Add a
 // 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.
-    return alignTo(Config->Wordsize * 2, Out::TlsPhdr->p_align);
+    if (Android)
+      return alignTo(Config->Wordsize * 6, Out::TlsPhdr->p_align);
+    else
+      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 -Out::TlsPhdr->p_memsz;

Is this understanding correct? If so, one radical but simple approach would be to always skip the first 6 words from TP on ARM and AArch64 whether it is Android or not. Obviously that wastes 4 words per a thread, but since thread is not a lightweight data structure, it might be negligible.

In D53906#1283665, @peter.smith wrote:

Increasing the alignment of the TLS segment is an interesting idea as it allows the possibility of a larger TCB and in theory could also be used by the loader to detect compatible ELF files (reject those without the alignment).

One possible way to achieve this without linker changes would be for the Android startup equivalent crt0.s to have a zero sized TLS .data section with 16 byte alignment. This would have the problem that all executables and shared libraries that didn't use TLS would get a 0 sized TLS segment.

@peter.smith Thanks for taking a look. That's a good summary of the problem.

As a near-equivalent to this patch, if we had a good way of increasing an executable's TLS segment alignment to 16 words, then libc could allocate the Bionic slots in the alignment padding between the 2-word TCB and the TLS segment. In that case, Android executables would still be following the official TLS ABI, but slightly subsetted.

Update according to the latest revision of D53905 (export getTlsTpOffset).

Make getTlsTpOffset a file-scope static function.

Rebase on updated parent revision (TLS layout consolidated in getTlsTpOffset).

Replace the TargetInfo TLS layout fields with a switch on Config->EMachine.

Yeah, I also need to modify libc and the loader. The problem is that, with the TLS local-exec (LE) access model used in executables, the static linker writes a thread-pointer-relative offset into an executable's text section, which the loader can't relocate. With the next-most efficient model, initial-exec (IE), the linker stores TLS offsets in the GOT, which the loader does relocate. (Disabling LE in the compiler+linker in favor of IE is another way of resolving this ABI conflict, but IIRC, it was also problematic. e.g. We can't do LD->LE relaxation anymore.)

Android/Bionic currently doesn't have ELF TLS support, so no existing binaries are affected. I'm working on adding support for ELF TLS to Android, but Android's existing use of the thread pointer conflicts with the ordinary ELF TLS ABI.

I'm currently planning to use a 16-word TCB layout on all architectures for Android, but I'm not completely committed to that yet. This patch can accommodate a larger TCB. I'm also considering using a variant 2 layout instead, which would reuse some of this patch, but would require additional work to accommodate negative offsets on arm64.

This change would make it possible to model RISC-V TLS without the (Config->EMachine == EM_RISCV) special case in https://reviews.llvm.org/D39324 (InputSection.cpp). RISC-V expects a fixed offset of 0.

I didn't see a good way to write a regression test. I tested it by hand with this script:

Adding zero padding to the end of the SHT_ANDROID_REL[A] section shouldn't be a problem. It looks like the original Android relocation_packer tool left up to a page of zero bytes at the end of the packed section, and the decoders in Bionic and LLVM would ignore the trailing bytes. relocation_packer doesn't loop stabilizing SLEB sizes, but I think it didn't need to. When it shrunk a .rel[a].dyn section, it apparently only shrunk the section by a multiple of the page size. It left the virtual addresses of the sections following .rel[a].dyn unchanged, and I don't think it adjusted the offset/addend values of the relocations.