[PATCH] D53906: [ELF] Allow configuring the TLS layout for an Android executable

Wed Oct 31 19:21:36 PDT 2018

rprichard added a subscriber: eugenis.
rprichard added a comment.

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

In https://reviews.llvm.org/D53906#1282053, @peter.smith wrote:

> Just trying to understand the implications here. Unfortunately I find I can't keep the details in my memory for more than the time I have to work on it. Can you correct me where I'm wrong?
>
> My understanding of the current AArch64 TLS is:
>
> - It is variant 1
> - TCB size is 2 64-bit words with the first word being a pointer to the dynamic thread vector, the second word being reserved for the implementation.
> - The TP points directly to the start of TCB.
> - Only positive offsets are needed to access the data from the thread pointer. From memory the load/store instructions with scaled offsets can only use unsigned offsets.
> - Android needs a larger TCB than 16 due to various slots such as the stack protector being used (https://android.googlesource.com/platform/bionic/+/master/libc/private/bionic_tls.h)
> - Android would need to mark binaries using ELF TLS regardless of whatever implementation of TLS Android chooses. Hence this patch.

That all sounds about right.

Ideally, Android would have used negative TP offsets on ARM from the start. e.g.: like Fuschia:

- https://github.com/fuchsia-mirror/zircon/blob/0dbac4d37ca09eaebcb5d1337775b9dc78a4ac06/system/public/zircon/tls.h
- https://github.com/llvm-mirror/llvm/blob/release_70/lib/Target/AArch64/AArch64ISelLowering.cpp#L11431

I believe most of the Bionic slots could be moved to negative slots (or equivalently, plain fields of `pthread_internal_t`) without breaking anything.

The stack protector cookie looks like the biggest exception. Even if we allocated a new slot for it today, new AArch64 binaries would continue to use the old slot for compatibility with old Android releases. Alternatively, we could revert https://reviews.llvm.org/D18632, but that would make binaries larger and possibly slower. If we did revert it, we'd only have to deal with existing AArch64 binaries, which is still a big problem.

The Android NDK's build systems (ndk-build and cmake) compile with `-fstack-protector-strong` by default, and since NDK r14, Clang has used `TLS_SLOT_STACK_GUARD` for AArch64. The NDK has defaulted to Clang (versus GCC) since NDK r13.

Moving the TSAN slot might also be a problem. I suspect it would quietly break existing sanitizer solibs on new Android releases.

It's common on Android to have application solibs loaded into a Zygote process (`/system/bin/app_process{32,64}`), and I think we can also see old vendor solibs running in the same process with new platform code (as of Project Treble).

> One potential solution for Android is to increase the TCB size to 16 words for Android. As I understand it this would only affect the Local Exec as that is where the relocations have to take into account the size of the TCB. I also think that this would limit the changes to the static linker and loader, the compiler and assembler wouldn't be affected.

Right -- increasing the TCB size affects the static linker, dynamic linker, and libc, but it doesn't affect the compiler or assembler.

> The alternative solutions are to disable Local Exec, or implement variant 2 TLS in Arm and AArch64 which would likely require compiler changes and probably wouldn't work as well on AArch64 as some of the immediate addressing modes aren't available on the load.
> 
> Personally I'd prefer not to see an implementation of variant 2 for AArch64, it isn't too difficult to test AArch64 compiler and linker changes using a native linux machine or the qemu user mode emulator. I'm a bit concerned that it would be difficult for non Android developers to test Android specific changes? I understand the concern that 16 slots might not be enough. Perhaps reserve the last entry for a pointer to an extension vector? Or do something similar to what PPC have done and use negative offsets from TP for reserved values.

I agree that it's good to minimize the amount of Android-specific logic in LLVM. A larger-TCB is a better fix than variant 2 in that regard, especially if variant 2 required a new static relocation.

I don't think I'm worried about running out of slots -- as you said, Bionic should be to able to use negative offsets from the TP for new slots.

Allocating an especially-large TCB (e.g. 4KB) would be one way to keep Bionic compatible with golang's "g" register allocation, because Bionic could place the `pthread_internal_t::key_data` field immediately after the Bionic slots: https://android.googlesource.com/platform/bionic/+/6f3a56bb18628243b6dbe470222e56bb56ed10ae/libc/bionic/pthread_internal.h#124

> As an aside it would help to have a similar page to Apple's https://developer.apple.com/library/archive/documentation/Xcode/Conceptual/iPhoneOSABIReference/Articles/ARM64FunctionCallingConventions.html describing differences from the ABI.

Yeah, that might be helpful. I think https://developer.android.com/ndk/guides/abis is the closest thing we currently have.

> On the patch itself, I've got a suggestion on an alternative mechanism to mark the ELF file that may be more extensible for the future.
> 
> There are a few architectural extensions coming for AArch64 such as pointer authentication (8.3) and branch target information BTI (8.5) both of which need static linker (PLT entries) and dynamic loader support (enable the feature for a range of addresses). These will need ELF files marked so that the linker and loader know what to do. The first implementation from Arm's GNU team is going to use .note.gnu.property section to describe these properties. It may be useful to examine these to see if you can use these to mark binaries rather than introduce an OS specific program header?
> Links:
>  https://github.com/hjl-tools/linux-abi/wiki/Linux-Extensions-to-gABI (see https://github.com/hjl-tools/linux-abi/wiki/linux-abi-draft.pdf in particular)
> 
> I'm not sure if LLD supports these sections yet http://lists.llvm.org/pipermail/llvm-dev/2018-March/121951.html . I've got the 8.3 and 8.5 extensions in LLD on my list of things to do once they are posted on the binutils mailing lists so I'd need to work on support for them.

Thanks for the links. I'll look into them more later. A note would work in general. To use `.note.gnu.property`, it looks like we'd need to allocate a `GNU_PROPERTY_TLS_TPOFF` property value. These are the currently-defined property types:

| GNU_PROPERTY_STACK_SIZE           | 1          |
| GNU_PROPERTY_NO_COPY_ON_PROTECTED | 2          |
| GNU_PROPERTY_LOPROC               | 0xc0000000 |
| GNU_PROPERTY_HIPROC               | 0xdfffffff |
| GNU_PROPERTY_LOUSER               | 0xe0000000 |
| GNU_PROPERTY_HIUSER               | 0xffffffff |
|

Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D53906