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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
