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

[oontvoo]

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)

[oontvoo]

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

[oontvoo]

add test

[vitalybuka]

Why we can't just switch to elf TLS?

[oontvoo]

In D89615#2342857, @vitalybuka wrote:

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+ )

[jyknight]

In D89615#2342857, @vitalybuka wrote:

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.

[vitalybuka]

In D89615#2345350, @jyknight wrote:

In D89615#2342857, @vitalybuka wrote:

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?

[srhines]

In D89615#2345350, @jyknight wrote:

In D89615#2342857, @vitalybuka wrote:

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

[eugenis]

In D89615#2345429, @vitalybuka wrote:

In D89615#2345350, @jyknight wrote:

In D89615#2342857, @vitalybuka wrote:

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.

[vitalybuka]

In D89615#2345430, @srhines wrote:

In D89615#2345350, @jyknight wrote:

In D89615#2342857, @vitalybuka wrote:

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?

[oontvoo]

<diff>

[oontvoo]

In D89615#2345448, @vitalybuka wrote:

In D89615#2345430, @srhines wrote:

In D89615#2345350, @jyknight wrote:

In D89615#2342857, @vitalybuka wrote:

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.

[vitalybuka]

compiler-rt/test/asan/lit.cfg.py

134 ↗

(On Diff #299780)

which bots?

[oontvoo]

compiler-rt/test/asan/lit.cfg.py

134 ↗

(On Diff #299780)

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

[vitalybuka]

compiler-rt/test/asan/lit.cfg.py

134 ↗

(On Diff #299780)

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

[srhines]

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]

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]

compiler-rt/test/asan/lit.cfg.py

134 ↗

(On Diff #299780)

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

[oontvoo]

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]

compiler-rt/test/asan/lit.cfg.py

134 ↗

(On Diff #299780)

looks related https://developer.android.com/ndk/guides/standalone_toolchain

[vitalybuka]

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]

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

[oontvoo]

rebase

[oontvoo]

check api-level and set ANDROID_HAS_ELF_TLS

[vitalybuka]

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 diff

[vitalybuka]

update

[vitalybuka]

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

369

I was not sure why we need api level of compilation

[oontvoo]

rebase

[oontvoo]

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.

[oontvoo]

rebase

[fhahn]

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]

compiler-rt/CMakeLists.txt

280

Thanks! Sent D90764. PTAL

compiler-rt/CMakeLists.txt

 if ("${COMPILER_RT_DEFAULT_TARGET_TRIPLE}" MATCHES ".*hf$")
   if (${COMPILER_RT_DEFAULT_TARGET_ARCH} MATCHES "^arm")
     set(COMPILER_RT_DEFAULT_TARGET_ARCH "armhf")
     CHECK_SYMBOL_EXISTS (__thumb__ "" COMPILER_RT_ARM_THUMB)
   endif()
 endif()
 if ("${COMPILER_RT_DEFAULT_TARGET_TRIPLE}" MATCHES ".*android.*")
   set(ANDROID 1)
+  string(REGEX MATCH "-target(=| +)[^ ]+android([0-9]+)" ANDROID_API_LEVEL "${CMAKE_C_FLAGS}")
+  set(ANDROID_API_LEVEL ${CMAKE_MATCH_2}) 
 endif()
 pythonize_bool(ANDROID)
 
 set(COMPILER_RT_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR})
 set(COMPILER_RT_BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR})
 
 pythonize_bool(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR)
 

 append_string_if(COMPILER_RT_HAS_STD_CXX14_FLAG -std=c++14 CMAKE_CXX_FLAGS)
 
 # Emulate C99 and C++11's __func__ for MSVC prior to 2013 CTP.
 if(NOT COMPILER_RT_HAS_FUNC_SYMBOL)
   add_definitions(-D__func__=__FUNCTION__)
 endif()
 
 # Provide some common commmandline flags for Sanitizer runtimes.
+if(${ANDROID_API_LEVEL} GREATER_EQUAL 28)
+  list(APPEND SANITIZER_COMMON_CFLAGS -fno-emulated-tls)
+  list(APPEND COMPILER_RT_TEST_COMPILER_CFLAGS -fno-emulated-tls)
+endif()
 if(NOT WIN32)
   append_list_if(COMPILER_RT_HAS_FPIC_FLAG -fPIC SANITIZER_COMMON_CFLAGS)
 endif()
 append_list_if(COMPILER_RT_HAS_FNO_BUILTIN_FLAG -fno-builtin SANITIZER_COMMON_CFLAGS)
 append_list_if(COMPILER_RT_HAS_FNO_EXCEPTIONS_FLAG -fno-exceptions SANITIZER_COMMON_CFLAGS)
 if(NOT COMPILER_RT_DEBUG AND NOT APPLE)
   append_list_if(COMPILER_RT_HAS_FOMIT_FRAME_POINTER_FLAG -fomit-frame-pointer SANITIZER_COMMON_CFLAGS)
 endif()

