[OpenMP] Add libomp unit test infrastructure #168063
Conversation
ro-i
force-pushed
the
users/ro-i/check-libomp-unit
branch
from
4c9e176 to
c801cb4
Compare
openmp/runtime/src/CMakeLists.txt
Outdated
| string(REPLACE "${LIBOMP_SRC_DIR}/exports_so.txt" | ||
| "${LIBOMP_SRC_DIR}/exports_test_so.txt" | ||
| LIBOMP_TEST_LDFLAGS "${LIBOMP_TEST_LDFLAGS}") |
There was a problem hiding this comment.
Instead of string-replacing which feels very hacky, could we make a parameterized version of libomp_get_libflags? E.g.
libomp_get_libflags(LIBOMP_TEST_LDFLAGS FOR_UNITTESTS)Where FOR_UNITTESTS is a cmake_parse_arguments option in libomp_get_libflags.
I think we don't need exports_test_so.txt at all; just do not use the --version-script switch and all symbols are visible to the unittests (assuming defaut -fvisibility=default).
There was a problem hiding this comment.
I think we don't need exports_test_so.txt at all; just do not use the --version-script switch
then we would need to string-remove that, tho?
There was a problem hiding this comment.
The modified libomp_get_libflags would not even add --version-script when collecting LIBOMP_TEST_LIBFLAGS for omp_testing
IMHO string replacement of command line options is a hacky thing to do, better to generate the right options in the first place.
openmp/runtime/src/CMakeLists.txt
Outdated
| endif() | ||
|
|
||
| # Create the testing library from the same objects as libomp. | ||
| add_library(omp_testing SHARED $<TARGET_OBJECTS:omp_objects>) |
There was a problem hiding this comment.
Consider moving the test-related CMake code into the unittests/ directory. In other LLVM projects, the test/ and unittest/ are skipped unlest LLVM_INCLUDE_TESTS (or its project-specific variant such as CLANG_INCLUDE_TESTS) is set.
There was a problem hiding this comment.
I did that with the latest commit, but it involves a little bit of cmake duplication, e.g. regarding LIBOMP_LINKER_LANGUAGE. Maybe we should stick to the previous version? Because this situation here is a little bit special in that we need to guarantee that omp_testing and omp are built as identically as possible.
There was a problem hiding this comment.
You can hoist the definition of LIBOMP_LINKER_LANGUAGE one level up, i.e. https://github.com/llvm/llvm-project/blob/main/openmp/runtime/CMakeLists.txt, so it is available in both, src/ and unittests/.
I usually add such things to the top-level CMakeLists.txt in the first place. I don't see why definitions such as LIBOMP_VERSION_MAJOR should not be available in the entire project.
(The tests in
TestKmpStr.cppare an automatically generated POC to make sure things work.)