This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Provide options for toggling on and off various runtime libraries.
ClosedPublic

Authored by beanz on Nov 19 2015, 3:06 PM.

Details

Summary

Rather than having to add new "experimental" options each time someone wants to work on bringing a sanitizer to a new platform, this patch makes options for all of them.

The default values for the options are set by the platform checks that would have enabled them, but they can be overridden on or off.

Diff Detail

Repository
rL LLVM

Event Timeline

beanz updated this revision to Diff 40711.Nov 19 2015, 3:06 PM
beanz retitled this revision from to [CMake] Provide options for toggling on and off various runtime libraries..
beanz updated this object.
beanz added reviewers: kubamracek, samsonov.
beanz added a subscriber: llvm-commits.
samsonov edited edge metadata.Nov 19 2015, 3:23 PM

Can we have smth. like LLVM_TARGETS_TO_BUILD ? I.e. have the one CMake variable which takes semicolon-separated list of sanitizers / configurations to build (which defaults to ALL, which expands to everything supported on a given platform)?

beanz updated this revision to Diff 40811.Nov 20 2015, 11:51 AM
beanz edited edge metadata.

Migrating enabling sanitizers to a semi-colon separated list. "all" defaults to the list of supported sanitizers determined by config-ix.cmake.

kubamracek added inline comments.Dec 1 2015, 5:49 AM
cmake/config-ix.cmake
585 ↗(On Diff #40811)

I think we need to add interception here as well. Otherwise using COMPILER_RT_RUNTIMES_TO_BUILD=asan doesn't build for me.

lib/lsan/CMakeLists.txt
28 ↗(On Diff #40811)

Can we wrap this in a macro (and use it in ubsan/CMakeLists.txt as well)?

lib/tsan/CMakeLists.txt
178 ↗(On Diff #40811)

Shouldn't this be just dd instead of tsan/dd?

test/ubsan/CMakeLists.txt
38 ↗(On Diff #40811)

remove the extra paren

42 ↗(On Diff #40811)

same here

beanz updated this revision to Diff 41951.Dec 4 2015, 3:24 PM

Updates based on feedback from Kuba.

Looks reasonable, several comments/nits below.

cmake/Modules/CompilerRTUtils.cmake
81 ↗(On Diff #41951)

I'm curious, can we make it look more like "check_c_compiler_flag" or "check_symbol_exists"

check_list_contains(list value output)

?

84 ↗(On Diff #41951)

use True/False instead?

90 ↗(On Diff #41951)

See note below - probably remove this?

cmake/config-ix.cmake
562 ↗(On Diff #41951)

cfi is available not only on Windows, I think lib/cfi is added unconditionally.

588 ↗(On Diff #41951)

Please describe what you're doing here.

lib/CMakeLists.txt
12 ↗(On Diff #41951)

Is it a debug output?

test/CMakeLists.txt
40 ↗(On Diff #41951)

How do you handle CFI special case here?

test/ubsan/CMakeLists.txt
23 ↗(On Diff #41951)

I actually think this macro is an overkill, and would prefer literal mentioning of COMPILER_RT_RUNTIMES_TO_BUILD here. Sorry for the nit.

beanz added a comment.Dec 9 2015, 1:20 PM

Updated patches coming shortly. One comment below.

lib/CMakeLists.txt
12 ↗(On Diff #41951)

I intended this to be much like the logging we do in LLVM for adding targets.

I thought this heading should be there because if there are no runtimes following it you know there are no runtimes being included.

beanz updated this revision to Diff 42331.Dec 9 2015, 1:20 PM

Updates based on feedback from samsonov.

samsonov accepted this revision.Dec 9 2015, 1:59 PM
samsonov edited edge metadata.

LGTM. Thanks!

test/cfi/CMakeLists.txt
2 ↗(On Diff #42331)

Copy the comment from test/CMakeLists.txt
\# CFI tests require diagnostic mode, which is implemented in UBSan.

This revision is now accepted and ready to land.Dec 9 2015, 1:59 PM
This revision was automatically updated to reflect the committed changes.