compiler-rt/lib/lsan/CMakeLists.txt

 include_directories(..)
 
 set(LSAN_CFLAGS ${SANITIZER_COMMON_CFLAGS})
+set(LSAN_LINK_FLAGS ${SANITIZER_COMMON_LINK_FLAGS})
 append_rtti_flag(OFF LSAN_CFLAGS)
 
 set(LSAN_COMMON_SOURCES
   lsan_common.cpp
   lsan_common_fuchsia.cpp
   lsan_common_linux.cpp
   lsan_common_mac.cpp
   )
set(LSAN_HEADERS
   lsan_flags.inc
   lsan_thread.h
   )
 
 set(LSAN_SRC_DIR ${CMAKE_CURRENT_SOURCE_DIR})
 
 add_compiler_rt_object_libraries(RTLSanCommon
     OS ${SANITIZER_COMMON_SUPPORTED_OS}
     ARCHS ${LSAN_COMMON_SUPPORTED_ARCH}
     SOURCES ${LSAN_COMMON_SOURCES}
     ADDITIONAL_HEADERS ${LSAN_HEADERS}
     CFLAGS ${LSAN_CFLAGS})
 
 if(COMPILER_RT_HAS_LSAN)
   add_compiler_rt_component(lsan)
   if(APPLE)
     set(LSAN_LINK_LIBS ${SANITIZER_COMMON_LINK_LIBS})
add_compiler_rt_runtime(clang_rt.lsan
       ADDITIONAL_HEADERS ${LSAN_HEADERS}
       OBJECT_LIBS RTLSanCommon
                   RTInterception
                   RTSanitizerCommon
                   RTSanitizerCommonLibc
                   RTSanitizerCommonCoverage
                   RTSanitizerCommonSymbolizer
       CFLAGS ${LSAN_CFLAGS}
-      LINK_FLAGS ${SANITIZER_COMMON_LINK_FLAGS} ${WEAK_SYMBOL_LINK_FLAGS}
+      LINK_FLAGS ${LSAN_LINK_FLAGS} ${WEAK_SYMBOL_LINK_FLAGS}
       LINK_LIBS ${LSAN_LINK_LIBS}
       PARENT_TARGET lsan)
   else()
     foreach(arch ${LSAN_SUPPORTED_ARCH})
       add_compiler_rt_runtime(clang_rt.lsan
         STATIC
         ARCHS ${arch}
         SOURCES ${LSAN_SOURCES}
                 $<TARGET_OBJECTS:RTInterception.${arch}>
                 $<TARGET_OBJECTS:RTSanitizerCommon.${arch}>
                 $<TARGET_OBJECTS:RTSanitizerCommonLibc.${arch}>
                 $<TARGET_OBJECTS:RTSanitizerCommonCoverage.${arch}>
                 $<TARGET_OBJECTS:RTSanitizerCommonSymbolizer.${arch}>
                 $<TARGET_OBJECTS:RTLSanCommon.${arch}>
         ADDITIONAL_HEADERS ${LSAN_HEADERS}
         CFLAGS ${LSAN_CFLAGS}
+        LINK_FLAGS ${LSAN_LINK_FLAGS}
         PARENT_TARGET lsan)
     endforeach()
   endif()
 endif()

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

if config.android:
   # suppression-files, to device.
   config.substitutions.append( ('%device_rundir', "/data/local/tmp/Output") )
   config.substitutions.append( ('%push_to_device', "%s -s '%s' push " % (adb, env['ANDROID_SERIAL']) ) )
   config.substitutions.append( ('%pull_from_device', "%s -s '%s' pull " % (adb, env['ANDROID_SERIAL']) ) )
   config.substitutions.append( ('%adb_shell ', "%s -s '%s' shell " % (adb, env['ANDROID_SERIAL']) ) )
 
   try:
     android_api_level_str = subprocess.check_output([adb, "shell", "getprop", "ro.build.version.sdk"], env=env).rstrip()
