[clang] Merge gtest binaries into AllClangUnitTests #134196
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reduces the size of the clang/unittests build directory by 64% and my overall build dir size by 5%. Static linking is the real driving factor here, but even if the default build configuration used shared libraries, I don't see why we should be building so many unit test binaries. To make the project more approachable for new contributors, I'm attempting to make the build a bit less resource-intensive. Build directory size is a common complaint, and this is low-hanging fruit.
There was a problem hiding this comment.
Pull Request Overview
This PR renames the gtest binary for unit tests to reduce build directory size and overall resource usage.
- Renames a test case from ModuleCacheTest to DriverModuleCacheTest
- Aligns unit test naming with the goal of merging binaries
Files not reviewed (3)
- clang/unittests/CMakeLists.txt: Language not supported
- clang/unittests/Interpreter/CMakeLists.txt: Language not supported
- clang/unittests/Interpreter/ExceptionTests/CMakeLists.txt: Language not supported
Comments suppressed due to low confidence (1)
clang/unittests/Driver/ModuleCacheTest.cpp:20
- [nitpick] The test case name 'DriverModuleCacheTest' is inconsistent with the file name 'ModuleCacheTest.cpp'. Consider renaming either the file or the test case for clarity.
TEST(DriverModuleCacheTest, GetTargetAndMode) {
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen @llvm/pr-subscribers-clang-driver Author: Reid Kleckner (rnk) ChangesThis reduces the size of the clang/unittests build directory by 64% and my overall build dir size by 5%. Static linking is the real driving factor here, but even if the default build configuration used shared libraries, I don't see why we should be building so many unit test binaries. To make the project more approachable for new contributors, I'm attempting to make the build a bit less resource-intensive. Build directory size is a common complaint, and this is low-hanging fruit. Full diff: https://github.com/llvm/llvm-project/pull/134196.diff 4 Files Affected:
diff --git a/clang/unittests/CMakeLists.txt b/clang/unittests/CMakeLists.txt
index f3823ba309420..8d4476761e03e 100644
--- a/clang/unittests/CMakeLists.txt
+++ b/clang/unittests/CMakeLists.txt
@@ -15,11 +15,11 @@ if(CLANG_BUILT_STANDALONE)
endif()
endif()
-# add_clang_unittest(test_name file1.cpp file2.cpp)
+# add_distinct_clang_unittest(test_name file1.cpp file2.cpp)
#
# Will compile the list of files together and link against the clang
# Produces a binary named 'basename(test_name)'.
-function(add_clang_unittest test_name)
+function(add_distinct_clang_unittest test_name)
cmake_parse_arguments(ARG
""
""
@@ -47,6 +47,33 @@ function(add_clang_unittest test_name)
target_link_libraries(${test_name} PRIVATE ${ARG_LINK_LIBS})
endfunction()
+define_property(GLOBAL PROPERTY CLANG_UNITTEST_SRCS)
+define_property(GLOBAL PROPERTY CLANG_UNITTEST_LLVM_COMPONENTS)
+define_property(GLOBAL PROPERTY CLANG_UNITTEST_CLANG_LIBS)
+define_property(GLOBAL PROPERTY CLANG_UNITTEST_LINK_LIBS)
+
+# add_clang_unittest(test_name file1.cpp file2.cpp)
+#
+# Adds unittests to the combined AllClangUnitTests binary. The unittest binary
+# is defined after adding all unittest subdirectories.
+function(add_clang_unittest test_name)
+ cmake_parse_arguments(ARG
+ ""
+ ""
+ "CLANG_LIBS;LINK_LIBS;LLVM_COMPONENTS"
+ ${ARGN})
+
+ file(RELATIVE_PATH src_prefix "${CMAKE_CURRENT_FUNCTION_LIST_DIR}" "${CMAKE_CURRENT_SOURCE_DIR}")
+ set(srcs_prefixed)
+ foreach(src ${ARG_UNPARSED_ARGUMENTS})
+ set(srcs_prefixed ${srcs_prefixed} "${src_prefix}/${src}")
+ endforeach()
+ set_property(GLOBAL APPEND PROPERTY CLANG_UNITTEST_SRCS ${srcs_prefixed})
+ set_property(GLOBAL APPEND PROPERTY CLANG_UNITTEST_CLANG_LIBS ${ARG_CLANG_LIBS})
+ set_property(GLOBAL APPEND PROPERTY CLANG_UNITTEST_LINK_LIBS ${ARG_LINK_LIBS})
+ set_property(GLOBAL APPEND PROPERTY CLANG_UNITTEST_LLVM_COMPONENTS ${ARG_LLVM_COMPONENTS})
+endfunction()
+
add_subdirectory(Basic)
add_subdirectory(Lex)
add_subdirectory(Parse)
@@ -77,3 +104,25 @@ add_subdirectory(Index)
add_subdirectory(InstallAPI)
add_subdirectory(Serialization)
add_subdirectory(Support)
+
+
+# If we're doing a single merged clang unit test binary, add that target after
+# all the previous subdirectories have been processed.
+get_property(SRCS GLOBAL PROPERTY CLANG_UNITTEST_SRCS)
+get_property(CLANG_LIBS GLOBAL PROPERTY CLANG_UNITTEST_CLANG_LIBS)
+get_property(LINK_LIBS GLOBAL PROPERTY CLANG_UNITTEST_LINK_LIBS)
+get_property(LLVM_COMPONENTS GLOBAL PROPERTY CLANG_UNITTEST_LLVM_COMPONENTS)
+add_distinct_clang_unittest(AllClangUnitTests
+ ${SRCS}
+ CLANG_LIBS
+ ${CLANG_LIBS}
+ LINK_LIBS
+ ${LINK_LIBS}
+ LLVM_COMPONENTS
+ ${LLVM_COMPONENTS}
+)
+
+# The Tooling library has some internal headers. Make those work. If we like
+# the merged clang unit test binary, we can udpate the include paths and make
+# this the default.
+include_directories(Tooling)
diff --git a/clang/unittests/Driver/ModuleCacheTest.cpp b/clang/unittests/Driver/ModuleCacheTest.cpp
index 48744415647e6..7aa5047c59bd3 100644
--- a/clang/unittests/Driver/ModuleCacheTest.cpp
+++ b/clang/unittests/Driver/ModuleCacheTest.cpp
@@ -17,7 +17,7 @@ using namespace clang::driver;
namespace {
-TEST(ModuleCacheTest, GetTargetAndMode) {
+TEST(DriverModuleCacheTest, GetTargetAndMode) {
SmallString<128> Buf;
Driver::getDefaultModuleCachePath(Buf);
StringRef Path = Buf;
diff --git a/clang/unittests/Interpreter/CMakeLists.txt b/clang/unittests/Interpreter/CMakeLists.txt
index 9df1a4b03da47..1dda9024075a1 100644
--- a/clang/unittests/Interpreter/CMakeLists.txt
+++ b/clang/unittests/Interpreter/CMakeLists.txt
@@ -1,4 +1,4 @@
-add_clang_unittest(ClangReplInterpreterTests
+add_distinct_clang_unittest(ClangReplInterpreterTests
IncrementalCompilerBuilderTest.cpp
IncrementalProcessingTest.cpp
InterpreterTest.cpp
diff --git a/clang/unittests/Interpreter/ExceptionTests/CMakeLists.txt b/clang/unittests/Interpreter/ExceptionTests/CMakeLists.txt
index eb366a860661c..dfd94d8e6442c 100644
--- a/clang/unittests/Interpreter/ExceptionTests/CMakeLists.txt
+++ b/clang/unittests/Interpreter/ExceptionTests/CMakeLists.txt
@@ -3,7 +3,7 @@
set(LLVM_REQUIRES_EH ON)
set(LLVM_REQUIRES_RTTI ON)
-add_clang_unittest(ClangReplInterpreterExceptionTests
+add_distinct_clang_unittest(ClangReplInterpreterExceptionTests
InterpreterExceptionTest.cpp
EXPORT_SYMBOLS
|
|
@llvm/pr-subscribers-clang Author: Reid Kleckner (rnk) ChangesThis reduces the size of the clang/unittests build directory by 64% and my overall build dir size by 5%. Static linking is the real driving factor here, but even if the default build configuration used shared libraries, I don't see why we should be building so many unit test binaries. To make the project more approachable for new contributors, I'm attempting to make the build a bit less resource-intensive. Build directory size is a common complaint, and this is low-hanging fruit. Full diff: https://github.com/llvm/llvm-project/pull/134196.diff 4 Files Affected:
diff --git a/clang/unittests/CMakeLists.txt b/clang/unittests/CMakeLists.txt
index f3823ba309420..8d4476761e03e 100644
--- a/clang/unittests/CMakeLists.txt
+++ b/clang/unittests/CMakeLists.txt
@@ -15,11 +15,11 @@ if(CLANG_BUILT_STANDALONE)
endif()
endif()
-# add_clang_unittest(test_name file1.cpp file2.cpp)
+# add_distinct_clang_unittest(test_name file1.cpp file2.cpp)
#
# Will compile the list of files together and link against the clang
# Produces a binary named 'basename(test_name)'.
-function(add_clang_unittest test_name)
+function(add_distinct_clang_unittest test_name)
cmake_parse_arguments(ARG
""
""
@@ -47,6 +47,33 @@ function(add_clang_unittest test_name)
target_link_libraries(${test_name} PRIVATE ${ARG_LINK_LIBS})
endfunction()
+define_property(GLOBAL PROPERTY CLANG_UNITTEST_SRCS)
+define_property(GLOBAL PROPERTY CLANG_UNITTEST_LLVM_COMPONENTS)
+define_property(GLOBAL PROPERTY CLANG_UNITTEST_CLANG_LIBS)
+define_property(GLOBAL PROPERTY CLANG_UNITTEST_LINK_LIBS)
+
+# add_clang_unittest(test_name file1.cpp file2.cpp)
+#
+# Adds unittests to the combined AllClangUnitTests binary. The unittest binary
+# is defined after adding all unittest subdirectories.
+function(add_clang_unittest test_name)
+ cmake_parse_arguments(ARG
+ ""
+ ""
+ "CLANG_LIBS;LINK_LIBS;LLVM_COMPONENTS"
+ ${ARGN})
+
+ file(RELATIVE_PATH src_prefix "${CMAKE_CURRENT_FUNCTION_LIST_DIR}" "${CMAKE_CURRENT_SOURCE_DIR}")
+ set(srcs_prefixed)
+ foreach(src ${ARG_UNPARSED_ARGUMENTS})
+ set(srcs_prefixed ${srcs_prefixed} "${src_prefix}/${src}")
+ endforeach()
+ set_property(GLOBAL APPEND PROPERTY CLANG_UNITTEST_SRCS ${srcs_prefixed})
+ set_property(GLOBAL APPEND PROPERTY CLANG_UNITTEST_CLANG_LIBS ${ARG_CLANG_LIBS})
+ set_property(GLOBAL APPEND PROPERTY CLANG_UNITTEST_LINK_LIBS ${ARG_LINK_LIBS})
+ set_property(GLOBAL APPEND PROPERTY CLANG_UNITTEST_LLVM_COMPONENTS ${ARG_LLVM_COMPONENTS})
+endfunction()
+
add_subdirectory(Basic)
add_subdirectory(Lex)
add_subdirectory(Parse)
@@ -77,3 +104,25 @@ add_subdirectory(Index)
add_subdirectory(InstallAPI)
add_subdirectory(Serialization)
add_subdirectory(Support)
+
+
+# If we're doing a single merged clang unit test binary, add that target after
+# all the previous subdirectories have been processed.
+get_property(SRCS GLOBAL PROPERTY CLANG_UNITTEST_SRCS)
+get_property(CLANG_LIBS GLOBAL PROPERTY CLANG_UNITTEST_CLANG_LIBS)
+get_property(LINK_LIBS GLOBAL PROPERTY CLANG_UNITTEST_LINK_LIBS)
+get_property(LLVM_COMPONENTS GLOBAL PROPERTY CLANG_UNITTEST_LLVM_COMPONENTS)
+add_distinct_clang_unittest(AllClangUnitTests
+ ${SRCS}
+ CLANG_LIBS
+ ${CLANG_LIBS}
+ LINK_LIBS
+ ${LINK_LIBS}
+ LLVM_COMPONENTS
+ ${LLVM_COMPONENTS}
+)
+
+# The Tooling library has some internal headers. Make those work. If we like
+# the merged clang unit test binary, we can udpate the include paths and make
+# this the default.
+include_directories(Tooling)
diff --git a/clang/unittests/Driver/ModuleCacheTest.cpp b/clang/unittests/Driver/ModuleCacheTest.cpp
index 48744415647e6..7aa5047c59bd3 100644
--- a/clang/unittests/Driver/ModuleCacheTest.cpp
+++ b/clang/unittests/Driver/ModuleCacheTest.cpp
@@ -17,7 +17,7 @@ using namespace clang::driver;
namespace {
-TEST(ModuleCacheTest, GetTargetAndMode) {
+TEST(DriverModuleCacheTest, GetTargetAndMode) {
SmallString<128> Buf;
Driver::getDefaultModuleCachePath(Buf);
StringRef Path = Buf;
diff --git a/clang/unittests/Interpreter/CMakeLists.txt b/clang/unittests/Interpreter/CMakeLists.txt
index 9df1a4b03da47..1dda9024075a1 100644
--- a/clang/unittests/Interpreter/CMakeLists.txt
+++ b/clang/unittests/Interpreter/CMakeLists.txt
@@ -1,4 +1,4 @@
-add_clang_unittest(ClangReplInterpreterTests
+add_distinct_clang_unittest(ClangReplInterpreterTests
IncrementalCompilerBuilderTest.cpp
IncrementalProcessingTest.cpp
InterpreterTest.cpp
diff --git a/clang/unittests/Interpreter/ExceptionTests/CMakeLists.txt b/clang/unittests/Interpreter/ExceptionTests/CMakeLists.txt
index eb366a860661c..dfd94d8e6442c 100644
--- a/clang/unittests/Interpreter/ExceptionTests/CMakeLists.txt
+++ b/clang/unittests/Interpreter/ExceptionTests/CMakeLists.txt
@@ -3,7 +3,7 @@
set(LLVM_REQUIRES_EH ON)
set(LLVM_REQUIRES_RTTI ON)
-add_clang_unittest(ClangReplInterpreterExceptionTests
+add_distinct_clang_unittest(ClangReplInterpreterExceptionTests
InterpreterExceptionTest.cpp
EXPORT_SYMBOLS
|
petrhosek
reviewed
Apr 3, 2025
Co-authored-by: Petr Hosek <[email protected]>
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
This is ready now, so please take a look. Apologies for all the push notifications. It took a lot of retries to get this to pass the premerge tests due to cmake environmental differences, which I guess shows the value of shared premerge testing. |
|
Ping, WDYT? |
| @@ -304,7 +304,7 @@ getCodeModel(const CodeGenOptions &CodeGenOpts) { | |||
| .Case("kernel", llvm::CodeModel::Kernel) | |||
| .Case("medium", llvm::CodeModel::Medium) | |||
| .Case("large", llvm::CodeModel::Large) | |||
| .Case("default", ~1u) | |||
| .Cases("default", "", ~1u) | |||
There was a problem hiding this comment.
Can you explain why this change is needed?
There was a problem hiding this comment.
Basically, after my change, some Frontend tests now exercise this codepath without setting up a code model, and they hit the assert below. What changed is that now the target they are generating IR for is registered, so this codepath executes, when it didn't before my change. Interpreting "empty" as the default code model here seemed like the most sensible thing to do, rather than adding logic to those tests to explicitly set a code model.
There was a problem hiding this comment.
I was going to ask about this: Why do FrontendTests now have to call llvm::InitializeAllTargetMCs();? They used to not depend on llvm/lib/Target at all. Why is this needed now that that code gets linked into a bigger binary?
jroelofs
approved these changes
Apr 26, 2025
petrhosek
approved these changes
Apr 28, 2025
…, but not the TargetMachine In this case, createTargetMachine may return null, which can happen in some unit tests.
|
If anyone is looking for stories about the value of premerge testing, I was all set to push this tonight, but I saw the linux premerge test failure. I debugged it, and it turns out to be a subtle interaction where if you run Clang codegen with registered targets but no registered TargetMachine classes, then one of our unittests will crash setting the large code model threshold 🙄 . Anyway, I pushed a fix and can land it tomorrow. Thanks for reviewing! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/19202 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/169/builds/10964 Here is the relevant piece of the build log for the reference |
@rnk not sure if you were notified or not, but the test you changed seems to be failing on MacOS. It seems to be consistently failing on my AArch64 MacOS bot, but only once randomly on my x86-64 MacOS bot (https://lab.llvm.org/buildbot/#/builders/23/builds/9847). Can you take a look? |
|
I attempted to repro on my macbook, but it doesn't reproduce. This test doesn't seem high value, and I thin kthe right next step is to disable it to green the bots before debugging it. |
swift-ci
pushed a commit
to swiftlang/llvm-project
that referenced
this pull request
Apr 29, 2025
|
I disabled the failing test in 743c32e. |
|
|
nico
added a commit
that referenced
this pull request
Apr 30, 2025
The CMake build merged most clang unittests into a single binary. The gn build doesn't (yet?) do this, but as part of that change, FrontendTests somehow picked up a dependency on AllTargetsDescs, see #134196 (comment)
FormatTests has many clang dependencies, but after rebuilding, it looks like it's still quite small, so I sent a PR with my file size data: You can see that the Interpreter tests are a much bigger target for optimization. |
|
This patch caused an assertion failure for b8bb1cc on several build bots and my mac (M2, 15.4.1): It also made us lose the Update: |
This was referenced May 1, 2025
|
Are the |
|
@rnk see #137577 (comment) for some of the bots that got the assertion failure.
I meant the assertion failure also disappeared after that commit. |
FWIW, I'm seeing the test failure on Windows x64 with a debug build of Clang's unit tests as of a fresh fetch of The failing test is: and the failure is this assertion: |
Sorry about that! I leaned on the Windows premerge tests to find Windows issues (they did, that was useful), but I'm pretty sure it uses release, not debug, builds. I won't be able to repro any windows build failures until Monday, and it's likely I'll be buried in other stuff. If it's a singular test case, I would disable the test (rename it to add the If you want to go ahead and debug it, the next steps I would try are adding all the various |
|
I've been seeing intermittent failures of AllClangUnitTests/TimeProfilerTest/ConstantEvaluationCxx20 and AllClangUnitTests/TimeProfilerTest/ConstantEvaluationC99 on my MacOS bot, but I haven't had a chance to try and figure out if it might be related to your change or not. ConstantEvaluationCxx20:
First instance of either failing: https://lab.llvm.org/buildbot/#/builders/190/builds/19216 |
|
I've also seen ConstantEvaluationC99 intermittently fail on our mac bots recently |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reduces the size of the clang/unittests build directory by 64% and my overall build dir size by 5%. Static linking is the real driving factor here, but even if the default build configuration used shared libraries, I don't see why we should be building so many unit test binaries.
To make the project more approachable for new contributors, I'm attempting to make the build a bit less resource-intensive. Build directory size is a common complaint, and this is low-hanging fruit.