Skip to content

[compiler-rt] Moved cmake libatomic check to top level config-ix #99437

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 22, 2024

Conversation

cjappl
Copy link
Contributor

@cjappl cjappl commented Jul 18, 2024

Also add atomic check to rtsan for when the tests are merged (related review #98679)

@llvmbot
Copy link
Member

llvmbot commented Jul 18, 2024

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

Author: Chris Apple (cjappl)

Changes

Also add atomic check to rtsan for when the tests are merged (related review #98679)


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

3 Files Affected:

  • (modified) compiler-rt/cmake/config-ix.cmake (+1)
  • (modified) compiler-rt/lib/rtsan/tests/CMakeLists.txt (+1)
  • (modified) compiler-rt/lib/scudo/standalone/tests/CMakeLists.txt (+2-5)
diff --git a/compiler-rt/cmake/config-ix.cmake b/compiler-rt/cmake/config-ix.cmake
index 74fdbd288a1f0..b69c391751707 100644
--- a/compiler-rt/cmake/config-ix.cmake
+++ b/compiler-rt/cmake/config-ix.cmake
@@ -176,6 +176,7 @@ 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 9eda116541ae3..3b783c90c2658 100644
--- a/compiler-rt/lib/rtsan/tests/CMakeLists.txt
+++ b/compiler-rt/lib/rtsan/tests/CMakeLists.txt
@@ -36,6 +36,7 @@ set(RTSAN_UNITTEST_LINK_FLAGS
   ${SANITIZER_TEST_CXX_LIBRARIES}
   -no-pie)
 
+append_list_if(COMPILER_RT_HAS_LIBATOMIC -latomic RTSAN_UNITTEST_LINK_FLAGS)
 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)
diff --git a/compiler-rt/lib/scudo/standalone/tests/CMakeLists.txt b/compiler-rt/lib/scudo/standalone/tests/CMakeLists.txt
index 672cdc0ba655e..a85eb737dba0a 100644
--- a/compiler-rt/lib/scudo/standalone/tests/CMakeLists.txt
+++ b/compiler-rt/lib/scudo/standalone/tests/CMakeLists.txt
@@ -46,11 +46,8 @@ set(SCUDO_UNITTEST_LINK_FLAGS
   ${COMPILER_RT_UNWINDER_LINK_LIBS}
   ${SANITIZER_TEST_CXX_LIBRARIES})
 list(APPEND SCUDO_UNITTEST_LINK_FLAGS -pthread -no-pie)
-# Linking against libatomic is required with some compilers
-check_library_exists(atomic __atomic_load_8 "" COMPILER_RT_HAS_LIBATOMIC)
-if (COMPILER_RT_HAS_LIBATOMIC)
-  list(APPEND SCUDO_UNITTEST_LINK_FLAGS -latomic)
-endif()
+
+append_list_if(COMPILER_RT_HAS_LIBATOMIC -latomic SCUDO_UNITTEST_LINK_FLAGS)
 
 set(SCUDO_TEST_HEADERS
   scudo_unit_test.h

@cjappl
Copy link
Contributor Author

cjappl commented Jul 18, 2024

@cjappl
Copy link
Contributor Author

cjappl commented Jul 22, 2024

Also for any future reviewers: I don't have merge access, so if things are looking good I'd love the help merging! Thanks :)

@MaskRay
Copy link
Member

MaskRay commented Jul 22, 2024

This looks good in the absence of further response.

I am not very experienced with CMake, so I waited a bit in case others have opinions... I think this should be safe.

@MaskRay MaskRay merged commit b846176 into llvm:main Jul 22, 2024
10 checks passed
@cjappl cjappl deleted the cjappl/atomic_cmake_check branch July 23, 2024 15:53
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
)

Also add atomic check to rtsan for when the tests are merged (related
review #98679)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants