[PATCH] D25157: [compiler-rt] [cmake] Respect COMPILER_RT_BUILD_* for libs, headers and tests

[Michał Górny via Phabricator via cfe-commits]

mgorny added inline comments.


================
Comment at: cmake/config-ix.cmake:438-441
 if (SANITIZER_COMMON_SUPPORTED_ARCH AND NOT LLVM_USE_SANITIZER AND
+    (COMPILER_RT_BUILD_SANITIZERS OR COMPILER_RT_BUILD_XRAY) AND
     (OS_NAME MATCHES "Android|Darwin|Linux|FreeBSD" OR
     (OS_NAME MATCHES "Windows" AND (NOT MINGW AND NOT CYGWIN))))
----------------
compnerd wrote:
> What does this really leave in terms of targets which the sanitizer supports but doesn't have the common library?
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.


================
Comment at: cmake/config-ix.cmake:562
   set(COMPILER_RT_HAS_XRAY FALSE)
 endif()
----------------
compnerd wrote:
> 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`).
Sounds like material for a separate patch to me.


================
Comment at: lib/CMakeLists.txt:60
+# the following set is conditional to COMPILER_RT_BUILD_XRAY
+# (via COMPILER_RT_HAS_* in config-ix.cmake)
+compiler_rt_build_runtime(xray)
----------------
compnerd wrote:
> Im not sure I understand the comment.  This looks like it may prevent disabling X-ray?
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.


https://reviews.llvm.org/D25157