[CMake][compiler-rt] Support for using compiler-rt atomic library

[petrhosek]

Not every toolchain provides and want to use libatomic which is a part of GCC, some toolchains may opt into using compiler-rt atomic library.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Petr Hosek (petrhosek)

Changes

Not every toolchain provides and want to use libatomic which is a part of GCC, some toolchains may opt into using compiler-rt atomic library.

---

Full diff: https://github.com/llvm/llvm-project/pull/106603.diff

5 Files Affected:

• (modified) clang/cmake/caches/Fuchsia-stage2.cmake (+1)
• (modified) cmake/Modules/HandleCompilerRT.cmake (+10-6)
• (modified) compiler-rt/CMakeLists.txt (+2)
• (modified) compiler-rt/cmake/config-ix.cmake (+8-1)
• (modified) compiler-rt/lib/rtsan/tests/CMakeLists.txt (+5-1)

diff --git a/clang/cmake/caches/Fuchsia-stage2.cmake b/clang/cmake/caches/Fuchsia-stage2.cmake
index 4ef37a5fad67f5..a6d619c5fe714b 100644
--- a/clang/cmake/caches/Fuchsia-stage2.cmake
+++ b/clang/cmake/caches/Fuchsia-stage2.cmake
@@ -169,6 +169,7 @@ foreach(target aarch64-unknown-linux-gnu;armv7-unknown-linux-gnueabihf;i386-unkn
     set(RUNTIMES_${target}_CMAKE_EXE_LINKER_FLAGS "-fuse-ld=lld" CACHE STRING "")
     set(RUNTIMES_${target}_COMPILER_RT_CXX_LIBRARY "libcxx" CACHE STRING "")
     set(RUNTIMES_${target}_COMPILER_RT_USE_BUILTINS_LIBRARY ON CACHE BOOL "")
+    set(RUNTIMES_${target}_COMPILER_RT_USE_ATOMIC_LIBRARY ON CACHE BOOL "")
     set(RUNTIMES_${target}_COMPILER_RT_USE_LLVM_UNWINDER ON CACHE BOOL "")
     set(RUNTIMES_${target}_COMPILER_RT_CAN_EXECUTE_TESTS ON CACHE BOOL "")
     set(RUNTIMES_${target}_COMPILER_RT_BUILD_STANDALONE_LIBATOMIC ON CACHE BOOL "")
diff --git a/cmake/Modules/HandleCompilerRT.cmake b/cmake/Modules/HandleCompilerRT.cmake
index 6865f45175ed79..0a7c56bc630934 100644
--- a/cmake/Modules/HandleCompilerRT.cmake
+++ b/cmake/Modules/HandleCompilerRT.cmake
@@ -51,7 +51,7 @@ endfunction()
 # This calls cache_compiler_rt_library that caches the path to speed up
 # repeated invocations with the same `name` and `target`.
 function(find_compiler_rt_library name variable)
-  cmake_parse_arguments(ARG "" "TARGET;FLAGS" "" ${ARGN})
+  cmake_parse_arguments(ARG "SHARED" "TARGET;FLAGS" "" ${ARGN})
   # While we can use compiler-rt runtimes with other compilers, we need to
   # query the compiler for runtime location and thus we require Clang.
   if(NOT CMAKE_CXX_COMPILER_ID MATCHES Clang)
@@ -112,12 +112,16 @@ function(find_compiler_rt_library name variable)
     # path and then checking if the resultant path exists. The result of
     # this check is also cached by cache_compiler_rt_library.
     set(library_file "${COMPILER_RT_LIBRARY_builtins_${target}}")
-    if(library_file MATCHES ".*clang_rt\.([a-z0-9_\-]+)\.(a|lib)")
-      set(from_name ${CMAKE_MATCH_0})
-      get_component_name(${name} to_name)
-      string(REPLACE "${from_name}" "${to_name}" library_file "${library_file}")
-      cache_compiler_rt_library(FALSE "${name}" "${target}" "${library_file}")
+    get_component_name("builtins" from_name)
+    get_component_name(${name} to_name)
+    get_filename_component(basename ${library_file} NAME)
+    string(REPLACE "${from_name}" "${to_name}" basename "${basename}")
+    if (ARG_SHARED)
+      string(REGEX REPLACE "\.(a|lib)$" ".so" basename "${basename}")
     endif()
+    get_filename_component(dirname ${library_file} DIRECTORY)
+    set(library_file "${dirname}/${basename}")
+    cache_compiler_rt_library(FALSE "${name}" "${target}" "${library_file}")
   endif()
   set(${variable} "${COMPILER_RT_LIBRARY_${name}_${target}}" PARENT_SCOPE)
 endfunction()
diff --git a/compiler-rt/CMakeLists.txt b/compiler-rt/CMakeLists.txt
index 2207555b03a03f..57914c3175e815 100644
--- a/compiler-rt/CMakeLists.txt
+++ b/compiler-rt/CMakeLists.txt
@@ -284,6 +284,8 @@ endif()
 option(COMPILER_RT_USE_BUILTINS_LIBRARY
   "Use compiler-rt builtins instead of libgcc" ${DEFAULT_COMPILER_RT_USE_BUILTINS_LIBRARY})
 
+option(COMPILER_RT_USE_ATOMIC_LIBRARY "Use compiler-rt atomic instead of libatomic" OFF)
+
 include(config-ix)
 
 #================================
diff --git a/compiler-rt/cmake/config-ix.cmake b/compiler-rt/cmake/config-ix.cmake
index 97204177cde9a4..a93a88a9205001 100644
--- a/compiler-rt/cmake/config-ix.cmake
+++ b/compiler-rt/cmake/config-ix.cmake
@@ -74,6 +74,14 @@ if (C_SUPPORTS_NODEFAULTLIBS_FLAG)
   endif()
 endif ()
 
+if (COMPILER_RT_USE_ATOMIC_LIBRARY)
+  include(HandleCompilerRT)
+  find_compiler_rt_library(atomic COMPILER_RT_ATOMIC_LIBRARY SHARED
+                           FLAGS ${SANITIZER_COMMON_FLAGS})
+else()
+  check_library_exists(atomic __atomic_load_8 "" COMPILER_RT_HAS_LIBATOMIC)
+endif()
+
 # CodeGen options.
 check_c_compiler_flag(-ffreestanding         COMPILER_RT_HAS_FFREESTANDING_FLAG)
 check_c_compiler_flag(-fomit-frame-pointer   COMPILER_RT_HAS_OMIT_FRAME_POINTER_FLAG)
@@ -175,7 +183,6 @@ check_cxx_compiler_flag(-nostdlib++ COMPILER_RT_HAS_NOSTDLIBXX_FLAG)
 check_include_files("sys/auxv.h"    COMPILER_RT_HAS_AUXV)
 
 # Libraries.
-check_library_exists(atomic __atomic_load_8 "" COMPILER_RT_HAS_LIBATOMIC)
 check_library_exists(dl dlopen "" COMPILER_RT_HAS_LIBDL)
 check_library_exists(rt shm_open "" COMPILER_RT_HAS_LIBRT)
 check_library_exists(m pow "" COMPILER_RT_HAS_LIBM)
diff --git a/compiler-rt/lib/rtsan/tests/CMakeLists.txt b/compiler-rt/lib/rtsan/tests/CMakeLists.txt
index 0320bbad592186..1681f1062e1ba1 100644
--- a/compiler-rt/lib/rtsan/tests/CMakeLists.txt
+++ b/compiler-rt/lib/rtsan/tests/CMakeLists.txt
@@ -36,7 +36,11 @@ set(RTSAN_UNITTEST_LINK_FLAGS
   ${SANITIZER_TEST_CXX_LIBRARIES}
   -no-pie)
 
-append_list_if(COMPILER_RT_HAS_LIBATOMIC -latomic RTSAN_UNITTEST_LINK_FLAGS)
+if (COMPILER_RT_USE_ATOMIC_LIBRARY)
+  list(APPEND RTSAN_UNITTEST_LINK_FLAGS ${COMPILER_RT_ATOMIC_LIBRARY})
+else()
+  append_list_if(COMPILER_RT_HAS_LIBATOMIC -latomic RTSAN_UNITTEST_LINK_FLAGS)
+endif()
 append_list_if(COMPILER_RT_HAS_LIBDL -ldl RTSAN_UNITTEST_LINK_FLAGS)
 append_list_if(COMPILER_RT_HAS_LIBRT -lrt RTSAN_UNITTEST_LINK_FLAGS)
 append_list_if(COMPILER_RT_HAS_LIBM -lm RTSAN_UNITTEST_LINK_FLAGS)

[cjappl]

Seems reasonable to me, I would wait for other approvals, as I'm relatively new here.

Just ensure this is good to go with rtsan, I recommend running the tests locally, as I don't think they're a part of the default pre-commit set

ninja check-rtsan

[smeenai]

LGTM

[petrhosek]

Seems reasonable to me, I would wait for other approvals, as I'm relatively new here.

Just ensure this is good to go with rtsan, I recommend running the tests locally, as I don't think they're a part of the default pre-commit set

ninja check-rtsan

I'm seeing a test failure but I'm not sure if it's an issue with RTSan or compiler-rt atomic implementation:

FAIL: RealtimeSanitizer-Unit :: ./Rtsan-x86_64-Test/13/75 (1 of 84)
******************** TEST 'RealtimeSanitizer-Unit :: ./Rtsan-x86_64-Test/13/75' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:/usr/local/google/home/phosek/llvm/llvm-project/build/fuchsia/runtimes/runtimes-x86_64-unknown-linux-gnu-bins/compiler-rt/lib/rtsan/tests/./Rtsan-x86_64-Test-RealtimeSanitizer-Unit-1864241-13-75.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=75 GTEST_SHARD_INDEX=13 /usr/local/google/home/phosek/llvm/llvm-project/build/fuchsia/runtimes/runtimes-x86_64-unknown-linux-gnu-bins/compiler-rt/lib/rtsan/tests/./Rtsan-x86_64-Test
--

Script:
--
/usr/local/google/home/phosek/llvm/llvm-project/build/fuchsia/runtimes/runtimes-x86_64-unknown-linux-gnu-bins/compiler-rt/lib/rtsan/tests/./Rtsan-x86_64-Test --gtest_filter=TestRtsan.AccessingALargeAtomicVariableDiesWhenRealtime
--
compiler-rt/lib/rtsan/tests/rtsan_test_utilities.h:40: Failure
Death test: RealtimeInvoke(std::forward<Function>(Func))
    Result: failed to die.
 Error msg:
[  DEATH   ] 


compiler-rt/lib/rtsan/tests/rtsan_test_utilities.h:40
Death test: RealtimeInvoke(std::forward<Function>(Func))
    Result: failed to die.
 Error msg:
[  DEATH   ] 



********************
********************
Failed Tests (1):
  RealtimeSanitizer-Unit :: ./Rtsan-x86_64-Test/TestRtsan/AccessingALargeAtomicVariableDiesWhenRealtime

[cjappl]

I'm seeing a test failure but I'm not sure if it's an issue with RTSan or compiler-rt atomic implementation:

Interesting, so that test basically tests that large atomics that insert locks under the hood die as expected. Zooming in on the second part of the test:

std::atomic<std::array<float, 2048>> large_atomic;
  ASSERT_FALSE(large_atomic.is_lock_free());     ///// HERE IS INTERESTING!
  auto Func = [&]() {
    std::array<float, 2048> x = large_atomic.load();
    return x;
  };
  ExpectRealtimeDeath(Func);
  ExpectNonRealtimeSurvival(Func);

It seems like the compiler-rt version says it is lock free, but then we aren't dying. Does it take another lock, different from a pthread_mutex_lock? Those are the locks we look for in our implementation.

[cjappl]

Ok, it appears that on Apple systems, we would catch this, as we intercept OSSpinLockLock used by compiler-rt atomics:

llvm-project/compiler-rt/lib/builtins/atomic.c

Lines 95 to 102 in c55e24b

	#elif defined(__APPLE__)
	#include <libkern/OSAtomic.h>
	typedef OSSpinLock Lock;
	__inline static void unlock(Lock *l) { OSSpinLockUnlock(l); }
	/// Locks a lock. In the current implementation, this is potentially
	/// unbounded in the contended case.
	__inline static void lock(Lock *l) { OSSpinLockLock(l); }
	static Lock locks[SPINLOCK_COUNT]; // initialized to OS_SPINLOCK_INIT which is 0

But, in the non-darwin case this is an atomic CAS spinlock, which we do not intercept (but we are trying to figure out how to catch potentially unbound loops like this):

llvm-project/compiler-rt/lib/builtins/atomic.c

Lines 112 to 119 in c55e24b

	/// Locks a lock. In the current implementation, this is potentially
	/// unbounded in the contended case.
	__inline static void lock(Lock *l) {
	uintptr_t old = 0;
	while (!__c11_atomic_compare_exchange_weak(l, &old, 1, __ATOMIC_ACQUIRE,
	__ATOMIC_RELAXED))
	old = 0;
	}

Would you please confirm that you end up in this area of atomic.c when you run this test? If so, I think it is adequate to #ifdef out this test under the circumstances that it is

1. Compiling with the builtin atomics
2. Not on darwin

llvm-project/compiler-rt/lib/rtsan/tests/rtsan_test_functional.cpp

Lines 164 to 171 in d58d105

	std::atomic<std::array<float, 2048>> large_atomic;
	ASSERT_FALSE(large_atomic.is_lock_free());
	auto Func = [&]() {
	std::array<float, 2048> x = large_atomic.load();
	return x;
	};
	ExpectRealtimeDeath(Func);
	ExpectNonRealtimeSurvival(Func);

Or if you're feeling ambitious, you could pull this little bit into a new test and ifdef out that, might be cleaner.

Let me know if I can be of any assistance with any of this.

[petrhosek]

I'm building on Linux so I believe we use the atomic CAS spinlock. There's an option to use pthreads though:

llvm-project/compiler-rt/lib/builtins/CMakeLists.txt

Lines 240 to 242 in 130eddf

	option(COMPILER_RT_LIBATOMIC_USE_PTHREAD
	"Whether libatomic should use pthreads if available."
	Off)

I'll check if enabling the use of pthreads would make this test pass, but it'd be better if we handled atomic CAS spinlocks as well since we would like to avoid the pthread dependency if possible.

[cjappl]

but it'd be better if we handled atomic CAS spinlocks as well since we would like to avoid the pthread dependency if possible.

Agreed, I have something in the works for this but it is a little ways off. (catching these unbound loops is a bit tricky)

My recommendation to make it work when using the builtins would be just extracting this test to another test, then #ifdefing it out in the situation where we are not using pthread