[clang] Merge gtest binaries into AllClangUnitTests #134196
Conversation
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
|
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? |
|
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: |
|
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 |
|
Yup, seeing this again on #132776 |
|
I hacked together a workaround here: #138613 I'm mostly just guessing remotely, since I don't have a reliable repro, that the final PerformPending... event is rounding into the previous event group that just ended. If the JSON encodes this nesting relationship already, it might be nice to expose that to deflake the test more. |
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. I've noticed that incremental builds leave behind the old, stale gtest binaries, and lit will keep running them. This mostly doesn't matter unless they use shared libraries, which will eventually stop working after successive builds. You can clean up the old test binaries with this command in the build directory: $ find tools/clang/unittests/ -iname '*Tests' -type f | xargs rm ... or you can simply clean the build directory in a more holistic way. --------- Co-authored-by: Petr Hosek <[email protected]>
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 llvm#134196 (comment)
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. I've noticed that incremental builds leave behind the old, stale gtest binaries, and lit will keep running them. This mostly doesn't matter unless they use shared libraries, which will eventually stop working after successive builds. You can clean up the old test binaries with this command in the build directory: $ find tools/clang/unittests/ -iname '*Tests' -type f | xargs rm ... or you can simply clean the build directory in a more holistic way. --------- Co-authored-by: Petr Hosek <[email protected]>
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 llvm#134196 (comment)
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. I've noticed that incremental builds leave behind the old, stale gtest binaries, and lit will keep running them. This mostly doesn't matter unless they use shared libraries, which will eventually stop working after successive builds. You can clean up the old test binaries with this command in the build directory: $ find tools/clang/unittests/ -iname '*Tests' -type f | xargs rm ... or you can simply clean the build directory in a more holistic way. --------- Co-authored-by: Petr Hosek <[email protected]>
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 llvm#134196 (comment)
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 llvm/llvm-project#134196 (comment)
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. I've noticed that incremental builds leave behind the old, stale gtest binaries, and lit will keep running them. This mostly doesn't matter unless they use shared libraries, which will eventually stop working after successive builds. You can clean up the old test binaries with this command in the build directory: $ find tools/clang/unittests/ -iname '*Tests' -type f | xargs rm ... or you can simply clean the build directory in a more holistic way. --------- Co-authored-by: Petr Hosek <[email protected]>
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 llvm#134196 (comment)
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. I've noticed that incremental builds leave behind the old, stale gtest binaries, and lit will keep running them. This mostly doesn't matter unless they use shared libraries, which will eventually stop working after successive builds. You can clean up the old test binaries with this command in the build directory: $ find tools/clang/unittests/ -iname '*Tests' -type f | xargs rm ... or you can simply clean the build directory in a more holistic way. --------- Co-authored-by: Petr Hosek <[email protected]>
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 llvm#134196 (comment)
|
hi folks, this change triggered some other failures at a distance, you can see detailed analysis in #143695 (comment). but tl;dr; LLVM shares some state throughout a process. This isn't something new with this change, unittests were prone/fragile to this already. But this change somewhat exacerbated this situation. As test ordering was only affecting unittests within the same directory, now they're global across all clang unit tests. you can see a repro that I managed to extract at this particular commit (db2315a). hopefully none of these tests should rely on certain aspects of the host environment and be reproducible (but even if not, i think the point is clear): I see that you already addressed some of this dependencies in the commit, but I am not sure if that actually scales. Would it make sense to have a custom main for |
That sounds reasonable. I toyed with the idea of putting the registration into a static initializer, but it raises the question of where you put it. Putting it into an existing test fixture file would work, but it's not nice. Registering targets in main like every other LLVM tool would be the obviously correct thing to do. |
|
Do you or @petrhosek have some free cycles to follow up on that soon? Because as things stand, unit test execution is currently in a very fragile state. Anyone that adds a unittest to clang currently runs the risk of braking something completely unrelated (I do agree that it's the fault of those tests to be fragile in the first place), resulting in lots of time debugging (I spent a good couple hours here). If we can't get a fix in soon, can we consider reverting this until someone has the capacity to land this with a custom main for clang unit tests? |
Addresses feedback in llvm#134196 (comment) Makes the tests less sensitive to target registration from unrelated test fixtures by registering everything up front.
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.