Page MenuHomePhabricator

Disable emulated-tls and use LLD for compiler-rt+tests on Android
Needs ReviewPublic

Authored by oontvoo on Fri, Oct 16, 11:08 PM.

Details

Summary

This is necessary for enabling LSAN on Android (D89251) because:

  • LSAN will have false negatives if run with emulated-tls.
  • Bionic ELF-TLS is not compatible with Gold (hence the need for LLD)

Diff Detail

Event Timeline

oontvoo created this revision.Fri, Oct 16, 11:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Oct 16, 11:08 PM
Herald added subscribers: Restricted Project, mgorny, dberris. · View Herald Transcript
oontvoo requested review of this revision.Fri, Oct 16, 11:08 PM
oontvoo updated this revision to Diff 298810.Sat, Oct 17, 12:12 AM

For arm(64), also need to overalign the TLS segment if tests run with ELF-TLS

dmajor added a subscriber: dmajor.Sat, Oct 17, 4:42 AM

Why we can't just switch to elf TLS?

Why we can't just switch to elf TLS?

Maybe I misunderstood the question, but this is exactly what this patch is trying to do.

(We can't switch to ELF TLS unless we know Bionic supports it. The idea is the build would set this ANDROID_HAS_ELF_TLS variable when building for Q+ )

Why we can't just switch to elf TLS?

It seems to me that ideally the NDK clang should be defaulting to -fno-emulated-tls for android with a new enough target platform version, and should also be defaulting to use lld. I think that's been the plan for a while -- do you know if it's going to actually happen soon?

That might then make some of these changes unnecessary.

Why we can't just switch to elf TLS?

It seems to me that ideally the NDK clang should be defaulting to -fno-emulated-tls for android with a new enough target platform version, and should also be defaulting to use lld. I think that's been the plan for a while -- do you know if it's going to actually happen soon?

That might then make some of these changes unnecessary.

@eugenis WDYT?

Why we can't just switch to elf TLS?

It seems to me that ideally the NDK clang should be defaulting to -fno-emulated-tls for android with a new enough target platform version, and should also be defaulting to use lld. I think that's been the plan for a while -- do you know if it's going to actually happen soon?

That might then make some of these changes unnecessary.

@danalbert to make sure I don't mix this up. https://android.googlesource.com/platform/ndk/+/master/docs/Roadmap.md#ndk-r22 shows that NDK r22 (which is going to beta soon) does indeed change the default linker to LLD. For TLS, this is indeed sensitive to target API, and Android will still be targeting those older APIs for quite some time (so we can't just remove support for them).

Why we can't just switch to elf TLS?

It seems to me that ideally the NDK clang should be defaulting to -fno-emulated-tls for android with a new enough target platform version, and should also be defaulting to use lld. I think that's been the plan for a while -- do you know if it's going to actually happen soon?

That might then make some of these changes unnecessary.

@eugenis WDYT?

This library is special - we target an old sdk level to be compatible with those devices, but use the new features under runtime checks, including all thread-local variable access.

Why we can't just switch to elf TLS?

It seems to me that ideally the NDK clang should be defaulting to -fno-emulated-tls for android with a new enough target platform version, and should also be defaulting to use lld. I think that's been the plan for a while -- do you know if it's going to actually happen soon?

That might then make some of these changes unnecessary.

@danalbert to make sure I don't mix this up. https://android.googlesource.com/platform/ndk/+/master/docs/Roadmap.md#ndk-r22 shows that NDK r22 (which is going to beta soon) does indeed change the default linker to LLD. For TLS, this is indeed sensitive to target API, and Android will still be targeting those older APIs for quite some time (so we can't just remove support for them).

if I read this correctly we can switch Android to lld unconditionally right now now?
If so @oontvoo can you please split TLS and LLD changes into separate patches?

Why we can't just switch to elf TLS?

It seems to me that ideally the NDK clang should be defaulting to -fno-emulated-tls for android with a new enough target platform version, and should also be defaulting to use lld. I think that's been the plan for a while -- do you know if it's going to actually happen soon?

That might then make some of these changes unnecessary.

@danalbert to make sure I don't mix this up. https://android.googlesource.com/platform/ndk/+/master/docs/Roadmap.md#ndk-r22 shows that NDK r22 (which is going to beta soon) does indeed change the default linker to LLD. For TLS, this is indeed sensitive to target API, and Android will still be targeting those older APIs for quite some time (so we can't just remove support for them).

if I read this correctly we can switch Android to lld unconditionally right now now?

The bot[0] needs to use ndk22. It's using 16.

[0] https://github.com/llvm/llvm-zorg/blob/e8ba87e92b857c14b7eb5466c4266a9e09a1f5fb/zorg/buildbot/builders/sanitizers/buildbot_android.sh#L28

If so @oontvoo can you please split TLS and LLD changes into separate patches?

Sorry, I'm not sure which changes you'd like to be split.
This patch contains *only* TLS and LLD changes.

oontvoo retitled this revision from Disable emulated-tls and use LLD for compiler-rt+tests on Android if ELF_TLS is presence. to Disable emulated-tls and use LLD for compiler-rt+tests on Android .Wed, Oct 21, 12:38 PM
vitalybuka added inline comments.Wed, Oct 21, 2:22 PM
compiler-rt/test/asan/lit.cfg.py
134

