Skip to content

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