[PATCH] D14846: [CMake] Provide options for toggling on and off various runtime libraries.

[Alexey Samsonov via llvm-commits]

samsonov added a comment.

Looks reasonable, several comments/nits below.


================
Comment at: cmake/Modules/CompilerRTUtils.cmake:81
@@ +80,3 @@
+
+macro(is_in_list outvar list input)
+  list(FIND ${list} ${input} idx)
----------------
I'm curious, can we make it look more like "check_c_compiler_flag" or "check_symbol_exists"
  check_list_contains(list value output)
?

================
Comment at: cmake/Modules/CompilerRTUtils.cmake:84
@@ +83,3 @@
+  if(idx GREATER -1)
+    set(${outvar} On)
+  else()
----------------
use True/False instead?

================
Comment at: cmake/Modules/CompilerRTUtils.cmake:90
@@ +89,3 @@
+
+macro(is_runtime_building outvar runtime)
+  is_in_list(${outvar} COMPILER_RT_RUNTIMES_TO_BUILD ${runtime})
----------------
See note below - probably remove this?

================
Comment at: cmake/config-ix.cmake:562
@@ -574,1 +561,3 @@
+if(OS_NAME MATCHES "Windows")
+  list(APPEND DEFAULT_RUNTIMES cfi)
 endif()
----------------
cfi is available not only on Windows, I think lib/cfi is added unconditionally.

================
Comment at: cmake/config-ix.cmake:588
@@ +587,3 @@
+
+if(SHOULD_BUILD_COMMON)
+  append_list_unique(COMPILER_RT_RUNTIMES_TO_BUILD sanitizer_common)
----------------
Please describe what you're doing here.

================
Comment at: lib/CMakeLists.txt:12
@@ -11,37 +11,3 @@
 if(COMPILER_RT_BUILD_SANITIZERS)
-  if(COMPILER_RT_HAS_INTERCEPTION)
-    add_subdirectory(interception)
-  endif()
-
-  if(COMPILER_RT_HAS_SANITIZER_COMMON)
-    add_subdirectory(sanitizer_common)
-    add_subdirectory(lsan)
-    add_subdirectory(ubsan)
-  endif()
-
-  add_subdirectory(cfi)
-
-  if(COMPILER_RT_HAS_ASAN)
-    add_subdirectory(asan)
-  endif()
-
-  if(COMPILER_RT_HAS_DFSAN)
-    add_subdirectory(dfsan)
-  endif()
-
-  if(COMPILER_RT_HAS_MSAN)
-    add_subdirectory(msan)
-  endif()
-
-  if(COMPILER_RT_HAS_PROFILE)
-    add_subdirectory(profile)
-  endif()
-
-  if(COMPILER_RT_HAS_TSAN)
-    add_subdirectory(tsan)
-    add_subdirectory(tsan/dd)
-  endif()
-
-  if(COMPILER_RT_HAS_SAFESTACK)
-    add_subdirectory(safestack)
-  endif()
+  message(STATUS "Adding runtimes...")
+  foreach(runtime ${COMPILER_RT_RUNTIMES_TO_BUILD})
----------------
Is it a debug output?

================
Comment at: test/CMakeLists.txt:40
@@ +39,3 @@
+  foreach(runtime ${COMPILER_RT_RUNTIMES_TO_BUILD})
+    add_subdirectory(${runtime})
+  endforeach()
----------------
How do you handle CFI special case here?

================
Comment at: test/ubsan/CMakeLists.txt:23
@@ -22,1 +22,3 @@
 
+is_runtime_building(HAS_ASAN asan)
+is_runtime_building(HAS_MSAN msan)
----------------
I actually think this macro is an overkill, and would prefer literal mentioning of COMPILER_RT_RUNTIMES_TO_BUILD here. Sorry for the nit.


http://reviews.llvm.org/D14846