This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] [cmake] Respect COMPILER_RT_BUILD_* for libs, headers and tests
AbandonedPublic

Authored by mgorny on Oct 1 2016, 6:18 AM.

Details

Summary

Refactor the code to respect COMPILER_RT_BUILD_SANITIZERS and COMPILER_RT_BUILD_XRAY consistently for libraries, headers and tests. In order to achieve that, factor those variables in when setting COMPILER_RT_* in config-ix.cmake. This makes the COMPILER_RT_* variables suitable for determining both whether to build libraries and whether to run tests.

Add a new set of conditionals to include/CMakeLists.txt to prevent installing xray and sanitizer headers when XRay is disabled or sanitizers are disabled or unsupported appropriately.

Update the conditionals in lib/CMakeLists.txt appropriately to remove unnecessary double-use of COMPILER_RT_BUILD_* variables when COMPILER_RT_HAS_* already controls the particular component.

Fix sanitizer_common tests to respect COMPILER_RT_HAS_* when determining which tools are supported.

Diff Detail

Event Timeline

mgorny updated this revision to Diff 73195.Oct 1 2016, 6:18 AM
mgorny retitled this revision from to [compiler-rt] [cmake] Respect COMPILER_RT_BUILD_* consistently for libs & tests.
mgorny updated this object.
mgorny added reviewers: beanz, samsonov.
mgorny added a subscriber: cfe-commits.
mgorny updated this revision to Diff 73198.EditedOct 1 2016, 8:24 AM
mgorny retitled this revision from [compiler-rt] [cmake] Respect COMPILER_RT_BUILD_* consistently for libs & tests to [compiler-rt] [cmake] Respect COMPILER_RT_BUILD_* for libs, headers and tests.
mgorny updated this object.

Updated to control installing headers as well.

mgorny updated this revision to Diff 93296.Mar 28 2017, 2:06 PM

Rebased. Ping. Now that we have lit tests for builtins, it would be really useful for us to be able to build them without having to enable sanitizers.

clm added a subscriber: clm.Mar 28 2017, 3:48 PM
jroelofs added inline comments.
test/sanitizer_common/CMakeLists.txt
7–8

Are the CMAKE_SYSTEM_NAME_MATCHES guards here necessary any more, now that the checks are happening in config-ix.cmake?

mgorny added inline comments.Mar 28 2017, 11:59 PM
test/sanitizer_common/CMakeLists.txt
7–8

ASAN doesn't have any system guard in config-ix.cmake, and the other guards are different (narrower) than those in config-ix. I think those restrictions might be like that on purpose, so if I were to drop them, I'd rather do it in separate patch. @samsonov?

mgorny updated this revision to Diff 93347.Mar 29 2017, 12:00 AM

Needed to rebase again.

weimingz edited edge metadata.Mar 29 2017, 9:58 AM

Looks good to me but I'm not very familiar with the build of sanitizer and xray.

compnerd added inline comments.Mar 29 2017, 11:18 AM
cmake/config-ix.cmake
418–421

What does this really leave in terms of targets which the sanitizer supports but doesn't have the common library?

535

Seems like it might be a good time to hoist the OS checks and add a COMPILER_RT_*_SUPPORTED macro (e.g. COMPILER_RT_XRAY_SUPPORTED).

lib/CMakeLists.txt
60

Im not sure I understand the comment. This looks like it may prevent disabling X-ray?

mgorny added inline comments.Mar 29 2017, 11:39 AM
cmake/config-ix.cmake
418–421

I'm not sure if I understand your question correctly. If it's about the platform logic, I don't know what it is about and I think it's a bit out of scope of this change. My goal is to merely disable sanitizer-related stuff when sanitizers are not built.

535

Sounds like material for a separate patch to me.

lib/CMakeLists.txt
60

The comment was supposed to indicate that we don't need if(COMPILER_RT_BUILD_XRAY) here because it is already included in COMPILER_RT_HAS_... value.

mgorny abandoned this revision.Apr 13 2017, 9:34 AM

D31864 covered all that was needed for Gentoo, and I lack the knowledge to push the full split properly forward.