Skip to content

[memprof] Use COMPILER_RT_TEST_COMPILER #88074

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.

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

Merged

Conversation

arichardson
Copy link
Member

Unlike the other compiler-rt unit tests MemProf was not using the
generate_compiler_rt_tests() helper that ensures the test is compiled
using the test compiler (generally the Clang binary built earlier).
This was exposed by #83088
because it started adding Clang-specific flags to
COMPILER_RT_UNITTEST_CFLAGS if the compiler ID matched "Clang".

This change should fix the buildbots that compile compiler-rt using
a GCC compiler with LLVM_ENABLE_PROJECTS=compiler-rt.

Created using spr 1.3.6-beta.1
@arichardson arichardson requested review from snehasish and vitalybuka and removed request for snehasish April 9, 2024 00:30
@llvmbot llvmbot added compiler-rt PGO Profile Guided Optimizations labels Apr 9, 2024
@arichardson arichardson requested review from snehasish and zeroomega and removed request for vitalybuka April 9, 2024 00:30
@llvmbot
Copy link
Member

llvmbot commented Apr 9, 2024

@llvm/pr-subscribers-pgo

Author: Alexander Richardson (arichardson)

Changes

Unlike the other compiler-rt unit tests MemProf was not using the
generate_compiler_rt_tests() helper that ensures the test is compiled
using the test compiler (generally the Clang binary built earlier).
This was exposed by #83088
because it started adding Clang-specific flags to
COMPILER_RT_UNITTEST_CFLAGS if the compiler ID matched "Clang".

This change should fix the buildbots that compile compiler-rt using
a GCC compiler with LLVM_ENABLE_PROJECTS=compiler-rt.


Full diff: https://github.com/llvm/llvm-project/pull/88074.diff

1 Files Affected:

  • (modified) compiler-rt/lib/memprof/tests/CMakeLists.txt (+34-20)
diff --git a/compiler-rt/lib/memprof/tests/CMakeLists.txt b/compiler-rt/lib/memprof/tests/CMakeLists.txt
index f812bd1f86ff8f..733e7dda72c611 100644
--- a/compiler-rt/lib/memprof/tests/CMakeLists.txt
+++ b/compiler-rt/lib/memprof/tests/CMakeLists.txt
@@ -41,33 +41,47 @@ if(NOT WIN32)
   list(APPEND MEMPROF_UNITTEST_LINK_FLAGS -pthread)
 endif()
 
+set(MEMPROF_UNITTEST_DEPS)
+if (TARGET cxx-headers OR HAVE_LIBCXX)
+  list(APPEND MEMPROF_UNITTEST_DEPS cxx-headers)
+endif()
+
 set(MEMPROF_UNITTEST_LINK_LIBRARIES
   ${COMPILER_RT_UNWINDER_LINK_LIBS}
   ${SANITIZER_TEST_CXX_LIBRARIES})
-list(APPEND MEMPROF_UNITTEST_LINK_LIBRARIES "dl")
-
-if(COMPILER_RT_DEFAULT_TARGET_ARCH IN_LIST MEMPROF_SUPPORTED_ARCH)
-  # MemProf unit tests are only run on the host machine.
-  set(arch ${COMPILER_RT_DEFAULT_TARGET_ARCH})
+append_list_if(COMPILER_RT_HAS_LIBDL -ldl MEMPROF_UNITTEST_LINK_LIBRARIES)
 
-  add_executable(MemProfUnitTests 
-    ${MEMPROF_UNITTESTS}
-    ${COMPILER_RT_GTEST_SOURCE}
-    ${COMPILER_RT_GMOCK_SOURCE}
-    ${MEMPROF_SOURCES}
+# Adds memprof tests for each architecture.
+macro(add_memprof_tests_for_arch arch)
+  set(MEMPROF_TEST_RUNTIME_OBJECTS
     $<TARGET_OBJECTS:RTSanitizerCommon.${arch}>
     $<TARGET_OBJECTS:RTSanitizerCommonCoverage.${arch}>
     $<TARGET_OBJECTS:RTSanitizerCommonLibc.${arch}>
     $<TARGET_OBJECTS:RTSanitizerCommonSymbolizer.${arch}>
-    $<TARGET_OBJECTS:RTSanitizerCommonSymbolizerInternal.${arch}>)
-  set_target_compile_flags(MemProfUnitTests ${MEMPROF_UNITTEST_CFLAGS})
-  set_target_link_flags(MemProfUnitTests ${MEMPROF_UNITTEST_LINK_FLAGS})
-  target_link_libraries(MemProfUnitTests ${MEMPROF_UNITTEST_LINK_LIBRARIES})
-
-  if (TARGET cxx-headers OR HAVE_LIBCXX)
-    add_dependencies(MemProfUnitTests cxx-headers)
-  endif()
+    $<TARGET_OBJECTS:RTSanitizerCommonSymbolizerInternal.${arch}>
+  )
+  set(MEMPROF_TEST_RUNTIME RTMemProfTest.${arch})
+  add_library(${MEMPROF_TEST_RUNTIME} STATIC ${MEMPROF_TEST_RUNTIME_OBJECTS})
+  set_target_properties(${MEMPROF_TEST_RUNTIME} PROPERTIES
+    ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
+    FOLDER "Compiler-RT Runtime tests")
+  set(MEMPROF_TEST_OBJECTS)
+  generate_compiler_rt_tests(MEMPROF_TEST_OBJECTS
+    MemProfUnitTests "MemProf-${arch}-UnitTest" ${arch}
+    RUNTIME ${MEMPROF_TEST_RUNTIME}
+    DEPS ${MEMPROF_UNITTEST_DEPS}
+    SOURCES ${MEMPROF_UNITTESTS} ${MEMPROF_SOURCES} ${COMPILER_RT_GTEST_SOURCE}
+    COMPILE_DEPS ${MEMPROF_UNIT_TEST_HEADERS}
+    CFLAGS ${MEMPROF_UNITTEST_CFLAGS}
+    LINK_FLAGS ${MEMPROF_UNITTEST_LINK_FLAGS} ${MEMPROF_UNITTEST_LINK_LIBRARIES})
+endmacro()
 
