[PATCH] D12292: [CMake] merge add_compiler_rt_runtime and add_compiler_rt_darwin_runtime into a single function

[Alexey Samsonov via llvm-commits]

samsonov added inline comments.

================
Comment at: cmake/Modules/AddCompilerRT.cmake:59
@@ -54,18 +58,3 @@
 #                         DEFS <compile definitions>
-#                         OUTPUT_NAME <output library name>)
-macro(add_compiler_rt_runtime name arch type)
-  if(CAN_TARGET_${arch})
-    cmake_parse_arguments(LIB "" "OUTPUT_NAME" "SOURCES;CFLAGS;LINKFLAGS;DEFS" ${ARGN})
-    add_library(${name} ${type} ${LIB_SOURCES})
-    # Setup compile flags and definitions.
-    set_target_compile_flags(${name}
-      ${TARGET_${arch}_CFLAGS} ${LIB_CFLAGS})
-    set_target_link_flags(${name}
-      ${TARGET_${arch}_CFLAGS} ${LIB_CFLAGS} ${LIB_LINKFLAGS})
-    set_property(TARGET ${name} APPEND PROPERTY
-      COMPILE_DEFINITIONS ${LIB_DEFS})
-    # Setup correct output directory in the build tree.
-    set_target_properties(${name} PROPERTIES
-      ARCHIVE_OUTPUT_DIRECTORY ${COMPILER_RT_LIBRARY_OUTPUT_DIR}
-      LIBRARY_OUTPUT_DIRECTORY ${COMPILER_RT_LIBRARY_OUTPUT_DIR}
-      RUNTIME_OUTPUT_DIRECTORY ${COMPILER_RT_LIBRARY_OUTPUT_DIR})
+#                         LIBS <linked libraries>
+#                         OUTPUT_NAME <output library name>
----------------
LIBS->LINK_LIBS (specify that it makes sense for shared libraries only)

================
Comment at: cmake/Modules/AddCompilerRT.cmake:63
@@ +62,3 @@
+function(add_compiler_rt_runtime name)
+  cmake_parse_arguments(LIB "SHARED;STATIC"
+    "OUTPUT_NAME;PARENT_TARGET"
----------------
I would leave SHARED/STATIC as `type` argument of `add_compiler_rt_runtime` function. Otherwise you can specify both STATIC and SHARED, which makes no sense. You should also diagnose if none is specified.

================
Comment at: cmake/Modules/AddCompilerRT.cmake:77
@@ +76,3 @@
+          set(extra_linkflags_${libname} ${DARWIN_${os}_LINKFLAGS} ${LIB_LINKFLAGS})
+          set(type_${libname} STATIC)
+        endif()
----------------
Looks like type_${libname} will be the same for all ${libname}, you don't really need it now.

================
Comment at: cmake/Modules/AddCompilerRT.cmake:101
@@ +100,3 @@
+        set(extra_cflags_${libname} ${TARGET_${arch}_CFLAGS} ${LIB_CFLAGS})
+        set(extra_linkflags_${libname} ${TARGET_${arch}_LINKFLAGS} ${LIB_LINKFLAGS})
+        set(type_${libname} STATIC)
----------------
Note that currently we pass LIB_CFLAGS to as linkflags as well, you probably want to keep this behavior for now.

================
Comment at: cmake/Modules/AddCompilerRT.cmake:101
@@ +100,3 @@
+        set(extra_cflags_${libname} ${TARGET_${arch}_CFLAGS} ${LIB_CFLAGS})
+        set(extra_linkflags_${libname} ${TARGET_${arch}_LINKFLAGS} ${LIB_LINKFLAGS})
+        set(type_${libname} STATIC)
----------------
samsonov wrote:
> Note that currently we pass LIB_CFLAGS to as linkflags as well, you probably want to keep this behavior for now.
There is no such thing as ${TARGET_${arch}_LINKFLAGS}

================
Comment at: cmake/Modules/AddCompilerRT.cmake:103
@@ +102,3 @@
+        set(type_${libname} STATIC)
+        if(NOT CAN_TARGET_${arch})
+          message(FATAL_ERROR "Architecture ${arch} can't be targeted")
----------------
You've already checked for that.

================
Comment at: cmake/Modules/AddCompilerRT.cmake:114
@@ +113,3 @@
+        set(type_${libname} SHARED)
+        if(NOT CAN_TARGET_${arch})
+          message(FATAL_ERROR "Architecture ${arch} can't be targeted")
----------------
You've already checked for that.

================
Comment at: cmake/Modules/AddCompilerRT.cmake:121
@@ +120,3 @@
+  endif()
+
+  if(LIB_PARENT_TARGET AND libnames)
----------------
You can probably do
  if(NOT libnames)
    return()
  endif()

and skip all the checks below.

================
Comment at: cmake/Modules/AddCompilerRT.cmake:123
@@ +122,3 @@
+  if(LIB_PARENT_TARGET AND libnames)
+    set(COMONENT_OPTION COMPONENT ${LIB_PARENT_TARGET})
+  endif()
----------------
COMPONENT_OPTION

================
Comment at: cmake/Modules/AddCompilerRT.cmake:141
@@ -77,2 +140,3 @@
+      set_target_properties(${libname} PROPERTIES
         OUTPUT_NAME ${LIB_OUTPUT_NAME})
     endif()
----------------
This whole OUTPUT_NAME logic looks terrible now, especially assuming you can have several ${libname} libraries, but you pass a single OUTPUT_NAME for them. Ugh. Looks like it's only really used on Windows, maybe you will just adjust for naming difference on Windows and Linux at the place where you generate ${libname}, and get rid of this argument?

================
Comment at: cmake/Modules/AddCompilerRT.cmake:143
@@ -78,3 +142,3 @@
     endif()
-    # Add installation command.
-    install(TARGETS ${name}
+    if(LIB_LIBS)
+      target_link_libraries(${libname} ${LIB_LIBS})
----------------
Only if we're linking SHARED compiler-rt runtime?

================
Comment at: lib/dfsan/CMakeLists.txt:25
@@ -22,3 +24,3 @@
             $<TARGET_OBJECTS:RTSanitizerCommonLibc.${arch}>
     CFLAGS ${DFSAN_CFLAGS})
   add_sanitizer_rt_symbols(clang_rt.dfsan-${arch} dfsan.syms.extra)
----------------
Why don't you use PARENT_TARGET here (and in similar places below)?


http://reviews.llvm.org/D12292