[Flang-RT][libc] LLVM-independent unittests #164794
Open
+81
−57
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
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Meinersbur
commented
Oct 23, 2025
| CFI_attribute_t attribute, CFI_type_t type, std::size_t elem_len, | ||
| CFI_rank_t rank, const CFI_index_t extents[]) { | ||
| #ifdef VERBOSE | ||
| DumpTestWorld(base_addr, attribute, type, elem_len, rank, extent); |
There was a problem hiding this comment.
Does not even compile (extents)
There was a problem hiding this comment.
I'm a bit under the weather at the moment so I have only read this briefly. My takeaways:
- The impact to the flang-rt tests is minimal.
- If someone did use llvm things in those tests, it would fail at link time (perhaps earlier if the include paths are also not added anymore).
- Doing this for just flang-rt right now is in theory fine, because compiler-rt has done the same thing in its own way.
- There's an impact on cmake used by other projects though, so we need some review from them.
For instance, Clang is built using gcc which uses libstdc++, but the runtimes is built by Clang which can be configured to use libcxx by default.
In our buildbot we attempt to link everything to the libc++ from the installed clang, so in theory we can pass enough flags to fix the issue in other ways (though I never did find the right runes to do it).
Your example though, I see why that cannot be done. Even if we added test specific library choices, those tests would link to a runtime library linked against something else.
Saying that you can't run tests in this scenario, of course we want to avoid that.
I will try this patch with our buildbot config, but expect it to be about a week before I get time to do so.
flang-rt/unittests/CMakeLists.txt
Outdated
| if (NOT TARGET llvm_gtest) | ||
| if (NOT TARGET default_gtest) | ||
| message(WARNING "Flang-RT unittests disabled due to GTest being unavailable; " | ||
| "Try LLVM_INSTALL_GTEST=ON for the LLVM build") |
There was a problem hiding this comment.
I suppose that this warning will change or become unreachable.
There was a problem hiding this comment.
I think it is becoming unreachable, I think. default_gtest is defined by runtimes/CMakeLists.txt whenever LLVM_INCLUDE_TEST is defined. When not defined, this CMakeLists.txt should also not be add_subdirectorie'd.
There was a problem hiding this comment.
Turns out it is controlled by FLANG_RT_INCLUDE_TESTS which can theoretically enabled even with LLVM_INCLUDE_TESTS=OFF. I left the warning and updated the message.
|
Now I realise our build needs a combination of this and the OpenMP fix, but I think the logic here makes sense anyway. |
libc also uses omptest and compiler-rt have different mechanisms (compiler-rt recompiles gtest-all.cpp for every test), so I left it out.
My mental model was the the LLVM_RUNTIME_TARGETS=default build existed for the purpose of ensuring ABI compatibility. In practice it seems too difficult to ensure. My fix #139712 was reverted and #158156 even is trying to remove the default build. It seems easier to not depend on ABI. |
|
@llvm/pr-subscribers-libc Author: Michael Kruse (Meinersbur) ChangesThe LLVM-customized GTest has a dependency on LLVM to support This patch makes the runtimes' GTest independent from LLVMSupport so we do not link any runtimes component with LLVM components. Runtime projects that use GTest unittests:
The current state of this PR tries to reuse https://github.com/llvm/llvm-project/blob/main/third-party/unittest/CMakeLists.txt as much as possible, altough personally I would prefer to make it use "modern CMake" style. A new macro Full diff: https://github.com/llvm/llvm-project/pull/164794.diff 13 Files Affected:
diff --git a/cmake/Modules/AddGTest.cmake b/cmake/Modules/AddGTest.cmake
new file mode 100644
index 0000000000000..d62b5b2c31995
--- /dev/null
+++ b/cmake/Modules/AddGTest.cmake
@@ -0,0 +1,12 @@
+
+function (build_gtest gtest_name)
+ cmake_parse_arguments(ARG "LLVM_SUPPORT" "" "" ${ARGN})
+
+ if (ARG_LLVM_SUPPORT)
+ set(GTEST_USE_LLVM 1)
+ else ()
+ set(GTEST_USE_LLVM 0)
+ endif ()
+ add_subdirectory("${LLVM_THIRD_PARTY_DIR}/unittest" "${CMAKE_BINARY_DIR}/third-party/${gtest_name}_gtest")
+endfunction ()
+
diff --git a/flang-rt/unittests/CMakeLists.txt b/flang-rt/unittests/CMakeLists.txt
index 53cd54dfd215e..f3ea468c5367a 100644
--- a/flang-rt/unittests/CMakeLists.txt
+++ b/flang-rt/unittests/CMakeLists.txt
@@ -22,7 +22,7 @@ if (CMAKE_CROSSCOMPILING)
return ()
endif ()
-if (NOT TARGET llvm_gtest)
+if (NOT TARGET default_gtest)
message(WARNING "Flang-RT unittests disabled due to GTest being unavailable; "
"Try LLVM_INSTALL_GTEST=ON for the LLVM build")
return ()
diff --git a/flang-rt/unittests/Evaluate/ISO-Fortran-binding.cpp b/flang-rt/unittests/Evaluate/ISO-Fortran-binding.cpp
index 8c0a6f29b6967..1a9817cc665de 100644
--- a/flang-rt/unittests/Evaluate/ISO-Fortran-binding.cpp
+++ b/flang-rt/unittests/Evaluate/ISO-Fortran-binding.cpp
@@ -9,7 +9,6 @@
#include "flang-rt/runtime/descriptor.h"
#include "flang/Common/ISO_Fortran_binding_wrapper.h"
#include "flang/Testing/testing.h"
-#include "llvm/Support/raw_ostream.h"
#include <type_traits>
using namespace Fortran::runtime;
@@ -73,26 +72,9 @@ static void AddNoiseToCdesc(CFI_cdesc_t *dv, CFI_rank_t rank) {
}
}
-#ifdef VERBOSE
-static void DumpTestWorld(const void *bAddr, CFI_attribute_t attr,
- CFI_type_t ty, std::size_t eLen, CFI_rank_t rank,
- const CFI_index_t *eAddr) {
- llvm::outs() << " base_addr: ";
- llvm::outs().write_hex(reinterpret_cast<std::intptr_t>(bAddr))
- << " attribute: " << static_cast<int>(attr)
- << " type: " << static_cast<int>(ty) << " elem_len: " << eLen
- << " rank: " << static_cast<int>(rank) << " extent: ";
- llvm::outs().write_hex(reinterpret_cast<std::intptr_t>(eAddr)) << '\n';
- llvm::outs().flush();
-}
-#endif
-
static void check_CFI_establish(CFI_cdesc_t *dv, void *base_addr,
CFI_attribute_t attribute, CFI_type_t type, std::size_t elem_len,
CFI_rank_t rank, const CFI_index_t extents[]) {
-#ifdef VERBOSE
- DumpTestWorld(base_addr, attribute, type, elem_len, rank, extent);
-#endif
// CFI_establish reqs from F2018 section 18.5.5
int retCode{
CFI_establish(dv, base_addr, attribute, type, elem_len, rank, extents)};
@@ -305,9 +287,6 @@ static void check_CFI_allocate(CFI_cdesc_t *dv,
const CFI_type_t type{dv->type};
const void *base_addr{dv->base_addr};
const int version{dv->version};
-#ifdef VERBOSE
- DumpTestWorld(base_addr, attribute, type, elem_len, rank, nullptr);
-#endif
int retCode{CFI_allocate(dv, lower_bounds, upper_bounds, elem_len)};
Descriptor *desc = reinterpret_cast<Descriptor *>(dv);
if (retCode == CFI_SUCCESS) {
diff --git a/flang-rt/unittests/Runtime/AccessTest.cpp b/flang-rt/unittests/Runtime/AccessTest.cpp
index d431d0d19bd61..d44f0ec3ced23 100644
--- a/flang-rt/unittests/Runtime/AccessTest.cpp
+++ b/flang-rt/unittests/Runtime/AccessTest.cpp
@@ -12,8 +12,8 @@
#include "CrashHandlerFixture.h"
#include "gtest/gtest.h"
#include "flang/Runtime/extensions.h"
-#include "llvm/ADT/Twine.h"
+#include <cstring>
#include <fcntl.h>
#include <sys/stat.h>
#include <sys/types.h>
@@ -82,8 +82,9 @@ static const char *temp_directory_path() {
static std::string createTemporaryFile(
const char *name, const AccessType &accessType) {
- std::string path =
- (llvm::Twine{temp_directory_path()} + "/" + addPIDSuffix(name)).str();
+ std::ostringstream pathS;
+ pathS << temp_directory_path() << "/" << addPIDSuffix(name);
+ std::string path = pathS.str();
// O_CREAT | O_EXCL enforces that this file is newly created by this call.
// This feels risky. If we don't have permission to create files in the
diff --git a/flang-rt/unittests/Runtime/CrashHandlerFixture.cpp b/flang-rt/unittests/Runtime/CrashHandlerFixture.cpp
index 8213edd1f9225..34901b5cd2139 100644
--- a/flang-rt/unittests/Runtime/CrashHandlerFixture.cpp
+++ b/flang-rt/unittests/Runtime/CrashHandlerFixture.cpp
@@ -17,12 +17,11 @@
char buffer[1000];
std::vsnprintf(buffer, sizeof buffer, message, ap);
va_end(ap);
- llvm::errs()
- << "Test "
- << ::testing::UnitTest::GetInstance()->current_test_info()->name()
- << " crashed in file "
- << (sourceFile ? sourceFile : "unknown source file") << '(' << sourceLine
- << "): " << buffer << '\n';
+ std::cerr << "Test "
+ << ::testing::UnitTest::GetInstance()->current_test_info()->name()
+ << " crashed in file "
+ << (sourceFile ? sourceFile : "unknown source file") << '('
+ << sourceLine << "): " << buffer << '\n';
std::exit(EXIT_FAILURE);
}
diff --git a/flang-rt/unittests/Runtime/Descriptor.cpp b/flang-rt/unittests/Runtime/Descriptor.cpp
index 3a4a7670fc62e..4a7bb43a492af 100644
--- a/flang-rt/unittests/Runtime/Descriptor.cpp
+++ b/flang-rt/unittests/Runtime/Descriptor.cpp
@@ -32,8 +32,8 @@ TEST(Descriptor, FixedStride) {
extent[0] = 8;
descriptor.Establish(integer, four, data, 1, extent);
ASSERT_EQ(descriptor.rank(), 1);
- ASSERT_EQ(descriptor.Elements(), 8);
- ASSERT_EQ(descriptor.ElementBytes(), four);
+ ASSERT_EQ(descriptor.Elements(), 8u);
+ ASSERT_EQ(descriptor.ElementBytes(), static_cast<unsigned>(four));
ASSERT_EQ(descriptor.GetDimension(0).LowerBound(), 0);
ASSERT_EQ(descriptor.GetDimension(0).ByteStride(), four);
ASSERT_EQ(descriptor.GetDimension(0).Extent(), 8);
diff --git a/flang-rt/unittests/Runtime/ExternalIOTest.cpp b/flang-rt/unittests/Runtime/ExternalIOTest.cpp
index 6c148b1de6f82..6421194f45141 100644
--- a/flang-rt/unittests/Runtime/ExternalIOTest.cpp
+++ b/flang-rt/unittests/Runtime/ExternalIOTest.cpp
@@ -16,7 +16,6 @@
#include "flang/Runtime/io-api.h"
#include "flang/Runtime/main.h"
#include "flang/Runtime/stop.h"
-#include "llvm/Support/raw_ostream.h"
#include <cstring>
#include <string_view>
diff --git a/libc/benchmarks/CMakeLists.txt b/libc/benchmarks/CMakeLists.txt
index 60f522d7d8c65..63051dad8e23b 100644
--- a/libc/benchmarks/CMakeLists.txt
+++ b/libc/benchmarks/CMakeLists.txt
@@ -38,8 +38,8 @@ function(add_libc_benchmark_unittest target_name)
)
target_link_libraries(${target_name}
PRIVATE
- llvm_gtest_main
- llvm_gtest
+ default_gtest_main
+ default_gtest
${LIBC_BENCHMARKS_UNITTEST_DEPENDS}
)
llvm_update_compile_flags(${target_name})
diff --git a/llvm/CMakeLists.txt b/llvm/CMakeLists.txt
index c450ee5a3d72e..19e541122541e 100644
--- a/llvm/CMakeLists.txt
+++ b/llvm/CMakeLists.txt
@@ -1340,9 +1340,10 @@ if( LLVM_INCLUDE_UTILS )
add_subdirectory(utils/mlgo-utils)
add_subdirectory(utils/llvm-test-mustache-spec)
if( LLVM_INCLUDE_TESTS )
- set(LLVM_SUBPROJECT_TITLE "Third-Party/Google Test")
- add_subdirectory(${LLVM_THIRD_PARTY_DIR}/unittest ${CMAKE_CURRENT_BINARY_DIR}/third-party/unittest)
- set(LLVM_SUBPROJECT_TITLE)
+ include(AddGTest)
+ build_gtest(llvm_gtest LLVM_SUPPORT)
+ add_library(default_gtest ALIAS llvm_gtest)
+ add_library(default_gtest_main ALIAS llvm_gtest_main)
endif()
else()
if ( LLVM_INCLUDE_TESTS )
diff --git a/llvm/cmake/modules/AddLLVM.cmake b/llvm/cmake/modules/AddLLVM.cmake
index 80e59a4df2433..45bbbbb6381d1 100644
--- a/llvm/cmake/modules/AddLLVM.cmake
+++ b/llvm/cmake/modules/AddLLVM.cmake
@@ -1797,7 +1797,7 @@ function(add_unittest test_suite test_name)
# libpthreads overrides some standard library symbols, so main
# executable must be linked with it in order to provide consistent
# API for all shared libaries loaded by this executable.
- target_link_libraries(${test_name} PRIVATE llvm_gtest_main llvm_gtest ${LLVM_PTHREAD_LIB})
+ target_link_libraries(${test_name} PRIVATE default_gtest_main default_gtest ${LLVM_PTHREAD_LIB})
add_dependencies(${test_suite} ${test_name})
endfunction()
diff --git a/runtimes/CMakeLists.txt b/runtimes/CMakeLists.txt
index d3280a5867dec..91c95473cb4ea 100644
--- a/runtimes/CMakeLists.txt
+++ b/runtimes/CMakeLists.txt
@@ -242,6 +242,14 @@ endif()
# This can be used to detect whether we're in the runtimes build.
set(LLVM_RUNTIMES_BUILD ON)
+# Make GTest available to all runtimes
+if (LLVM_INCLUDE_TESTS)
+ include(AddGTest)
+ build_gtest(runtimes_gtest)
+ add_library(default_gtest ALIAS runtimes_gtest)
+ add_library(default_gtest_main ALIAS runtimes_gtest_main)
+endif ()
+
foreach(entry ${runtimes})
get_filename_component(projName ${entry} NAME)
diff --git a/third-party/unittest/CMakeLists.txt b/third-party/unittest/CMakeLists.txt
index 3fa885a16ea1e..5242bf30dcc77 100644
--- a/third-party/unittest/CMakeLists.txt
+++ b/third-party/unittest/CMakeLists.txt
@@ -11,6 +11,15 @@
#
# Project-wide settings
+set(LLVM_SUBPROJECT_TITLE "Third-Party/Google Test")
+
+if (GTEST_USE_LLVM)
+ set(GTEST_LLVM_COMPONENTS "Support") # For llvm::raw_ostream
+else ()
+ # Override locally; never install a non-LLVM GTest
+ set(LLVM_INSTALL_GTEST OFF)
+endif ()
+
if(WIN32)
add_definitions(-DGTEST_OS_WINDOWS=1)
endif()
@@ -48,7 +57,7 @@ if (LLVM_INSTALL_GTEST)
set(BUILDTREE_ONLY "")
endif ()
-add_llvm_library(llvm_gtest
+add_llvm_library("${gtest_name}"
googletest/src/gtest-all.cc
googlemock/src/gmock-all.cc
@@ -56,7 +65,7 @@ add_llvm_library(llvm_gtest
${LIBS}
LINK_COMPONENTS
- Support # Depends on llvm::raw_ostream
+ ${GTEST_LLVM_COMPONENTS}
# This is a library meant only for the build tree.
${BUILDTREE_ONLY}
@@ -67,15 +76,15 @@ add_llvm_library(llvm_gtest
# that warning here for any targets that link to gtest.
if(CXX_SUPPORTS_SUGGEST_OVERRIDE_FLAG)
add_definitions("-Wno-suggest-override")
- set_target_properties(llvm_gtest PROPERTIES INTERFACE_COMPILE_OPTIONS "-Wno-suggest-override")
+ set_target_properties("${gtest_name}" PROPERTIES INTERFACE_COMPILE_OPTIONS "-Wno-suggest-override")
endif()
if (NOT LLVM_ENABLE_THREADS)
- target_compile_definitions(llvm_gtest PUBLIC GTEST_HAS_PTHREAD=0)
+ target_compile_definitions("${gtest_name}" PUBLIC GTEST_HAS_PTHREAD=0)
endif ()
# Top-level include directory required for "llvm/Support/raw_os_ostream.h"
-target_include_directories(llvm_gtest
+target_include_directories("${gtest_name}"
PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/googletest/include>
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/googlemock/include>
$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}/>
@@ -89,16 +98,32 @@ target_include_directories(llvm_gtest
# FIXME: Shouldn't this be done for all LLVM libraries? Currently, LLVM uses a
# big giant `include_directories( ${LLVM_INCLUDE_DIR} ${LLVM_MAIN_INCLUDE_DIR})`
# which CMake does not add to the import library.
-target_include_directories(llvm_gtest BEFORE
- PUBLIC $<BUILD_INTERFACE:${LLVM_SOURCE_DIR}/include>
- $<BUILD_INTERFACE:${LLVM_BINARY_DIR}/include>
- )
+if (GTEST_USE_LLVM)
+ target_include_directories("${gtest_name}" BEFORE
+ PUBLIC $<BUILD_INTERFACE:${LLVM_SOURCE_DIR}/include>
+ $<BUILD_INTERFACE:${LLVM_BINARY_DIR}/include>
+ )
+else ()
+ target_compile_definitions("${gtest_name}" PUBLIC GTEST_NO_LLVM_SUPPORT=1)
+endif ()
-add_subdirectory(UnitTestMain)
+# Library that contains main()
+if (GTEST_USE_LLVM)
+ add_subdirectory(UnitTestMain)
+else ()
+ add_llvm_library("${gtest_name}_main"
+ googletest/src/gtest_main.cc
+
+ LINK_LIBS
+ "${gtest_name}"
+
+ ${BUILDTREE_ONLY}
+ )
+endif ()
if (LLVM_INSTALL_GTEST)
- install(DIRECTORY googletest/include/gtest/ DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/llvm-gtest/gtest/" COMPONENT llvm_gtest)
- install(DIRECTORY googlemock/include/gmock/ DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/llvm-gmock/gmock/" COMPONENT llvm_gtest)
+ install(DIRECTORY googletest/include/gtest/ DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/llvm-gtest/gtest/" COMPONENT "${gtest_name}")
+ install(DIRECTORY googlemock/include/gmock/ DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/llvm-gmock/gmock/" COMPONENT "${gtest_name}")
endif()
# When LLVM_LINK_LLVM_DYLIB is enabled, libLLVM.so is added to the interface
@@ -118,5 +143,5 @@ function (gtest_remove_dylib_from_link_interface target)
endif()
endfunction()
-gtest_remove_dylib_from_link_interface(llvm_gtest)
-gtest_remove_dylib_from_link_interface(llvm_gtest_main)
+gtest_remove_dylib_from_link_interface("${gtest_name}")
+gtest_remove_dylib_from_link_interface("${gtest_name}_main")
diff --git a/third-party/unittest/UnitTestMain/CMakeLists.txt b/third-party/unittest/UnitTestMain/CMakeLists.txt
index 729ea7e3fa7e2..225c33c911f7e 100644
--- a/third-party/unittest/UnitTestMain/CMakeLists.txt
+++ b/third-party/unittest/UnitTestMain/CMakeLists.txt
@@ -3,11 +3,11 @@ if (LLVM_INSTALL_GTEST)
set(BUILDTREE_ONLY "")
endif ()
-add_llvm_library(llvm_gtest_main
+add_llvm_library("${gtest_name}_main"
TestMain.cpp
LINK_LIBS
- llvm_gtest
+ "${gtest_name}"
LINK_COMPONENTS
Support # Depends on llvm::cl
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
The LLVM-customized GTest has a dependency on LLVM to support
llvm::raw_ostreamand hence has to link to LLVMSupport. The runtimes use the LLVMSupport from the bootstrapping LLVM build. The problem is that the boostrapping compiler and the runtimes target can diverge in their ABI, even in the runtimes default build. For instance, Clang is built using gcc which uses libstdc++, but the runtimes is built by Clang which can be configured to use libcxx by default. This has caused flang-aarch64-libcxx) to break, and is still broken.This patch makes the runtimes' GTest independent from LLVMSupport so we do not link any runtimes component with LLVM components.
Runtime projects that use GTest unittests:
The current state of this PR tries to reuse https://github.com/llvm/llvm-project/blob/main/third-party/unittest/CMakeLists.txt as much as possible, altough personally I would prefer to make it use "modern CMake" style. A new macro
build_gtestadds a configurable build of GTest, one for LLVM (llvm_gtest, NFCI) and another one for the runtimes (runtimes_gtest). It is not possible to reusellvm_gtestfor both sincellvm_gtestis imported usingfind_package(LLVM)if LLVM_INSTALL_GTEST. An aliasdefault_gtestis used to select between the two.default_gtestcould also be used for openmp which also supports standalone and LLVM_ENABLE_PROJECTS build mode.