This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by leonardchan on Sep 10 2021, 12:49 PM.

Details

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.

Diff Detail

Event Timeline

leonardchan created this revision.Sep 10 2021, 12:49 PM
leonardchan requested review of this revision.Sep 10 2021, 12:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2021, 12:49 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
phosek added inline comments.Sep 16 2021, 10:43 PM
compiler-rt/test/CMakeLists.txt
48

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 retitled this revision from [compiler-rt] Ensure LIT_TEST_DEP targets are actually built to [compiler-rt] Ensure required deps for tests targets are actually built.
leonardchan added inline comments.
compiler-rt/test/CMakeLists.txt
48

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 added inline comments.
compiler-rt/test/CMakeLists.txt
48

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

phosek added inline comments.Sep 23 2021, 12:53 AM
compiler-rt/test/CMakeLists.txt
48

llvm-readelf should be addressed by D110313.

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.

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?

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.

leonardchan edited the summary of this revision. (Show Details)
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2021, 2:35 PM
phosek accepted this revision.Sep 29 2021, 2:38 PM

LGTM thanks for debugging this!

This revision is now accepted and ready to land.Sep 29 2021, 2:38 PM
This revision was landed with ongoing or failed builds.Sep 29 2021, 2:41 PM
This revision was automatically updated to reflect the committed changes.

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.