[compiler-rt] [CMake] Only configure sanitizer-common if BUILD_ and HAS_ a dep #160962
Conversation
DanBlackwell
left a comment
DanBlackwell
left a comment
There was a problem hiding this comment.
I think adding the new toggle is a good idea. Can you please check whether the current boolean logic makes sense (I've added a comment).
Also, my understanding is that this change won't affect the default behaviour (i.e. the flag is ON by default), can you update the title of the PR to remove the 'by default' if that's the case.
compiler-rt/lib/CMakeLists.txt
Outdated
| @@ -8,7 +8,7 @@ include(SanitizerUtils) | |||
| # sanitizers or xray (or both). | |||
| # | |||
| #TODO: Refactor sanitizer_common into smaller pieces (e.g. flag parsing, utils). | |||
| if (COMPILER_RT_HAS_SANITIZER_COMMON AND | |||
| if (COMPILER_RT_HAS_SANITIZER_COMMON AND COMPILER_RT_BUILD_SANITIZER_COMMON AND | |||
| (COMPILER_RT_BUILD_SANITIZERS OR COMPILER_RT_BUILD_XRAY OR COMPILER_RT_BUILD_MEMPROF OR COMPILER_RT_BUILD_CTX_PROFILE)) | |||
There was a problem hiding this comment.
I presume that these options depend on having sanitizer_common built; should we instead be adding the new option to the 'OR'? i.e. is there a use case for building sanitizers/xray/memprof without sanitizer_common?
There was a problem hiding this comment.
So:
if (COMPILER_RT_HAS_SANITIZER_COMMON AND
(COMPILER_RT_BUILD_SANITIZER_COMMON OR COMPILER_RT_BUILD_SANITIZERS OR COMPILER_RT_BUILD_XRAY OR COMPILER_RT_BUILD_MEMPROF OR COMPILER_RT_BUILD_CTX_PROFILE))
There was a problem hiding this comment.
For most of these projects, it is a dependency. So the list of ORed projects can be read as "if we are configuring these other projects we probably need sanitizer common too". I think it's intended to be exhaustive.
What we are trying to do here is NOT configure sanitizer-common when we do not need it for the default build. I think the PR description explains why it's so important that we do that. The other option i guess is to force users do disable all of the things in the OR even if they are not available on their platform.
…_HAS_ a dep LLVM_ENABLE_RUNTIMES uses the default build target for building compiler-rt, which means a bunch of stuff gets built that isn't necessarily depended upon (and wasn't getting built under LLVM_ENABLE_PROJECTS). Right now, we configure sanitizer_common (and thus it gets built in the default build) based on COMPILER_RT_BUILD_* options. However, compiler_rt_build_runtime also uses the _HAS option to determine whether to configure a project. Thus, there are cases where we are configuring (and building with the default target) sanitizer-common when we do not need it. This change helps to slim down builds of compiler-rt that do not need sanitizer-common built.
ndrewh
force-pushed
the
compiler-rt-build-sanitizer-common
branch
from
803eb26 to
e2445ea
Compare
ndrewh
changed the title
|
Arguably, users should have to specify |
ndrewh
changed the title
3 participants
LLVM_ENABLE_RUNTIMES uses the default build target for building compiler-rt,
which means a bunch of stuff gets built that isn't necessarily depended upon
(and wasn't getting built under LLVM_ENABLE_PROJECTS).
Right now, we configure sanitizer_common (and thus it gets built in the default
build) based on COMPILER_RT_BUILD_* options. However, compiler_rt_build_runtime
also uses the _HAS option to determine whether to configure a runtime.
Thus, there are cases where we are configuring (and building with the default
target) sanitizer-common when we do not need it. This change helps to slim
down builds of compiler-rt that do not need sanitizer-common built.