Page MenuHomePhabricator

[ARM][AArch64] Put Android Bionic PT_TLS overaligning hack under --android-tls
AbandonedPublic

Authored by MaskRay on May 11 2019, 9:24 AM.

Details

Summary

D53906 increased p_align of PT_TLS on ARM/AArch64 to 32/64 to make the
TLS layout compatible with Android Bionic. However, this can make glibc
ARM/AArch64 programs using {initial,local}-exec TLS models crash (see
PR41527).

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

When this happens, lld and different ld.so implementations have
different opinions on the offset of the first module. glibc elf/dl-tls.c
computes an offset that satisfies: offset%p_align == p_vaddr%p_align.
This is correct as a basic ELF requirement.

lld, musl (1.1.20 ~ latest 1.1.22), FreeBSD rtld, and Bionic place the
main TLS block at a wrong offset: `alignUp(2*sizeof(void*),
main_tls_align)`. This choice doesn't take p_vaddr%p_align into account.

An approach to make both Bionic and glibc happy is to overalign SHF_TLS
instead of PT_TLS. It has several disadvantages:

  • Moving the hack to the OutputSection layer makes the hack more perceivable as it has interaction with the linker script ALIGNUP command.
  • It may waste up to 8*WordSize * 2 bytes of each thread (when both .tbss and .tdata exist and are overaligned). With more code, we can apply the overalignment to the first TLS output section (either .tbss or .tdata) and decrease the wastage to 8*WordSize, but that is still unsatisfactory on other platforms.

This patch keeps the hack under an option. This option may be deleted
when a better fix arises, e.g. a solution that works across
ld.bfd/gold/lld is to add a dummy .tdata (1-sized 64-aligned if Bionic
aligns the end of the TLS block; 48-sized 1-aligned if Bionic aligns the
start of the TLS block) in Android's crtbegin_{dynamic,static}.o . A
R_{ARM,AARCH64}_NONE relocation reference from _start is required to
make the .tdata survive --gc-sections.

Currently, however, Bionic has an alignment check
__bionic_check_tls_alignment that collides with this fix and it may be
too late due to deployment issues. Ideally the vaddr%p_align != 0 case
should also be fixed.

Event Timeline

MaskRay created this revision.May 11 2019, 9:24 AM

If we always align first PT_TLS section on 64 bytes wouldn't this fix the problem?

grimar added a subscriber: grimar.May 13 2019, 5:39 AM

If we always align first PT_TLS section on 64 bytes wouldn't this fix the problem?

The alternative solution D61824 does that.

MaskRay updated this revision to Diff 199556.May 14 2019, 10:25 PM
MaskRay edited the summary of this revision. (Show Details)
MaskRay removed subscribers: grimar, evgeny777.

Update description

MaskRay updated this revision to Diff 199557.May 14 2019, 10:44 PM
MaskRay edited the summary of this revision. (Show Details)
MaskRay removed a subscriber: krytarowski.

Update description

MaskRay added a comment.EditedMay 14 2019, 11:28 PM

@rprichard @srhines I know the workaround is important for Android ELF TLS, but according to our analysis this is bad to other platforms... The 8.0.1 release is coming, I hope we can fix this issue before that... I just created a clang-side change D61931.

See my updated description for a better long-term solution (".tdata (1-sized 64-aligned if Bionic aligns the end of the TLS block; 48-sized 1-aligned if Bionic aligns the start of the TLS block")

MaskRay abandoned this revision.May 17 2019, 3:23 AM

Abandon in favor of a revert: D62055. Ryan told me reverting is preferable.