[compiler-rt] Use installed libc++(abi) for tests instead of build tree #115077
Conversation
Created using spr 1.3.6-beta.1
llvmbot
added
compiler-rt
compiler-rt:fuzzer
compiler-rt:tsan
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Alexander Richardson (arichardson) ChangesUsing the build tree is somewhat fragile since the layout is not Full diff: https://github.com/llvm/llvm-project/pull/115077.diff 5 Files Affected:
diff --git a/compiler-rt/cmake/Modules/AddCompilerRT.cmake b/compiler-rt/cmake/Modules/AddCompilerRT.cmake
index bfa1834229baab..abbeea0c493a75 100644
--- a/compiler-rt/cmake/Modules/AddCompilerRT.cmake
+++ b/compiler-rt/cmake/Modules/AddCompilerRT.cmake
@@ -680,15 +680,16 @@ macro(add_custom_libcxx name prefix)
ExternalProject_Add(${name}
DEPENDS ${name}-clobber ${LIBCXX_DEPS}
- PREFIX ${CMAKE_CURRENT_BINARY_DIR}/${name}
+ PREFIX ${prefix}
SOURCE_DIR ${LLVM_MAIN_SRC_DIR}/../runtimes
- BINARY_DIR ${prefix}
+ BINARY_DIR ${prefix}/build
CMAKE_ARGS ${CMAKE_PASSTHROUGH_VARIABLES}
${compiler_args}
${verbose}
-DCMAKE_C_FLAGS=${LIBCXX_C_FLAGS}
-DCMAKE_CXX_FLAGS=${LIBCXX_CXX_FLAGS}
-DCMAKE_BUILD_TYPE=Release
+ -DCMAKE_INSTALL_PREFIX=<INSTALL_DIR>
-DCMAKE_TRY_COMPILE_TARGET_TYPE=STATIC_LIBRARY
-DLLVM_ENABLE_RUNTIMES=libcxx|libcxxabi
-DLIBCXXABI_USE_LLVM_UNWINDER=OFF
@@ -701,16 +702,17 @@ macro(add_custom_libcxx name prefix)
-DLIBCXX_INCLUDE_BENCHMARKS=OFF
-DLIBCXX_INCLUDE_TESTS=OFF
-DLIBCXX_ENABLE_STATIC_ABI_LIBRARY=ON
+ -DLLVM_INCLUDE_TESTS=OFF
+ -DLLVM_INCLUDE_DOCS=OFF
${LIBCXX_CMAKE_ARGS}
- INSTALL_COMMAND ""
- STEP_TARGETS configure build
+ STEP_TARGETS configure build install
BUILD_ALWAYS 1
USES_TERMINAL_CONFIGURE 1
USES_TERMINAL_BUILD 1
USES_TERMINAL_INSTALL 1
LIST_SEPARATOR |
EXCLUDE_FROM_ALL TRUE
- BUILD_BYPRODUCTS "${prefix}/lib/libc++.a" "${prefix}/lib/libc++abi.a"
+ INSTALL_BYPRODUCTS "${prefix}/lib/libc++.a" "${prefix}/lib/libc++abi.a"
)
if (CMAKE_GENERATOR MATCHES "Make")
diff --git a/compiler-rt/lib/fuzzer/CMakeLists.txt b/compiler-rt/lib/fuzzer/CMakeLists.txt
index fb5adf1e5c9e69..a6175564e55a20 100644
--- a/compiler-rt/lib/fuzzer/CMakeLists.txt
+++ b/compiler-rt/lib/fuzzer/CMakeLists.txt
@@ -166,11 +166,11 @@ if(OS_NAME MATCHES "Android|Linux|Fuchsia" AND
-DLIBCXX_ABI_NAMESPACE=__Fuzzer
-DLIBCXX_ENABLE_EXCEPTIONS=OFF)
target_compile_options(RTfuzzer.${arch} PRIVATE -isystem ${LIBCXX_${arch}_PREFIX}/include/c++/v1)
- add_dependencies(RTfuzzer.${arch} libcxx_fuzzer_${arch}-build)
+ add_dependencies(RTfuzzer.${arch} libcxx_fuzzer_${arch}-install)
target_compile_options(RTfuzzer_main.${arch} PRIVATE -isystem ${LIBCXX_${arch}_PREFIX}/include/c++/v1)
- add_dependencies(RTfuzzer_main.${arch} libcxx_fuzzer_${arch}-build)
+ add_dependencies(RTfuzzer_main.${arch} libcxx_fuzzer_${arch}-install)
target_compile_options(RTfuzzer_interceptors.${arch} PRIVATE -isystem ${LIBCXX_${arch}_PREFIX}/include/c++/v1)
- add_dependencies(RTfuzzer_interceptors.${arch} libcxx_fuzzer_${arch}-build)
+ add_dependencies(RTfuzzer_interceptors.${arch} libcxx_fuzzer_${arch}-install)
partially_link_libcxx(fuzzer_no_main ${LIBCXX_${arch}_PREFIX} ${arch})
partially_link_libcxx(fuzzer_interceptors ${LIBCXX_${arch}_PREFIX} ${arch})
partially_link_libcxx(fuzzer ${LIBCXX_${arch}_PREFIX} ${arch})
diff --git a/compiler-rt/lib/fuzzer/tests/CMakeLists.txt b/compiler-rt/lib/fuzzer/tests/CMakeLists.txt
index 5086c0334b22be..73ebc135312090 100644
--- a/compiler-rt/lib/fuzzer/tests/CMakeLists.txt
+++ b/compiler-rt/lib/fuzzer/tests/CMakeLists.txt
@@ -64,7 +64,7 @@ if(COMPILER_RT_DEFAULT_TARGET_ARCH IN_LIST FUZZER_SUPPORTED_ARCH)
COMPILER_RT_LIBCXX_PATH AND
COMPILER_RT_LIBCXXABI_PATH)
file(GLOB libfuzzer_headers ../*.h)
- set(LIBFUZZER_TEST_RUNTIME_DEPS libcxx_fuzzer_${arch}-build ${libfuzzer_headers})
+ set(LIBFUZZER_TEST_RUNTIME_DEPS libcxx_fuzzer_${arch}-install ${libfuzzer_headers})
set(LIBFUZZER_TEST_RUNTIME_CFLAGS -isystem ${LIBCXX_${arch}_PREFIX}/include/c++/v1)
set(LIBFUZZER_TEST_RUNTIME_LINK_FLAGS ${LIBCXX_${arch}_PREFIX}/lib/libc++.a)
endif()
diff --git a/compiler-rt/lib/msan/tests/CMakeLists.txt b/compiler-rt/lib/msan/tests/CMakeLists.txt
index e0771dd5a1a7e1..3ddae6d08b7f67 100644
--- a/compiler-rt/lib/msan/tests/CMakeLists.txt
+++ b/compiler-rt/lib/msan/tests/CMakeLists.txt
@@ -69,7 +69,7 @@ macro(msan_compile obj_list source arch kind cflags)
sanitizer_test_compile(
${obj_list} ${source} ${arch}
KIND ${kind}
- COMPILE_DEPS ${MSAN_UNITTEST_HEADERS} libcxx_msan_${arch}-build
+ COMPILE_DEPS ${MSAN_UNITTEST_HEADERS} libcxx_msan_${arch}-install
DEPS msan
CFLAGS -isystem ${MSAN_LIBCXX_DIR}/../include/c++/v1
${MSAN_UNITTEST_INSTRUMENTED_CFLAGS} ${cflags}
@@ -117,10 +117,10 @@ macro(add_msan_tests_for_arch arch kind cflags)
DEPS ${MSAN_INST_LOADABLE_OBJECTS})
set(MSAN_TEST_OBJECTS ${MSAN_INST_TEST_OBJECTS} ${MSAN_INST_GTEST})
- set(MSAN_TEST_DEPS ${MSAN_TEST_OBJECTS} libcxx_msan_${arch}-build
+ set(MSAN_TEST_DEPS ${MSAN_TEST_OBJECTS} libcxx_msan_${arch}-install
${MSAN_LOADABLE_SO}
- "${MSAN_LIBCXX_DIR}/libc++.a" "${MSAN_LIBCXX_DIR}/libc++abi.a")
- list(APPEND MSAN_TEST_DEPS msan libcxx_msan_${arch}-build)
+ "${MSAN_LIBCXX_DIR}/libc++.a" "${MSAN_LIBCXX_DIR}/libc++abi.a")
+ list(APPEND MSAN_TEST_DEPS msan libcxx_msan_${arch}-install)
get_target_flags_for_arch(${arch} TARGET_LINK_FLAGS)
add_compiler_rt_test(MsanUnitTests "Msan-${arch}${kind}-Test" ${arch}
OBJECTS ${MSAN_TEST_OBJECTS} "${MSAN_LIBCXX_DIR}/libc++.a" "${MSAN_LIBCXX_DIR}/libc++abi.a"
diff --git a/compiler-rt/lib/tsan/CMakeLists.txt b/compiler-rt/lib/tsan/CMakeLists.txt
index e192506f0c9358..f7e2b5b6a35631 100644
--- a/compiler-rt/lib/tsan/CMakeLists.txt
+++ b/compiler-rt/lib/tsan/CMakeLists.txt
@@ -31,7 +31,7 @@ if(COMPILER_RT_LIBCXX_PATH AND
DEPS ${TSAN_RUNTIME_LIBRARIES}
CFLAGS ${TARGET_CFLAGS} -fsanitize=thread
USE_TOOLCHAIN)
- list(APPEND libcxx_tsan_deps libcxx_tsan_${arch}-build)
+ list(APPEND libcxx_tsan_deps libcxx_tsan_${arch}-install)
endforeach()
add_custom_target(libcxx_tsan DEPENDS ${libcxx_tsan_deps})
|
There was a problem hiding this comment.
I'm not very familiar with the compiler-rt build, but I love this in principle.
We shouldn't be making assumptions about the internal state of a project's build outside of that project itself. The "public" interface of libc++ is the installation step so that's the only thing we should be using from compiler-rt.
Thanks for this cleanup!
arichardson
deleted the
users/arichardson/spr/compiler-rt-use-installed-libcabi-for-tests-instead-of-build-tree
branch
| SOURCE_DIR ${LLVM_MAIN_SRC_DIR}/../runtimes | ||
| BINARY_DIR ${prefix} | ||
| BINARY_DIR ${prefix}/build |
There was a problem hiding this comment.
Was this an expected change? If so, doesn't that mean the -isystem path in fuzzer's CMakeLists.txt should include the /build/ prefix?
There was a problem hiding this comment.
No, the install directory is ${prefix}, so it uses the installed headers now instead of the build tree.
There was a problem hiding this comment.
So adding the /build suffix is intended then? We're seeing issues where certain C++ headers aren't found and it looks like someone else has commented the same as well.
There was a problem hiding this comment.
Yes this is intentional - and the dependency for RTFuzzer is still there so I am not sure why it is failing for you:
target_compile_options(RTfuzzer.${arch} PRIVATE -isystem ${LIBCXX_${arch}_PREFIX}/include/c++/v1)
add_dependencies(RTfuzzer.${arch} libcxx_fuzzer_${arch}-install)
There was a problem hiding this comment.
Yes I saw that. In our case, the headers were defined under ${LIBCXX_${arch}_PREFIX}/build/include/c++/v1, hence my suspicion on /build whether that was intentional. But perhaps, it might a configuration issue on our end. For now, I've disabled building fuzzer since it's not needed for our internal workloads.
|
Any idea if there is a possibility/risk that this patch introduces some race condition so some files might be used before they're installed? The reason I'm asking is that we now sometimes see errors like |
There shouldn't be, but I'll take a look if there are some missing dependencies. |
@mikaelholmen @thevinster Do you have a build log/cmake command that shows this issue? I am unable to reproduce this and CMake appears to have all the required dependencies. |
|
I just looked at the generated build.ninja and there is definitely a dependency from |
This was needed before #115077 since the compiler-rt test build made assumptions about the build layout of libc++ and libc++abi, but now they link against a local installation of these libraries so we no longer need this workaround. The last attempt at landing this was reverted due to buildbot failures which should be fixed by llvm/llvm-zorg#299. Pull Request: #115379
Here is the scenario (from @mikaelholmen): So the install step is installing cstdint in But then when building FuzzerExtraCountersWindows.cpp we find When searching the build dir I find: But I guess the goal was to use a path to the just installed version here (from the DESTDIR)? |
Or should the isystem paths setup by compiler-rt/lib/fuzzer/CMakeLists.txt use |
Btw, it doesn't seem to be enough to change the isystem paths. Then it ends up with |
Maybe it is more correct to do: |
|
We should not be using the headers from the build dir, this will be removed in #115380 . It sounds to me that the problem is DESTDIR being set while building since that affects the destination of the local libc++ installation. I'll see if I can make the ExternalProject_add() call ignore DESTDIR. |
|
As can be seen from your log: /tmp/tmp.3hBN2EZpSo/repo/llvm/build-all/runtimes/runtimes-x86_64-unknown-linux-gnu-bins/compiler-rt/lib/fuzzer/libcxx_fuzzer_x86_64/include/c++/v1/cstdint |
|
Sounds like I need to override the install command to clear DESTDIR: https://gitlab.kitware.com/cmake/cmake/-/issues/18165 |
If DESTDIR is set in the environment during the build/test stage, the local libc++ installation will be installed under DESTDIR instead of being in the build directory. See #115077 (comment) and https://gitlab.kitware.com/cmake/cmake/-/issues/18165. Pull Request: #115525
This was needed before llvm#115077 since the compiler-rt test build made assumptions about the build layout of libc++ and libc++abi, but now they link against a local installation of these libraries so we no longer need this workaround. The last attempt at landing this was reverted due to buildbot failures which should be fixed by llvm/llvm-zorg#299. Pull Request: llvm#115379
If DESTDIR is set in the environment during the build/test stage, the local libc++ installation will be installed under DESTDIR instead of being in the build directory. See llvm#115077 (comment) and https://gitlab.kitware.com/cmake/cmake/-/issues/18165. Pull Request: llvm#115525
Using the build tree is somewhat fragile since the layout is not
guaranteed to be stable and means the tests are tightly coupled to the
libc++/libc++abi build tree layout. Instead update the ExternalProject
to install the library and headers and do not add the build tree to
the include/linker flags.