+    android_api_codename = subprocess.check_output([adb, "shell", "getprop", "ro.build.version.codename"], env=env).rstrip().decode("utf-8")
   except (subprocess.CalledProcessError, OSError):
     lit_config.fatal("Failed to read ro.build.version.sdk (using '%s' as adb)" % adb)
   try:
     android_api_level = int(android_api_level_str)
   except ValueError:
     lit_config.fatal("Failed to read ro.build.version.sdk (using '%s' as adb): got '%s'" % (adb, android_api_level_str))
   if android_api_level >= 26:
     config.available_features.add('android-26')
   if android_api_level >= 28:
     config.available_features.add('android-28')
+  if android_api_level >= 31 or android_api_codename == 'S':
+    config.available_features.add('android-thread-properties-api')
 
   # Prepare the device.
   android_tmpdir = '/data/local/tmp/Output'
   subprocess.check_call([adb, "shell", "mkdir", "-p", android_tmpdir], env=env)
   for file in config.android_files_to_push:
     subprocess.check_call([adb, "push", file, android_tmpdir], env=env)
 else:
   config.substitutions.append( ('%device_rundir', "") )
   config.substitutions.append( ('%push_to_device', "echo ") )
   config.substitutions.append( ('%pull_from_device', "echo ") )
   config.substitutions.append( ('%adb_shell', "echo ") )
 
 if config.host_os == 'Linux':
   # detect whether we are using glibc, and which version
   # NB: 'ldd' is just one of the tools commonly installed as part of glibc
   ldd_ver_cmd = subprocess.Popen(['ldd', '--version'],
                                  stdout=subprocess.PIPE,
                                  env={'LANG': 'C'})
   sout, _ = ldd_ver_cmd.communicate()
   ver_line = sout.splitlines()[0]
-  if ver_line.startswith(b"ldd "):
+  if not config.android and ver_line.startswith(b"ldd "):
     from distutils.version import LooseVersion
     ver = LooseVersion(ver_line.split()[-1].decode())
     # 2.27 introduced some incompatibilities
     if ver >= LooseVersion("2.27"):
       config.available_features.add("glibc-2.27")
 
 sancovcc_path = os.path.join(config.llvm_tools_dir, "sancov")
 if os.path.exists(sancovcc_path):

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

 set_default("emulator", "@COMPILER_RT_EMULATOR@")
 set_default("asan_shadow_scale", "@COMPILER_RT_ASAN_SHADOW_SCALE@")
 set_default("memprof_shadow_scale", "@COMPILER_RT_MEMPROF_SHADOW_SCALE@")
 set_default("apple_platform", "osx")
 set_default("apple_platform_min_deployment_target_flag", "-mmacosx-version-min")
 set_default("sanitizer_can_use_cxxabi", @SANITIZER_CAN_USE_CXXABI_PYBOOL@)
 set_default("has_lld", @COMPILER_RT_HAS_LLD_PYBOOL@)
 set_default("can_symbolize", @CAN_SYMBOLIZE@)
 set_default("use_lld", @COMPILER_RT_TEST_USE_LLD_PYBOOL@)
 set_default("use_thinlto", False)
 set_default("use_lto", config.use_thinlto)
 set_default("use_newpm", False)
 set_default("android", @ANDROID_PYBOOL@)
+set_default("android_ndk_version", @ANDROID_NDK_VERSION@)
 set_default("android_serial", "@ANDROID_SERIAL_FOR_TESTING@")
 set_default("android_files_to_push", [])
 set_default("have_rpc_xdr_h", @HAVE_RPC_XDR_H@)
 set_default("gwp_asan", @COMPILER_RT_HAS_GWP_ASAN_PYBOOL@)
 set_default("expensive_checks", @LLVM_ENABLE_EXPENSIVE_CHECKS_PYBOOL@)
 config.available_features.add('target-is-%s' % config.target_arch)
 
 if config.enable_per_target_runtime_dir:

compiler-rt/test/sanitizer_common/TestCases/Linux/use_tls_test.cpp

+// Test that executable with ELF-TLS will link/run successfully on aarch64
+// RUN: %clangxx -fno-emulated-tls %s -o %t
+// RUN: %run %t 2>&1
+// REQUIRES: android-28
+
+#include <stdio.h>
+#include <stdlib.h>
+
+__thread void *tls_var;
+int var;
+
+void set_var() {
+  var = 123;
+  tls_var = &var;
+}
+int main() {
+  set_var();
+  fprintf(stderr, "Test alloc: %p\n", tls_var);
+  fflush(stderr);
+  return 0;
+}