-  set_target_properties(MemProfUnitTests PROPERTIES
-    RUNTIME_OUTPUT_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR})
+# Interception unit tests testsuite.
+add_custom_target(MemProfUnitTests)
+set_target_properties(MemProfUnitTests PROPERTIES FOLDER "Compiler-RT Tests")
+if(COMPILER_RT_CAN_EXECUTE_TESTS AND COMPILER_RT_DEFAULT_TARGET_ARCH IN_LIST MEMPROF_SUPPORTED_ARCH)
+  # MemProf unit tests are only run on the host machine.
+  foreach(arch ${COMPILER_RT_DEFAULT_TARGET_ARCH})
+    add_memprof_tests_for_arch(${arch})
+  endforeach()
 endif()

@arichardson
Copy link
Member Author

arichardson commented Apr 9, 2024

@jplehr / @dyung I believe this should fix the issue you saw after #83088 landed.

Copy link
Collaborator

@vitalybuka vitalybuka left a comment

Choose a reason for hiding this comment

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

LGTM, but maybe wait with re-landing #88075

@jplehr
Copy link
Contributor

jplehr commented Apr 9, 2024

I'm no expert, but from description and what I understand this looks good to me.

Created using spr 1.3.6-beta.1
@arichardson arichardson merged commit 5601e35 into main Apr 9, 2024
@arichardson arichardson deleted the users/arichardson/spr/memprof-use-compiler_rt_test_compiler branch April 9, 2024 16:23
arichardson added a commit that referenced this pull request Apr 9, 2024
Created using spr 1.3.6-beta.1
arichardson added a commit that referenced this pull request Apr 12, 2024
Currently, the testsuite uses the default runtimes path to find the
runtimes libraries which may or may not match the just-built runtimes.
This change uses the `-resource-dir` flag for clang whenever
`COMPILER_RT_TEST_STANDALONE_BUILD_LIBS` is set to ensure that we are
actually testing the currently built libraries rather than the ones
bundled with `${COMPILER_RT_TEST_COMPILER}`.

The existing logic works fine when clang and compiler-rt share the same
build directory ``-DLLVM_ENABLE_PROJECTS=clang;compiler-rt`, but when
building compiler-rt separately we need to tell the compiler used for
the tests where it can find the just-built libraries.

This reduces the fixes check-all failures to one in my configuration:
```
cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo -G Ninja
-DCMAKE_C_COMPILER=$HOME/output/upstream-llvm/bin/clang
-DCMAKE_CXX_COMPILER=$HOME/output/upstream-llvm/bin/clang++
-DCOMPILER_RT_INCLUDE_TESTS=ON
-DLLVM_EXTERNAL_LIT=$HOME/build/upstream-llvm-project-build/bin/llvm-lit
-DLLVM_CMAKE_DIR=$HOME/output/upstream-llvm
-DCOMPILER_RT_DEBUG=OFF
-S $HOME/src/upstream-llvm-project/compiler-rt
-B $HOME/src/upstream-llvm-project/compiler-rt/cmake-build-all-sanitizers
```

This relands the previous PR with fixes for Windows.
Depends on #88074 to be merged
first for GCC buildbots.

Pull Request: #88075
bazuzi pushed a commit to bazuzi/llvm-project that referenced this pull request Apr 15, 2024
Currently, the testsuite uses the default runtimes path to find the
runtimes libraries which may or may not match the just-built runtimes.
This change uses the `-resource-dir` flag for clang whenever
`COMPILER_RT_TEST_STANDALONE_BUILD_LIBS` is set to ensure that we are
actually testing the currently built libraries rather than the ones
bundled with `${COMPILER_RT_TEST_COMPILER}`.

The existing logic works fine when clang and compiler-rt share the same
build directory ``-DLLVM_ENABLE_PROJECTS=clang;compiler-rt`, but when
building compiler-rt separately we need to tell the compiler used for
the tests where it can find the just-built libraries.

This reduces the fixes check-all failures to one in my configuration:
```
cmake -DCMAKE_BUILD_TYPE=RelWithDebInfo -G Ninja
-DCMAKE_C_COMPILER=$HOME/output/upstream-llvm/bin/clang
-DCMAKE_CXX_COMPILER=$HOME/output/upstream-llvm/bin/clang++
-DCOMPILER_RT_INCLUDE_TESTS=ON
-DLLVM_EXTERNAL_LIT=$HOME/build/upstream-llvm-project-build/bin/llvm-lit
-DLLVM_CMAKE_DIR=$HOME/output/upstream-llvm
-DCOMPILER_RT_DEBUG=OFF
-S $HOME/src/upstream-llvm-project/compiler-rt
-B $HOME/src/upstream-llvm-project/compiler-rt/cmake-build-all-sanitizers
```

This relands the previous PR with fixes for Windows.
Depends on llvm#88074 to be merged
first for GCC buildbots.

Pull Request: llvm#88075
mtrofin added a commit that referenced this pull request Apr 22, 2024
This reverts commit 8b2ba6a.

The uild errors (see below) were likely due to the same issue PR #88074 fixed. Addressed by following that PR.

https://lab.llvm.org/buildbot/#/builders/165/builds/52789
https://lab.llvm.org/buildbot/#/builders/91/builds/25273
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler-rt PGO Profile Guided Optimizations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants