This is an archive of the discontinued LLVM Phabricator instance.

Allow compiler-rt test targets to work with multi-config CMake generators
ClosedPublic

Authored by gbedwell on Oct 2 2017, 10:53 AM.

Details

Summary

Allow compiler-rt test targets to work with multi-config CMake generators

Multi-config CMake generators need lit to be able to resolve paths of
artifacts from previous build steps at lit time, rather than expect them
to be fully resolved at CMake time as they may contain the build mode.

Previously check-asan, etc failed due to not being able to resolve
"$(Configuration)" in various paths.

https://reviews.llvm.org/D38470 is a pre-requisite.

Diff Detail

Repository
rL LLVM

Event Timeline

gbedwell created this revision.Oct 2 2017, 10:53 AM
rnk accepted this revision.Oct 2 2017, 12:39 PM

lgtm

test/lit.common.configured.in
19 ↗(On Diff #117391)

I see, this has the same treatment in LLVM.

This revision is now accepted and ready to land.Oct 2 2017, 12:39 PM

Could you give me a few minutes to test it before merging, please?

mgorny added a comment.Oct 2 2017, 1:19 PM

Well, I think I did everything right, yet I still get:

CMake Error at cmake/Modules/AddCompilerRT.cmake:521 (set_llvm_build_mode):
  Unknown CMake command "set_llvm_build_mode".
Call Stack (most recent call first):
  unittests/CMakeLists.txt:1 (configure_compiler_rt_lit_site_cfg)

AFAICS AddLLVM is not included anywhere if building stand-alone (that is, outside LLVM sources).

rnk added a comment.Oct 2 2017, 1:20 PM

Well, I think I did everything right, yet I still get:

CMake Error at cmake/Modules/AddCompilerRT.cmake:521 (set_llvm_build_mode):
  Unknown CMake command "set_llvm_build_mode".
Call Stack (most recent call first):
  unittests/CMakeLists.txt:1 (configure_compiler_rt_lit_site_cfg)

AFAICS AddLLVM is not included anywhere if building stand-alone (that is, outside LLVM sources).

Did you apply the LLVM side change and install it locally to test? I believe AddLLVM.cmake is shipped in the install step...

mgorny added a comment.Oct 2 2017, 2:13 PM

Yes, I did apply it both to the installed CMake files and the external LLVM source tree used by compiler-rt. I can't see where else it could have gotten an old version from, and I don't grep any AddLLVM includes either.

beanz added inline comments.Oct 2 2017, 2:24 PM
test/CMakeLists.txt
2 ↗(On Diff #117391)

You can't remove this. We only include AddLLVM in the test directory so that we can support building compiler-rt without LLVM, but we need this if you want to support running tests when building out of tree.

unittests/CMakeLists.txt
2 ↗(On Diff #117391)

Same as my earlier comment. You can't remove this.

Ah. Thanks! I tested my patches against multiple platforms and generators, but didn't test an external build. D'oh!

Given that I've moved the usages of AddLLVM items into AddCompilerRT.cmake does it make sense to move the include there too? I'm happy either way. Thanks everyone for reviewing!

beanz edited edge metadata.Oct 2 2017, 4:57 PM

You can put the include inside the function body of configure_compiler_rt_lit_site_cfg, but you can't put it just inside the file. By design AddCompilerRT is mostly usable without AddLLVM.

gbedwell updated this revision to Diff 119856.Oct 23 2017, 8:42 AM

Sorry for the delay on this. I got pulled onto something else for a bit. I've put the include(AddLLVM) statements back and tested a standalone compiler-rt build (on Linux) too. Thanks again for taking a look.

gbedwell requested review of this revision.Oct 26 2017, 9:54 AM
gbedwell edited edge metadata.
beanz accepted this revision.Nov 7 2017, 3:22 PM

LGTM.

This revision is now accepted and ready to land.Nov 7 2017, 3:22 PM
This revision was automatically updated to reflect the committed changes.