which bots?

oontvoo marked an inline comment as done.Wed, Oct 21, 2:24 PM
oontvoo added inline comments.
compiler-rt/test/asan/lit.cfg.py
134

I meant, llvm-zorg, the one that stills uses ndk 16

vitalybuka added inline comments.Wed, Oct 21, 2:48 PM
compiler-rt/test/asan/lit.cfg.py
134

Then I'd rather update ndk there than use this hack.

srhines added inline comments.Wed, Oct 21, 3:41 PM
compiler-rt/test/asan/lit.cfg.py
134

NDK 16 is very old, and should really be updated. I assume you're building and using your new Clang instead of using the ancient NDK's Clang. In that case, you should be able to get the default to LLD.

oontvoo added inline comments.Wed, Oct 21, 4:52 PM
compiler-rt/test/asan/lit.cfg.py
134

NDK 16 is very old, and should really be updated. I assume you're building and using your new Clang instead of using the ancient NDK's Clang. In that case, you should be able to get the default to LLD.

I have tried using 21 and got stuck at the following error:

-- Check for working C compiler: /mnt/ssd/repo/buildbot/d_emulated_tls/run_dir/llvm_build64/bin/clang -- broken
CMake Error at /usr/share/cmake-3.16/Modules/CMakeTestCCompiler.cmake:60 (message):
  The C compiler

    "/mnt/ssd/repo/buildbot/d_emulated_tls/run_dir/llvm_build64/bin/clang"

  is not able to compile a simple test program.

  It fails with the following output:

    Change Dir: /mnt/ssd/repo/buildbot/d_emulated_tls/run_dir/compiler_rt_build_android_aarch64/CMakeFiles/CMakeTmp
    
    Run Build Command(s):/usr/bin/ninja cmTC_c605e && [1/2] Building C object CMakeFiles/cmTC_c605e.dir/testCCompiler.c.o
    [2/2] Linking C executable cmTC_c605e
    FAILED: cmTC_c605e 
    : && /mnt/ssd/repo/buildbot/d_emulated_tls/run_dir/llvm_build64/bin/clang --target=aarch64-linux-android --sysroot=/mnt/ssd/repo/buildbot/d_emulated_tls/run_dir/android_ndk/standalone-aarch64/sysroot -B/mnt/ssd/repo/buildbot/d_emulated_tls/run_dir/android_ndk/standalone-aarch64   CMakeFiles/cmTC_c605e.dir/testCCompiler.c.o  -o cmTC_c605e   && :
    /mnt/ssd/repo/buildbot/d_emulated_tls/run_dir/android_ndk/standalone-aarch64/lib/gcc/aarch64-linux-android/4.9.x/../../../../aarch64-linux-android/bin/ld: cannot find crtbegin_dynamic.o: No such file or directory
    /mnt/ssd/repo/buildbot/d_emulated_tls/run_dir/android_ndk/standalone-aarch64/lib/gcc/aarch64-linux-android/4.9.x/../../../../aarch64-linux-android/bin/ld: cannot find crtend_android.o: No such file or directory
    clang-12: error: linker command failed with exit code 1 (use -v to see invocation)
    ninja: build stopped: subcommand failed.

The only thing I found was this bug b/22405257, which was supposed to be fixed already ....

vitalybuka added inline comments.Wed, Oct 21, 5:56 PM
compiler-rt/test/asan/lit.cfg.py
134

ndk layout changed
zorg script needs update in how sysroot is calculate

oontvoo added inline comments.Wed, Oct 21, 6:11 PM
compiler-rt/test/asan/lit.cfg.py
134

No, the sysroot is still at the expected location in the ndk package.

I think it's the clang driver <run_dir>/llvm_build64/bin/clang (that llvm-zorg built) that is doing the wrong thing.

vitalybuka added inline comments.Wed, Oct 21, 6:14 PM
compiler-rt/test/asan/lit.cfg.py
134
vitalybuka added inline comments.Wed, Oct 21, 6:34 PM
compiler-rt/test/asan/lit.cfg.py
134

before r19 it was always at
./android_ndk/standalone-i686/sysroot/usr/lib/crtbegin_dynamic.o

now it's like
./android_ndk/standalone-i686/sysroot/usr/lib/x86_64-linux-android/24/crtbegin_dynamic.o
./android_ndk/standalone-i686/sysroot/usr/lib/x86_64-linux-android/26/crtbegin_dynamic.o
./android_ndk/standalone-i686/sysroot/usr/lib/x86_64-linux-android/27/crtbegin_dynamic.o
./android_ndk/standalone-i686/sysroot/usr/lib/x86_64-linux-android/28/crtbegin_dynamic.o

oontvoo updated this revision to Diff 300111.Thu, Oct 22, 3:12 PM
oontvoo marked 2 inline comments as done.
oontvoo retitled this revision from Disable emulated-tls and use LLD for compiler-rt+tests on Android to Disable emulated-tls and use LLD for compiler-rt+tests on Android.

Removed the over-align hack, because D89924 is upgrading ndk to 21.