This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by oontvoo on Oct 16 2020, 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.Oct 16 2020, 11:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 16 2020, 11:08 PM
Herald added subscribers: Restricted Project, mgorny, dberris. · View Herald Transcript
oontvoo requested review of this revision.Oct 16 2020, 11:08 PM
oontvoo updated this revision to Diff 298810.Oct 17 2020, 12:12 AM

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

dmajor added a subscriber: dmajor.Oct 17 2020, 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 .Oct 21 2020, 12:38 PM
vitalybuka added inline comments.Oct 21 2020, 2:22 PM
compiler-rt/test/asan/lit.cfg.py
134 ↗(On Diff #299780)

which bots?

oontvoo marked an inline comment as done.Oct 21 2020, 2:24 PM
oontvoo added inline comments.
compiler-rt/test/asan/lit.cfg.py
134 ↗(On Diff #299780)

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

vitalybuka added inline comments.Oct 21 2020, 2:48 PM
compiler-rt/test/asan/lit.cfg.py
134 ↗(On Diff #299780)

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

srhines added inline comments.Oct 21 2020, 3:41 PM
compiler-rt/test/asan/lit.cfg.py
134 ↗(On Diff #299780)

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.Oct 21 2020, 4:52 PM
compiler-rt/test/asan/lit.cfg.py
134 ↗(On Diff #299780)

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.Oct 21 2020, 5:56 PM
compiler-rt/test/asan/lit.cfg.py
134 ↗(On Diff #299780)

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

oontvoo added inline comments.Oct 21 2020, 6:11 PM
compiler-rt/test/asan/lit.cfg.py
134 ↗(On Diff #299780)

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.Oct 21 2020, 6:14 PM
compiler-rt/test/asan/lit.cfg.py
134 ↗(On Diff #299780)
vitalybuka added inline comments.Oct 21 2020, 6:34 PM
compiler-rt/test/asan/lit.cfg.py
134 ↗(On Diff #299780)

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.Oct 22 2020, 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.

oontvoo updated this revision to Diff 302657.Nov 3 2020, 12:15 PM

check api-level and set ANDROID_HAS_ELF_TLS

vitalybuka added inline comments.Nov 3 2020, 12:57 PM
compiler-rt/cmake/config-ix.cmake
9 ↗(On Diff #302657)

It should not be an option, just a variable

11 ↗(On Diff #302657)

Can you move below close to other COMPILER_RT_HAS_ var assignment

and wrap that with if (ANDROID)

compiler-rt/lib/asan/CMakeLists.txt
232 ↗(On Diff #302657)

I would prefer we switch android to lld completly so COMPILER_RT_HAS_GNU_VERSION_SCRIPT_COMPAT is OFF

91 ↗(On Diff #300111)

why just asan? we should do that for entire compiler rt?

compiler-rt/lib/lsan/CMakeLists.txt
39

lets move it to upper level

compiler-rt/test/asan/TestCases/Linux/use_tls_test.cpp
6 ↗(On Diff #302657)

the test can go into sanitizer_common

compiler-rt/test/asan/lit.cfg.py
133 ↗(On Diff #302657)

undo

compiler-rt/test/lit.common.cfg.py
376

has_lld is controlled by cmake and should not be touched here

391

not config.android here

compiler-rt/test/lit.common.configured.in
38

maybe

oontvoo updated this revision to Diff 302698.Nov 3 2020, 3:02 PM
oontvoo marked 8 inline comments as done.

updated diff

vitalybuka accepted this revision.Nov 4 2020, 4:13 AM
vitalybuka added inline comments.
compiler-rt/test/lit.common.cfg.py
369

I was not sure why we need api level of compilation

This revision is now accepted and ready to land.Nov 4 2020, 4:13 AM
oontvoo updated this revision to Diff 302828.Nov 4 2020, 6:24 AM
oontvoo marked an inline comment as done.

rebase

oontvoo added inline comments.Nov 4 2020, 6:30 AM
compiler-rt/test/lit.common.cfg.py
369

Sorry, hadn't realised you updated the diff. (was wondering why it looked different from what I had locally).

The thread-properties-api feature is different than the rest because it's unreleased. That is to say, this >=31 will be false for now. We'll need both the api-level and the codename.
(In the future, when it's released, the codename will be "REL" and API level will be 31. But right now, the API level will be 29 or 30, and codename can be "S").

In any case, this should be moved to the lsan patch.

This revision was landed with ongoing or failed builds.Nov 4 2020, 6:51 AM
This revision was automatically updated to reflect the committed changes.
fhahn added a subscriber: fhahn.Nov 4 2020, 7:02 AM
fhahn added inline comments.
compiler-rt/CMakeLists.txt
280

This appears to cause the following failure with cmake version 3.16.5

CMake Error at lvm-project/compiler-rt/CMakeLists.txt:280 (if):
  if given arguments:

    "GREATER_EQUAL" "28"

  Unknown arguments specified
oontvoo marked an inline comment as done.Nov 4 2020, 7:15 AM
oontvoo added inline comments.
compiler-rt/CMakeLists.txt
280

Thanks! Sent D90764. PTAL