[compiler-rt] Allow running tests without installing first #83088
Conversation
Created using spr 1.3.4 [skip ci]
Created using spr 1.3.4
|
@llvm/pr-subscribers-xray @llvm/pr-subscribers-compiler-rt-sanitizer Author: Alexander Richardson (arichardson) ChangesCurrently, the testsuite uses the default runtimes path to find the This mostly fixes check-all in my configuration: I am still seeing two test failures due to an msan error inside Full diff: https://github.com/llvm/llvm-project/pull/83088.diff 5 Files Affected:
diff --git a/compiler-rt/CMakeLists.txt b/compiler-rt/CMakeLists.txt
index bbb4e8d7c333e4..68ed37498587c6 100644
--- a/compiler-rt/CMakeLists.txt
+++ b/compiler-rt/CMakeLists.txt
@@ -571,6 +571,30 @@ string(APPEND COMPILER_RT_TEST_COMPILER_CFLAGS " ${stdlib_flag}")
string(REPLACE " " ";" COMPILER_RT_UNITTEST_CFLAGS "${COMPILER_RT_TEST_COMPILER_CFLAGS}")
set(COMPILER_RT_UNITTEST_LINK_FLAGS ${COMPILER_RT_UNITTEST_CFLAGS})
+option(COMPILER_RT_TEST_STANDALONE_BUILD_LIBS
+ "When set to ON and testing in a standalone build, test the runtime \
+ libraries built by this standalone build rather than the runtime libraries \
+ shipped with the compiler (used for testing). When set to OFF and testing \
+ in a standalone build, test the runtime libraries shipped with the compiler \
+ (used for testing). This option has no effect if the compiler and this \
+ build are configured to use the same runtime library path."
+ ON)
+if (COMPILER_RT_TEST_STANDALONE_BUILD_LIBS)
+ # Ensure that the unit tests can find the sanitizer headers prior to installation.
+ list(APPEND COMPILER_RT_UNITTEST_CFLAGS "-I${CMAKE_CURRENT_LIST_DIR}/include")
+ # Ensure that unit tests link against the just-built runtime libraries instead
+ # of the ones bundled with the compiler by overriding the resource directory.
+ #
+ if ("${COMPILER_RT_TEST_COMPILER_ID}" MATCHES "Clang")
+ list(APPEND COMPILER_RT_UNITTEST_LINK_FLAGS "-resource-dir=${CMAKE_CURRENT_BINARY_DIR}")
+ endif()
+ get_compiler_rt_output_dir(${COMPILER_RT_DEFAULT_TARGET_ARCH} output_dir)
+ list(APPEND COMPILER_RT_UNITTEST_LINK_FLAGS "-Wl,-rpath,${output_dir}")
+endif()
+message(WARNING "COMPILER_RT_UNITTEST_LINK_FLAGS=${COMPILER_RT_UNITTEST_LINK_FLAGS}, COMPILER_RT_TEST_STANDALONE_BUILD_LIBS=${COMPILER_RT_TEST_STANDALONE_BUILD_LIBS} COMPILER_RT_TEST_COMPILER_ID=${COMPILER_RT_TEST_COMPILER_ID}")
+
+
+
if(COMPILER_RT_USE_LLVM_UNWINDER)
# We're linking directly against the libunwind that we're building so don't
# try to link in the toolchain's default libunwind which may be missing.
diff --git a/compiler-rt/test/CMakeLists.txt b/compiler-rt/test/CMakeLists.txt
index c186be1e44fd9a..edc007aaf477a7 100644
--- a/compiler-rt/test/CMakeLists.txt
+++ b/compiler-rt/test/CMakeLists.txt
@@ -1,14 +1,6 @@
# Needed for lit support in standalone builds.
include(AddLLVM)
-option(COMPILER_RT_TEST_STANDALONE_BUILD_LIBS
- "When set to ON and testing in a standalone build, test the runtime \
- libraries built by this standalone build rather than the runtime libraries \
- shipped with the compiler (used for testing). When set to OFF and testing \
- in a standalone build, test the runtime libraries shipped with the compiler \
- (used for testing). This option has no effect if the compiler and this \
- build are configured to use the same runtime library path."
- ON)
pythonize_bool(COMPILER_RT_TEST_STANDALONE_BUILD_LIBS)
pythonize_bool(LLVM_ENABLE_EXPENSIVE_CHECKS)
diff --git a/compiler-rt/test/fuzzer/lit.cfg.py b/compiler-rt/test/fuzzer/lit.cfg.py
index 4e203236b16708..29fd45dbc02fa4 100644
--- a/compiler-rt/test/fuzzer/lit.cfg.py
+++ b/compiler-rt/test/fuzzer/lit.cfg.py
@@ -101,6 +101,7 @@ def generate_compiler_cmd(is_cpp=True, fuzzer_enabled=True, msan_enabled=False):
return " ".join(
[
compiler_cmd,
+ config.target_cflags,
std_cmd,
"-O2 -gline-tables-only",
sanitizers_cmd,
diff --git a/compiler-rt/test/lit.common.cfg.py b/compiler-rt/test/lit.common.cfg.py
index 113777b0ea8a19..6753389ec5bf27 100644
--- a/compiler-rt/test/lit.common.cfg.py
+++ b/compiler-rt/test/lit.common.cfg.py
@@ -14,6 +14,26 @@
import lit.util
+def get_path_from_clang(args, allow_failure):
+ clang_cmd = [
+ config.clang.strip(),
+ f"--target={config.target_triple}",
+ *args,
+ ]
+ path = None
+ try:
+ result = subprocess.run(
+ clang_cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, check=True
+ )
+ path = result.stdout.decode().strip()
+ except subprocess.CalledProcessError as e:
+ msg = f"Failed to run {clang_cmd}\nrc:{e.returncode}\nstdout:{e.stdout}\ne.stderr{e.stderr}"
+ if allow_failure:
+ lit_config.warning(msg)
+ else:
+ lit_config.fatal(msg)
+ return path, clang_cmd
+
def find_compiler_libdir():
"""
Returns the path to library resource directory used
@@ -26,26 +46,6 @@ def find_compiler_libdir():
# TODO: Support other compilers.
return None
- def get_path_from_clang(args, allow_failure):
- clang_cmd = [
- config.clang.strip(),
- f"--target={config.target_triple}",
- ]
- clang_cmd.extend(args)
- path = None
- try:
- result = subprocess.run(
- clang_cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, check=True
- )
- path = result.stdout.decode().strip()
- except subprocess.CalledProcessError as e:
- msg = f"Failed to run {clang_cmd}\nrc:{e.returncode}\nstdout:{e.stdout}\ne.stderr{e.stderr}"
- if allow_failure:
- lit_config.warning(msg)
- else:
- lit_config.fatal(msg)
- return path, clang_cmd
-
# Try using `-print-runtime-dir`. This is only supported by very new versions of Clang.
# so allow failure here.
runtime_dir, clang_cmd = get_path_from_clang(
@@ -172,6 +172,20 @@ def push_dynamic_library_lookup_path(config, new_path):
# doesn't match config.compiler_rt_libdir then it means we might be testing the
# compiler's own runtime libraries rather than the ones we just built.
# Warn about about this and handle appropriately.
+if config.test_standalone_build_libs:
+ if config.compiler_id == "Clang":
+ # Ensure that we use the just-built libraries when linking by overriding
+ # the Clang resource directory. However, this also means that we can no
+ # longer find the builtin headers from that path, so we explicitly add
+ # the builtin headers as an include path.
+ resource_dir, _ = get_path_from_clang(
+ shlex.split(config.target_cflags) + ["-print-resource-dir"], allow_failure=False
+ )
+ config.target_cflags += f" -nobuiltininc"
+ config.target_cflags += f" -I{config.compiler_rt_src_root}/include"
+ config.target_cflags += f" -idirafter {resource_dir}/include"
+ config.target_cflags += f" -resource-dir={config.compiler_rt_obj_root}"
+ config.target_cflags += f" -Wl,--rpath={config.compiler_rt_libdir}"
compiler_libdir = find_compiler_libdir()
if compiler_libdir:
compiler_rt_libdir_real = os.path.realpath(config.compiler_rt_libdir)
@@ -182,7 +196,7 @@ def push_dynamic_library_lookup_path(config, new_path):
f'compiler-rt libdir: "{compiler_rt_libdir_real}"'
)
if config.test_standalone_build_libs:
- # Use just built runtime libraries, i.e. the the libraries this built just built.
+ # Use just built runtime libraries, i.e. the libraries this build just built.
if not config.test_suite_supports_overriding_runtime_lib_path:
# Test suite doesn't support this configuration.
# TODO(dliew): This should be an error but it seems several bots are
diff --git a/compiler-rt/test/safestack/lit.cfg.py b/compiler-rt/test/safestack/lit.cfg.py
index adf27a0d7e5eab..bdc316e2f6bc74 100644
--- a/compiler-rt/test/safestack/lit.cfg.py
+++ b/compiler-rt/test/safestack/lit.cfg.py
@@ -13,10 +13,10 @@
# Add clang substitutions.
config.substitutions.append(
- ("%clang_nosafestack ", config.clang + " -O0 -fno-sanitize=safe-stack ")
+ ("%clang_nosafestack ", config.clang + config.target_cflags + " -O0 -fno-sanitize=safe-stack ")
)
config.substitutions.append(
- ("%clang_safestack ", config.clang + " -O0 -fsanitize=safe-stack ")
+ ("%clang_safestack ", config.clang + config.target_cflags + " -O0 -fsanitize=safe-stack ")
)
if config.lto_supported:
|
arichardson
requested review from
petrhosek,
MaskRay,
vitalybuka,
delcypher and
yln
compiler-rt/test/lit.common.cfg.py
Outdated
| # Ensure that we use the just-built libraries when linking by overriding | ||
| # the Clang resource directory. However, this also means that we can no | ||
| # longer find the builtin headers from that path, so we explicitly add | ||
| # the builtin headers as an include path. |
There was a problem hiding this comment.
Sorry. It's been a while since I looked at this code. If you use a different resource directory why can't the compiler use the header files in that resource directory?
There was a problem hiding this comment.
I'm overriding the resource directory to point at the compiler-rt build directory. Unlike the clang install dir it won't contain the builtin headers (e.g. stdint.h, stddef.h, etc.). I could copy those files, but using this approach is less error prone.
There was a problem hiding this comment.
Clarified comment, should hopefully address this.
compiler-rt/test/lit.common.cfg.py
Outdated
| config.target_cflags += f" -I{config.compiler_rt_src_root}/include" | ||
| config.target_cflags += f" -idirafter {resource_dir}/include" | ||
| config.target_cflags += f" -resource-dir={config.compiler_rt_obj_root}" | ||
| config.target_cflags += f" -Wl,--rpath={config.compiler_rt_libdir}" |
There was a problem hiding this comment.
The flags below create kind of a odd mix
- Some external clang that has its own independent resource directory.
- The external clang is being forced to use a different resource directory than what it was shipped with
- The external clang is being forced to use a different set of headers than what it was shipped with
I'm not sure that combination is guaranteed to work given that compiler-rt can be tightly coupled with clang.
If I've read this code correctly the change you've made is on the common path (because COMPILER_RT_TEST_STANDALONE_BUILD_LIBS is ON by default) but AFAIK your change was never needed before because normally you would build compiler-rt with a just built clang which I think should mean there's no difference between what -print-resource-dir reports and config.compiler_rt_obj_root. Or have I misunderstood something?
There was a problem hiding this comment.
@arichardson To be clear. I'm not necessarily against your change but I'm a bit concerned about your change being on the default path given that it's not normally necessary.
There was a problem hiding this comment.
The flags below create kind of a odd mix
- Some external clang that has its own independent resource directory.
In my case it is actually a clang built from the same monorepo commit hash, but it should also work with a system-provided clang.
- The external clang is being forced to use a different resource directory than what it was shipped with
The intention of this change is to test the libraries that we actually built just now rather than whatever happened to be installed to clang's resource directory. There is absolutely no guarantee that those match.
- The external clang is being forced to use a different set of headers than what it was shipped with
The whole point of this extra -idirafter is to make sure that the external clang uses its set of headers (the resource_dir variable points to the non-overriden directory).
I'm not sure that combination is guaranteed to work given that compiler-rt can be tightly coupled with clang.
If I've read this code correctly the change you've made is on the common path (because
COMPILER_RT_TEST_STANDALONE_BUILD_LIBSisONby default) but AFAIK your change was never needed before because normally you would build compiler-rt with a just built clang which I think should mean there's no difference between what-print-resource-dirreports andconfig.compiler_rt_obj_root. Or have I misunderstood something?
The resource directories will only match if you build everything in on go (I believe with LLVM_ENABLE_PROJECTS=clang;compiler-rt). However, I am installing clang first and then building only compiler-rt (using the cmake invocation listed above), so the clang resource directory points to where clang was installed.
I order to not affect the all-in-one build I will update the check to not add flags if the resource-dir matches the compiler_rt_objdir. Hopefully that addresses your concerns.
There was a problem hiding this comment.
Logic has been updated now to only add these flags when using the non-LLVM_ENABLE_PROJECTS=clang;compiler-rt build.
I'm trying to use getcord/spr for stacked pull requests and it appears this is what it does to the commit messages when uploading them. |
Created using spr 1.3.4 [skip ci]
Created using spr 1.3.4
Note: since the base branch is not |
There was a problem hiding this comment.
I have a regular clang build at /tmp/Rel. If I create a standalone compiler-rt build at /tmp/out/crt, I see that with this PR, clang invoked by llvm-lit will get -resource-dir=/tmp/out/crt -Wl,-rpath,/tmp/out/crt/lib/linux, which utilizes include/sanitizer/*_interface.h (used by certain asan tests) and runtime libraries.
#include <sanitizer/allocator_interface.h>
#include "sanitizer/asan_interface.h"
Thanks, trying to get my head around using spr, but I'm a bit scared of |
| # going to be in the current compiler-rt build directory). | ||
| # We also tell the linker to add an RPATH entry for the local library | ||
| # directory so that the just-built shared libraries are used. | ||
| config.target_cflags += f" -nobuiltininc" |
There was a problem hiding this comment.
I wondered whether target_cflags can be made to a list instead of space-separated str, but I think it doesn't really improve.
There was a problem hiding this comment.
I agree that this would be cleaner, I will submit an independent cleanup PR.
Created using spr 1.3.4 [skip ci]
Created using spr 1.3.4
arichardson
changed the base branch from
users/arichardson/spr/main.compiler-rt-allow-running-tests-without-installing-first
to
main
Created using spr 1.3.4 [skip ci]
llvmbot
added
compiler-rt:asan
Since this standalone build configuration uses the runtime libraries that are being built just now, we need to ensure that e.g. the TSan unit tests depend on the tsan runtime library. Also fix TSAN_DEPS being overridden to not include the tsan runtime (commit .....). This change fixes a build race seen in the CI checks for TsanRtlTest-x86_64-Test in llvm#83088. Pull Request: llvm#83650
With llvm#83088, we now need the runtimes to be built before running test if COMPILER_RT_TEST_STANDALONE_BUILD_LIBS is true, since otherwise we get failures running `ninja check-all` such as the following: ``` /usr/bin/ld: cannot find .../compiler-rt/cmake-build-all-sanitizers/lib/linux/libclang_rt.fuzzer-x86_64.a: No such file or directory /usr/bin/ld: cannot find .../compiler-rt/cmake-build-all-sanitizers/lib/linux/libclang_rt.xray-x86_64.a: No such file or directory /usr/bin/ld: cannot find .../compiler-rt/cmake-build-all-sanitizers/lib/linux/libclang_rt.xray-basic-x86_64.a: No such file or directory /usr/bin/ld: cannot find .../compiler-rt/cmake-build-all-sanitizers/lib/linux/libclang_rt.xray-fdr-x86_64.a: No such file or directory ``` This is a follow-up to 058e9b0 which started removing these checks and it should make it easier to stop forcing COMPILER_RT_STANDALONE_BUILD for runtimes builds in the future. Pull Request: llvm#83651
Currently, the testsuite uses the default runtimes path to find the
runtimes libraries which may or may not match the just-built runtimes.
This change uses the -resource-dir flag for clang whenever
COMPILER_RT_TEST_STANDALONE_BUILD_LIBS is set to ensure that we are
actually testing the currently built libraries rather than the ones
bundled with ${COMPILER_RT_TEST_COMPILER}.
The existing logic works fine when clang and compiler-rt share the same
build directory -DLLVM_ENABLE_PROJECTS=clang;compiler-rt, but when building
compiler-rt separately we need to tell the compiler used for the tests
where it can find the just-built libraries.
This mostly fixes check-all in my configuration:
```
cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo -G Ninja
-DCMAKE_C_COMPILER=$HOME/output/upstream-llvm/bin/clang
-DCMAKE_CXX_COMPILER=$HOME/output/upstream-llvm/bin/clang++
-DCOMPILER_RT_INCLUDE_TESTS=ON
-DLLVM_EXTERNAL_LIT=$HOME/build/upstream-llvm-project-build/bin/llvm-lit
-DLLVM_CMAKE_DIR=$HOME/output/upstream-llvm
-DCOMPILER_RT_DEBUG=OFF
-S $HOME/src/upstream-llvm-project/compiler-rt
-B $HOME/src/upstream-llvm-project/compiler-rt/cmake-build-all-sanitizers
```
I am still seeing two test failures due to an msan error inside
__gxx_personality_v0, but that is most likely due to an uninstrumented
`/lib/x86_64-linux-gnu/libgcc_s.so.1` being used by these tests.
Pull Request: llvm#83088
There was a problem hiding this comment.
Thanks for addressing my feedback. I have a minor nit that you can address if you feel its worth it. When landing please make sure you squash your commits into a single commit and rewrite the commit message to be something that conforms to LLVM's style for commit messages.
Thanks, I'll make sure to leave enough delays for buildbots to shout at me when landing these changes :) |
With #83088, we now need the runtimes to be built before running test if COMPILER_RT_TEST_STANDALONE_BUILD_LIBS is true, since otherwise we get failures running `ninja check-all` such as the following: ``` /usr/bin/ld: cannot find .../compiler-rt/cmake-build-all-sanitizers/lib/linux/libclang_rt.fuzzer-x86_64.a: No such file or directory /usr/bin/ld: cannot find .../compiler-rt/cmake-build-all-sanitizers/lib/linux/libclang_rt.xray-x86_64.a: No such file or directory /usr/bin/ld: cannot find .../compiler-rt/cmake-build-all-sanitizers/lib/linux/libclang_rt.xray-basic-x86_64.a: No such file or directory /usr/bin/ld: cannot find .../compiler-rt/cmake-build-all-sanitizers/lib/linux/libclang_rt.xray-fdr-x86_64.a: No such file or directory ``` This is a follow-up to 058e9b0 which started removing these checks and it should make it easier to stop forcing COMPILER_RT_STANDALONE_BUILD for runtimes builds in the future. Reviewed By: vitalybuka Pull Request: #83651
Since this standalone build configuration uses the runtime libraries that are being built just now, we need to ensure that e.g. the TSan unit tests depend on the tsan runtime library. Also fix TSAN_DEPS being overridden to not include the tsan runtime (commit .....). This change fixes a build race seen in the CI checks for TsanRtlTest-x86_64-Test in #83088. Reviewed By: vitalybuka Pull Request: #83650
Created using spr 1.3.6-beta.1 [skip ci]
Created using spr 1.3.6-beta.1
With llvm#83088, we now need the runtimes to be built before running test if COMPILER_RT_TEST_STANDALONE_BUILD_LIBS is true, since otherwise we get failures running `ninja check-all` such as the following: ``` /usr/bin/ld: cannot find .../compiler-rt/cmake-build-all-sanitizers/lib/linux/libclang_rt.fuzzer-x86_64.a: No such file or directory /usr/bin/ld: cannot find .../compiler-rt/cmake-build-all-sanitizers/lib/linux/libclang_rt.xray-x86_64.a: No such file or directory /usr/bin/ld: cannot find .../compiler-rt/cmake-build-all-sanitizers/lib/linux/libclang_rt.xray-basic-x86_64.a: No such file or directory /usr/bin/ld: cannot find .../compiler-rt/cmake-build-all-sanitizers/lib/linux/libclang_rt.xray-fdr-x86_64.a: No such file or directory ``` This is a follow-up to 058e9b0 which started removing these checks and it should make it easier to stop forcing COMPILER_RT_STANDALONE_BUILD for runtimes builds in the future. Reviewed By: vitalybuka Pull Request: llvm#83651
Since this standalone build configuration uses the runtime libraries that are being built just now, we need to ensure that e.g. the TSan unit tests depend on the tsan runtime library. Also fix TSAN_DEPS being overridden to not include the tsan runtime (commit .....). This change fixes a build race seen in the CI checks for TsanRtlTest-x86_64-Test in llvm#83088. Reviewed By: vitalybuka Pull Request: llvm#83650
Created using spr 1.3.6-beta.1 [skip ci]
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
arichardson
changed the base branch from
users/arichardson/spr/main.compiler-rt-allow-running-tests-without-installing-first-3
to
main
arichardson
deleted the
users/arichardson/spr/compiler-rt-allow-running-tests-without-installing-first
branch
|
Hi. This may have caused a breakage on one of our buildbots https://lab.llvm.org/buildbot/#/builders/165/builds/51837 |
|
We are also seeing similar problems with our internal builder after this commit: |
|
Hey @arichardson I had to just revert this patch with 2084a07 because it was breaking the buildbot as mentioned by @jplehr earlier along with our local builds. In both cases, the error was the same : Let me or @jplehr know if we can assist you in anyway. |
| config.target_cflags += f" -I{config.compiler_rt_src_root}/include" | ||
| config.target_cflags += f" -idirafter {test_cc_resource_dir}/include" | ||
| config.target_cflags += f" -resource-dir={config.compiler_rt_output_dir}" | ||
| config.target_cflags += f" -Wl,-rpath,{config.compiler_rt_libdir}" |
There was a problem hiding this comment.
In addition to the fix for Windows that was made in 10b1864, this bit here also would need a similar condition - otherwise testing fails with the same error, see e.g. https://github.com/mstorsjo/llvm-mingw/actions/runs/8592846078/job/23549755414.
I see that this whole PR was reverted later, but when preparing for a reland, make sure to conditionalize this case of -Wl,-rpath as well.
There was a problem hiding this comment.
That makes sense. Unfortunately I don't have a way of testing this on Windows, and I was hoping the pre-commit CI would have caught it but I guess that does not include mingw.
That sounds like a either a misconfigured build-bot or a bug in compiler-rt tests using the host compiler instead of the test compiler for this target? This flag will only be added if COMPILER_RT_TEST_COMPILER_ID==Clang, so it definitely should not be added for GCC. |
It was working before your change, and it is a similar failure as what @jplehr's buildbot reported (they seem to be using |
|
Build bot still uses '-DLLVM_ENABLE_PROJECTS=clang;lld;clang-tools-extra;compiler-rt' can be updated here https://github.com/llvm/llvm-zorg/blob/3ed3d915a9d4de7967dd20a35a9a266b09cdba86/zorg/buildbot/builders/annotated/hip-build.sh#L82 |
Should be fixed by #88074. This was a pre-existing bug that just happened to be exposed by this change. |
Thanks! I'm also updating our internal builder to use |
Unlike the other compiler-rt unit tests MemProf was not using the `generate_compiler_rt_tests()` helper that ensures the test is compiled using the test compiler (generally the Clang binary built earlier). This was exposed by #83088 because it started adding Clang-specific flags to COMPILER_RT_UNITTEST_CFLAGS if the compiler ID matched "Clang". This change should fix the buildbots that compile compiler-rt using a GCC compiler with LLVM_ENABLE_PROJECTS=compiler-rt. Reviewed By: vitalybuka Pull Request: #88074
Currently, the testsuite uses the default runtimes path to find the
runtimes libraries which may or may not match the just-built runtimes.
This change uses the
-resource-dirflag for clang wheneverCOMPILER_RT_TEST_STANDALONE_BUILD_LIBSis set to ensure that we areactually testing the currently built libraries rather than the ones
bundled with
${COMPILER_RT_TEST_COMPILER}.The existing logic works fine when clang and compiler-rt share the same
build directory ``-DLLVM_ENABLE_PROJECTS=clang;compiler-rt`, but when
building compiler-rt separately we need to tell the compiler used for
the tests where it can find the just-built libraries.
This reduces the fixes check-all failures to one in my configuration: