[compiler-rt] Ensure required deps for tests targets are actually built

[leonardchan]

Summary

When building compiler-rt via runtimes, many tests fail because tools like FileCheck and count aren't built yet. This is because the RUNTIME_TEST_DEPENDENCIES haven't been added to any of the compiler-rt targets. The fix is to explicitly add any runtimes as test_targets.

[leonardchan]

*ping*

[phosek]

compiler-rt/test/CMakeLists.txt

48 ↗

(On Diff #371991)

This is going to run Ninja in the LLVM build once for each dependency listed above, correct? That seems quite expensive.

We already pass most of these to the child build via corresponding CMake variables, see https://github.com/llvm/llvm-project/blob/ed921282e551f2252ccfcbddd7a85ad8a006ed3f/llvm/cmake/modules/LLVMExternalProjectUtils.cmake#L160

For example, if just need some readelf implementation and not necessarily llvm-readelf, it may be better to use the value of CMAKE_READELF and propagate that down to tests through substitution (that is wherever the tests invoke llvm-readelf, we would replace it with %readelf).

We're still going to need this logic for tools where there's no corresponding CMake variable like FileCheck but it should be significantly fewer ones.

[leonardchan]

compiler-rt/test/CMakeLists.txt

48 ↗

(On Diff #371991)

So it looks like only FileCheck, count, and not are required but don't have cmake variables. Would what I have now be ok where instead we just iterate over a trimmed list of tools?

[leonardchan]

compiler-rt/test/CMakeLists.txt

48 ↗

(On Diff #371991)

I lied, there's also clang-resource-headers and llvm-readelf.

[phosek]

compiler-rt/test/CMakeLists.txt

48 ↗

(On Diff #371991)

llvm-readelf should be addressed by D110313.

[leonardchan]

Updated to exclude readelf. We also don't need any of the other tools added in the NOT COMPILER_RT_STANDALONE_BUILD case, so I just added them to LIT_ONLY_TOOLS in the COMPILER_RT_STANDALONE_BUILD case.

[phosek]

I'm still unsure if this is the right strategy because it creates a dependency cycle. Specifically, you have the LLVM build trigger the build of runtimes, and that build would invoke LLVM build again to ensure that the necessary tools have been built. That shouldn't be necessary though. The LLVM build already ensures that those tools are being built by making the runtimes build depend on these tools, see https://github.com/llvm/llvm-project/blob/ac2daacb310cbb1732de1c139be7a0e8e982169e/llvm/runtimes/CMakeLists.txt#L461

What might be needed is a way to tell the compiler-rt build where to find these tools. I looked elsewhere in LLVM and it seems like Polly is already taking that approach, see https://github.com/llvm/llvm-project/blob/main/polly/test/CMakeLists.txt#L6. Do you think that we could use this strategy?

[leonardchan]

In D109625#3029581, @phosek wrote:

I'm still unsure if this is the right strategy because it creates a dependency cycle. Specifically, you have the LLVM build trigger the build of runtimes, and that build would invoke LLVM build again to ensure that the necessary tools have been built. That shouldn't be necessary though. The LLVM build already ensures that those tools are being built by making the runtimes build depend on these tools, see https://github.com/llvm/llvm-project/blob/ac2daacb310cbb1732de1c139be7a0e8e982169e/llvm/runtimes/CMakeLists.txt#L461

What might be needed is a way to tell the compiler-rt build where to find these tools. I looked elsewhere in LLVM and it seems like Polly is already taking that approach, see https://github.com/llvm/llvm-project/blob/main/polly/test/CMakeLists.txt#L6. Do you think that we could use this strategy?

Ah, so it seems that both`test_targets` and SUB_CHECK_TARGETS in https://github.com/llvm/llvm-project/blob/ac2daacb310cbb1732de1c139be7a0e8e982169e/llvm/runtimes/CMakeLists.txt#L474 are both empty, so these dependencies are never added. Lemme see if I can find why they're empty.

[phosek]

LGTM thanks for debugging this!

[leonardchan]

Hmm. Just found out about this, but for future reference, I think this will only work for ninja check-runtimes and ninja check-runtimes-{target}, but not necessarily ninja -C runtimes/runtimes-{target} check-runtimes/check-compilter-rt.

llvm/runtimes/CMakeLists.txt

foreach(component ${LLVM_RUNTIME_DISTRIBUTION_COMPONENTS})
     set(install-${component}-${name}-stripped install-${component}-stripped)
     list(APPEND ${name}_extra_targets ${component}-${name} install-${component}-${name} install-${component}-${name}-stripped)
   endforeach()
 
   if(LLVM_INCLUDE_TESTS)
     set(runtimes-test-depends-${name} runtimes-test-depends)
     set(check-runtimes-${name} check-runtimes)
     list(APPEND ${name}_test_targets runtimes-test-depends-${name} check-runtimes-${name})
+    list(APPEND test_targets ${${name}_test_targets})
     foreach(target_name IN LISTS SUB_CHECK_TARGETS)
       set(${target_name}-${name} ${target_name})
       list(APPEND ${name}_test_targets ${target_name}-${name})
       list(APPEND test_targets ${target_name}-${name})
     endforeach()
     set(test_targets "${test_targets}" PARENT_SCOPE)
   endif()