[PATCH] D61824: [ARM][AArch64] Overalign SHF_TLS instead of PT_TLS (Android Bionic hack) to make glibc happy

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I have a question based on the updated description of D61825 <https://reviews.llvm.org/D61825> (makes more sense in the context of this patch)

  - 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.

I'm not sure I understand the comment about ALIGNUP, did you mean the linker script command ALIGN? I guess by perceivable you mean it has more impact on the file and following sections?

I noticed while trying to construct a test case on Arm that does not need a shared library, that LLD will page align the first SHF_TLS .tdata section as (with the default layout and most linker scripts) it is the first Section in the Read/Write PT_LOAD segment. It does not do this for .tbss as it returns false for needsPtLoad(). I think that this means that increasing the alignment for .tdata to 32/64 won't make any difference given that it will be aligned to a 4k/64k page anyway. The problem case can only currently happen when there is only .tbss. Given that no extra padding is needed for .tdata I think the impact of extra padding for .tbss if we only output it if there is no .tdata will be minimal. I think the extra padding would also be a maximum of 48 bytes on AArch64 and 16 on Arm, as the section would follow the .plt and that is 16-byte aligned with a size that is a multiple of 16-bytes. In summary, while an extra 48-bytes of padding to the TLS if there is only .tbss is unfortunate, I don't think it would be overfully painful to other platforms.

I agree that a change in Bionic that would work for all linkers would be ideal, but if there were a change needed in LLD, I prefer D61824 <https://reviews.llvm.org/D61824> to D61825 <https://reviews.llvm.org/D61825>.

My Arm test case just in case it is useful to anyone. Arm does not do TLS relaxation so it may not fail on AArch64 due to relaxation of IE to LE:

tls.c

  #include <stdio.h>
  __thread int val __attribute__((tls_model("initial-exec")));
  
  extern int func();
  
  int main(void) {
      printf("From IE local %p\n", &val);
      func();
      return 0;
  }

tls2.c

  #include<stdio.h>
  extern __thread int val __attribute__((tls_model("initial-exec")));
  
  int func(void)  {
      printf("From IE non local %p\n", &val);
      return val;
  }

clang -fpie -pie --target=armv7a-linux-gnueabihf tls.c tls2.c -O2 -o tls.axf -fuse-ld=lld
QEMU gives me different addresses for the static and dynamic locations of the variable.
>From IE local 0xf6657940
>From IE non local 0xf6657930


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D61824