Honor COMPILER_RT_INCLUDE_TESTS when using LLVM_BUILD_EXTERNAL_COMPILER_RT=ON

[azharudd]

Summary

When building with LLVM_BUILD_EXTERNAL_COMPILER_RT=ON (where we build
compiler-rt with the just built clang), the compiler-rt tests are being
enabled based on LLVM_INCLUDE_TESTS and not COMPILER_RT_INCLUDE_TESTS,
thereby providing no way to disable just the compiler-rt tests.

This change makes the compiler-rt tests depend on COMPILER_RT_INCLUDE_TESTS
instead. The default value of COMPILER_RT_INCLUDE_TESTS is set to the
value of LLVM_INCLUDE_TESTS (this retains the existing behavior too).

[beanz]

I question whether we should be extending this or killing it off... Is there a reason you're using LLVM_BUILD_EXTERNAL_COMPILER_RT instead of LLVM_ENABLE_RUNTIMES=compiler-rt?

[azharudd]

In D124474#3475432, @beanz wrote:

I question whether we should be extending this or killing it off... Is there a reason you're using LLVM_BUILD_EXTERNAL_COMPILER_RT instead of LLVM_ENABLE_RUNTIMES=compiler-rt?

In this case I'm just trying to make it work with existing build configurations where it is being used (for example Swift). We should definitely be moving away from LLVM_BUILD_EXTERNAL_COMPILER_RT in favor of LLVM_ENABLE_RUNTIMES eventually though.

[beanz]

In D124474#3475539, @azharudd wrote:

In this case I'm just trying to make it work with existing build configurations where it is being used (for example Swift). We should definitely be moving away from LLVM_BUILD_EXTERNAL_COMPILER_RT in favor of LLVM_ENABLE_RUNTIMES eventually though.

My point here is that maybe you should move away from the legacy way of building now rather than improving it... Not sure how @phosek feels about this. I would personally very much prefer if we only had one way to build compiler-rt instead of the half dozen we have today.

[phosek]

In D124474#3475550, @beanz wrote:

In D124474#3475539, @azharudd wrote:

In this case I'm just trying to make it work with existing build configurations where it is being used (for example Swift). We should definitely be moving away from LLVM_BUILD_EXTERNAL_COMPILER_RT in favor of LLVM_ENABLE_RUNTIMES eventually though.

My point here is that maybe you should move away from the legacy way of building now rather than improving it... Not sure how @phosek feels about this. I would personally very much prefer if we only had one way to build compiler-rt instead of the half dozen we have today.

I'd definitely prefer moving towards LLVM_ENABLE_RUNTIMES. We already require LLVM_ENABLE_RUNTIMES for libc++, libc++abi and libunwind and I'm going to propose doing the same for compiler-rt as well.

[azharudd]

In D124474#3475550, @beanz wrote:

In D124474#3475539, @azharudd wrote:

In this case I'm just trying to make it work with existing build configurations where it is being used (for example Swift). We should definitely be moving away from LLVM_BUILD_EXTERNAL_COMPILER_RT in favor of LLVM_ENABLE_RUNTIMES eventually though.

My point here is that maybe you should move away from the legacy way of building now rather than improving it... Not sure how @phosek feels about this. I would personally very much prefer if we only had one way to build compiler-rt instead of the half dozen we have today.

I understand, and I agree there should be only one way to do it. I did think about it but it is more involved and I unfortunately don't have the time to commit to it right now. I'm hoping we can take care of it at some point (I'm tracking it), but until then this satisfies the immediate need.

[beanz]

I don't want to roadblock or say "no we absolutely can't let this in", _but_... Apple Clang and Swift have long been the only reason we've kept this code alive. I didn't have the time to fix Apple Clang years ago, and nobody has made the effort since.

I'm very aware of the technical debt of build-script, and I don't want LLVM to be in a position where we need to continue supporting confusing build scenarios because the Swift build is too complex to fix.

I'll defer to whatever @phosek, @compnerd, @smeenai, and @ldionne feel is right here, but I do believe at some point in the future we should delete this code path, whether downstream users have migrated or not.

[delcypher]

I'd definitely prefer moving towards LLVM_ENABLE_RUNTIMES. We already require LLVM_ENABLE_RUNTIMES for libc++, libc++abi and libunwind and I'm going to propose doing the same for compiler-rt as well.

+1 on this. Compiler-RT's CMake code is extremely and unnecessarily complicated because we support far too many exotic build configurations.

I don't want to roadblock or say "no we absolutely can't let this in", _but_... Apple Clang and Swift have long been the only reason we've kept this code alive. I didn't have the time to fix Apple Clang years ago, and nobody has made the effort since.

I'm going to approve because this change seems reasonable. However, I think we need to have a serious conversation about removing LLVM_BUILD_EXTERNAL_COMPILER_RT with an actual deadline so that we can make forward progress here. This review is probably the wrong place to have that discussion. We probably should have an RFC on the forums with the deprecation plan.

[delcypher]

Change LGTM.

[beanz]

Posted an RFC: https://discourse.llvm.org/t/rfc-deprecate-and-remove-llvm-build-external-compiler-rt/62058

[beanz]

I posted a PR to swift that migrates build-script to use LLVM_ENABLE_RUNTIMES: https://github.com/apple/swift/pull/58465

Obviously I can't test this against your internal infrastructure, but the change here is pretty trivial.

clang/runtime/CMakeLists.txt

if (CMAKE_GENERATOR MATCHES "Make")
     # Use special command for Makefiles to support parallelism.
     set(${out_var} "$(MAKE)" "${target}" PARENT_SCOPE)
   else()
     set(${out_var} ${CMAKE_COMMAND} --build . --target ${target}
                                     --config $<CONFIG> PARENT_SCOPE)
   endif()
 endfunction()
 
+option(COMPILER_RT_INCLUDE_TESTS "Generate build targets for compiler-rt tests." ${LLVM_INCLUDE_TESTS})
+
 set(COMPILER_RT_SRC_ROOT ${LLVM_MAIN_SRC_DIR}/projects/compiler-rt)
 # Fallback to the external path, if the other one isn't available.
 # This is the same behavior (try "internal", then check the LLVM_EXTERNAL_...
 # variable) as in add_llvm_external_project
 if(NOT EXISTS ${COMPILER_RT_SRC_ROOT})
   # We don't want to set it if LLVM_EXTERNAL_COMPILER_RT_SOURCE_DIR is ""
   if(LLVM_EXTERNAL_COMPILER_RT_SOURCE_DIR)
     set(COMPILER_RT_SRC_ROOT ${LLVM_EXTERNAL_COMPILER_RT_SOURCE_DIR})
if(variableName MATCHES "^COMPILER_RT")
         -D${variableName}=${value})
     endif()
   endforeach()
 
   set(compiler_rt_configure_deps)
   if(TARGET cxx-headers)
     list(APPEND compiler_rt_configure_deps "cxx-headers")
   endif()
-  if(LLVM_INCLUDE_TESTS)
+  if(COMPILER_RT_INCLUDE_TESTS)
     list(APPEND compiler_rt_configure_deps LLVMTestingSupport)
   endif()
 
   ExternalProject_Add(compiler-rt
     DEPENDS llvm-config clang ${compiler_rt_configure_deps}
     PREFIX ${COMPILER_RT_PREFIX}
     SOURCE_DIR ${COMPILER_RT_SRC_ROOT}
     STAMP_DIR ${STAMP_DIR}
     BINARY_DIR ${BINARY_DIR}
     CMAKE_ARGS ${CLANG_COMPILER_RT_CMAKE_ARGS}
                -DCMAKE_C_COMPILER=${LLVM_RUNTIME_OUTPUT_INTDIR}/clang
                -DCMAKE_CXX_COMPILER=${LLVM_RUNTIME_OUTPUT_INTDIR}/clang++
                -DCMAKE_ASM_COMPILER=${LLVM_RUNTIME_OUTPUT_INTDIR}/clang
                -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
                -DCMAKE_MAKE_PROGRAM=${CMAKE_MAKE_PROGRAM}
                -DCMAKE_C_COMPILER_LAUNCHER=${CMAKE_C_COMPILER_LAUNCHER}
                -DCMAKE_CXX_COMPILER_LAUNCHER=${CMAKE_CXX_COMPILER_LAUNCHER}
                -DLLVM_CONFIG_PATH=${LLVM_RUNTIME_OUTPUT_INTDIR}/llvm-config
                -DLLVM_LIT_ARGS=${LLVM_LIT_ARGS}
                -DCOMPILER_RT_OUTPUT_DIR=${LLVM_LIBRARY_OUTPUT_INTDIR}/clang/${CLANG_VERSION}
                -DCOMPILER_RT_EXEC_OUTPUT_DIR=${LLVM_RUNTIME_OUTPUT_INTDIR}
                -DCOMPILER_RT_INSTALL_PATH:PATH=lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION}
-               -DCOMPILER_RT_INCLUDE_TESTS=${LLVM_INCLUDE_TESTS}
                -DCMAKE_INSTALL_PREFIX=${CMAKE_INSTALL_PREFIX}
                -DLLVM_LIBDIR_SUFFIX=${LLVM_LIBDIR_SUFFIX}
                -DLLVM_RUNTIME_OUTPUT_INTDIR=${LLVM_RUNTIME_OUTPUT_INTDIR}
                -DCMAKE_OSX_DEPLOYMENT_TARGET=${CMAKE_OSX_DEPLOYMENT_TARGET}
                -DCMAKE_OSX_SYSROOT:PATH=${CMAKE_OSX_SYSROOT}
                ${COMPILER_RT_PASSTHROUGH_VARIABLES}
     INSTALL_COMMAND ""
     STEP_TARGETS configure build
foreach(runtime ${COMPILER_RT_RUNTIMES})
     get_ext_project_build_command(build_runtime_cmd ${runtime})
     add_custom_target(${runtime}
       COMMAND ${build_runtime_cmd}
       DEPENDS compiler-rt-configure
       WORKING_DIRECTORY ${BINARY_DIR}
       VERBATIM USES_TERMINAL)
   endforeach()
 
-  if(LLVM_INCLUDE_TESTS)
+  if(COMPILER_RT_INCLUDE_TESTS)
     # Add binaries that compiler-rt tests depend on.
     set(COMPILER_RT_TEST_DEPENDENCIES
       FileCheck count not llvm-nm llvm-objdump llvm-symbolizer llvm-jitlink)
 
     # Add top-level targets for various compiler-rt test suites.
     set(COMPILER_RT_TEST_SUITES check-fuzzer check-asan check-hwasan check-asan-dynamic check-dfsan
       check-lsan check-msan check-sanitizer check-tsan check-ubsan check-ubsan-minimal
       check-profile check-cfi check-cfi-and-supported check-safestack check-gwp_asan)