Skip to content

[OpenMP][omptest] Improve CMake and address review comments #159416

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[OpenMP][omptest] Improve CMake and address review comments #159416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f9670c
[OpenMP][omptest] Improve CMake and address review comments
mhalk Sep 17, 2025
ca62f9c
fixup! [OpenMP][omptest] Improve CMake and address review comments
mhalk Nov 12, 2025
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 36 additions & 18 deletions openmp/tools/omptest/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ add_library(omptest
)

# Target: ompTest library
# On (implicit) request of GoogleTest, link against the one provided with LLVM.
# On (implicit) request of GoogleTest, embed the sources provided with LLVM.
if ((NOT LIBOMPTEST_BUILD_STANDALONE) OR LIBOMPTEST_BUILD_UNITTESTS)
# Check if standalone build was requested together with unittests
if (LIBOMPTEST_BUILD_STANDALONE)
Expand All @@ -65,26 +65,44 @@ if ((NOT LIBOMPTEST_BUILD_STANDALONE) OR LIBOMPTEST_BUILD_UNITTESTS)
set(LIBOMPTEST_BUILD_STANDALONE OFF)
endif()

# Add dependency llvm_gtest; emits error if unavailable.
add_dependencies(omptest llvm_gtest)

# Link llvm_gtest as whole-archive to expose required symbols
set(GTEST_LINK_CMD "-Wl,--whole-archive" llvm_gtest
"-Wl,--no-whole-archive" LLVMSupport)

# Add GoogleTest-based header
target_sources(omptest PRIVATE ./include/OmptTesterGoogleTest.h)
# Set and check GTest's source directory.
set(OMPTEST_GTEST_PATH ${LLVM_THIRD_PARTY_DIR}/unittest/googletest)
if(NOT EXISTS "${OMPTEST_GTEST_PATH}/src/gtest-all.cc")
message(FATAL_ERROR "Missing gtest-all.cc under ${OMPTEST_GTEST_PATH}. "
"Make sure LLVM_THIRD_PARTY_DIR is set properly.")
endif()

# Add LLVM-provided GoogleTest include directories.
target_include_directories(omptest PRIVATE
${LLVM_THIRD_PARTY_DIR}/unittest/googletest/include)
# Set GTest include directories for omptest
target_include_directories(omptest PUBLIC
$<BUILD_INTERFACE:${OMPTEST_GTEST_PATH}>
$<BUILD_INTERFACE:${OMPTEST_GTEST_PATH}/include>
)

# TODO: Re-visit ABI breaking checks, disable for now.
target_compile_definitions(omptest PUBLIC
-DLLVM_DISABLE_ABI_BREAKING_CHECKS_ENFORCING)
# Make the targets default_gtest and default_gtest_main available.
build_gtest()
Copy link
Member

@Meinersbur Meinersbur Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

build_gtest() unfortunatelty only exists in a runtime build (Using runtime/CMakeLists.txt as top-level CMake stript).

In a LLVM_ENABLE_PROJECTS=openmp build, default_gtest should already be present. No build_gtest() needed (I wish I could have done without introducing build_gtest())

In an openmp-standalone build, we need to add_subdirectory(third-party/unittest) ourselves. Could also be done in a runtimes build instead of build_gtest() to reduce the number of cases we handle. See e.g. how done by others
https://github.com/llvm/llvm-project/blob/main/lldb/cmake/modules/LLDBStandalone.cmake#L109-L111
https://github.com/llvm/llvm-project/blob/main/flang/CMakeLists.txt#L227-L231

# Link against gtest and gtest_main
target_link_libraries(omptest PRIVATE ${GTEST_LINK_CMD})
# Add GoogleTest-based header and merge GTest into the shared lib.
target_sources(omptest PRIVATE
./include/OmptTesterGoogleTest.h
$<TARGET_OBJECTS:default_gtest>
Copy link
Member

@Meinersbur Meinersbur Nov 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to not use?

target_link_libraries(omptest default_gtest)

It should also make the target_include_directories and target_link_libraries redundant.
)

# Link against LLVMSupport and Threads (recommended for GTest).
find_package(Threads REQUIRED)
target_link_libraries(omptest PUBLIC LLVMSupport Threads::Threads)
Copy link
Member

@Meinersbur Meinersbur Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[serious] The point of #164794 was to NOT link LLVMSupport. This is what is causing the ABI linking problem.

# Ensure that embedded GTest symbols are exported from libomptest.so even in
# builds that default to hidden.
set_target_properties(omptest PROPERTIES
CXX_VISIBILITY_PRESET default
VISIBILITY_INLINES_HIDDEN OFF
)

# Make sure correct include directories are used, e.g. by the unit tests.
# Otherwise, ABI-checks may fail.
if(DEFINED LLVM_INCLUDE_DIRS)
target_include_directories(omptest PUBLIC ${LLVM_INCLUDE_DIRS})
endif()
else()
# Add 'standalone' compile definitions
target_compile_definitions(omptest PRIVATE
Expand Down
2 changes: 1 addition & 1 deletion openmp/tools/omptest/test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ set(UNITTEST_SOURCES
unittests/asserter-seq-test.cpp
unittests/internal-event-eq-test.cpp
unittests/internal-event-tostring-test.cpp
unittests/internal-util-test
unittests/internal-util-test.cpp
unittests/main-test.cpp
)
add_executable(omptest-unittests ${UNITTEST_SOURCES})
Expand Down
Loading