[PATCH] D13059: [CMake] [Darwin] Bug 21562 - Add a CMake equivalent for make/platform/clang_darwin.mk in compiler_rt

[Alexey Samsonov via llvm-commits]

samsonov added inline comments.

================
Comment at: cmake/Modules/CompilerRTDarwinUtils.cmake:137
@@ +136,3 @@
+    ${ARGN})
+  set(libname "${name}_${LIB_ARCH}_${LIB_OS}")
+  add_library(${libname} STATIC ${LIB_SOURCES})
----------------
Any reason you can't delegate it to `add_compiler_rt_runtime` (with adding isysroot and builtin min-version flags to ${LIB_CFLAGS})?

================
Comment at: cmake/Modules/CompilerRTDarwinUtils.cmake:144
@@ +143,3 @@
+  set_property(TARGET ${libname} APPEND PROPERTY 
+              COMPILE_DEFINITIONS ${LIB_DEFS})
+  set_target_properties(${libname} PROPERTIES
----------------
LIB_DEFS is not defined, there's no DEFS argument. Just delete this property.

================
Comment at: cmake/Modules/CompilerRTDarwinUtils.cmake:155
@@ +154,3 @@
+
+macro(darwin_add_builtin_libraries)
+  foreach (os ${BUILTIN_SUPPORTED_OS})
----------------
Shouldn't you take the list of OS passed in ${ARGN}. I'm fine with either way, but please be consistent.

================
Comment at: cmake/Modules/CompilerRTDarwinUtils.cmake:161
@@ +160,3 @@
+    foreach (arch ${DARWIN_BUILTIN_ARCHS})
+      darwin_find_builtins_list(${os} ${arch} 6)
+      # Filter out generic versions of routines that are re-implemented in
----------------
Variable instead of magic constant 6?

================
Comment at: cmake/Modules/CompilerRTDarwinUtils.cmake:167
@@ +166,3 @@
+        get_filename_component(_name_we ${_file} NAME_WE)
+        list(FIND ${arch}_${os}_BUILTINS ${_name_we} _found)
+        if(_found GREATER -1)
----------------
So, the variable name here is confusing as well - it's actually a list of excluded builtins.

================
Comment at: lib/builtins/CMakeLists.txt:20
@@ -19,1 +19,3 @@
   ashrti3.c
+  atomic_flag_clear.c
+  atomic_flag_clear_explicit.c
----------------
It's interesting these files were excluded from the build. Can you add them in a separate commit to check if all compiler-rt users are ok with that?

================
Comment at: lib/builtins/CMakeLists.txt:318
@@ -311,41 +317,3 @@
 if (APPLE)
-  foreach (os ${BUILTIN_SUPPORTED_OS})
-    list_union(DARWIN_BUILTIN_ARCHS DARWIN_${os}_ARCHS BUILTIN_SUPPORTED_ARCH)
-    set(${os}_builtin_libs)
-    set(${os}_builtin_lipo_flags)
-    foreach (arch ${DARWIN_BUILTIN_ARCHS})
-      # Filter out generic versions of routines that are re-implemented in
-      # architecture specific manner.  This prevents multiple definitions of the
-      # same symbols, making the symbol selection non-deterministic.
-      foreach (_file ${${arch}_SOURCES})
-        if (${_file} MATCHES ${arch}/*)
-          get_filename_component(_name ${_file} NAME)
-          string(REPLACE ".S" ".c" _cname "${_name}")
-          list(REMOVE_ITEM ${arch}_SOURCES ${_cname})
-        endif ()
-      endforeach ()
-
-      add_compiler_rt_runtime(clang_rt.builtins_${arch} STATIC
-                              OS ${os}
-                              ARCHS ${arch}
-                              SOURCES ${${arch}_SOURCES}
-                              CFLAGS "-std=c99" ${DARWIN_${os}_CFLAGS} -arch ${arch}
-                              PARENT_TARGET builtins)
-      list(APPEND ${os}_builtin_libs clang_rt.builtins_${arch}_${os})
-      list(APPEND ${os}_builtin_lipo_flags -arch ${arch} 
-        ${COMPILER_RT_LIBRARY_OUTPUT_DIR}/libclang_rt.builtins_${arch}_${os}.a)
-    endforeach()
-
-    if(${os}_builtin_libs)
-      add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/libclang_rt.builtins_${os}.a
-        COMMAND lipo -output
-                ${COMPILER_RT_LIBRARY_OUTPUT_DIR}/libclang_rt.builtins_${os}.a
-                -create ${${os}_builtin_lipo_flags}
-        DEPENDS ${${os}_builtin_libs}
-        )
-      add_custom_target(clang_rt.builtins_${os}
-        DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/libclang_rt.builtins_${os}.a)
-      add_dependencies(builtins clang_rt.builtins_${os})
-    endif()
-  endforeach()
+  darwin_add_builtin_libraries($BUILTIN_SUPPORTED_OS})
 elseif (NOT WIN32 OR MINGW)
----------------
This argument is not used.

================
Comment at: lib/builtins/Darwin/README.TXT:9
@@ +8,3 @@
+supported target OS. Meaning if minimum deployment target is iOS 6, all builtins
+not included in the ios6-<arch>.txt files need to be included. The one catch is
+that this is per-architecture. Since iOS 6 doesn't support arm64, when supporting
----------------
I would clarify that .txt files contain files that should be *excluded* in the filename - `ios6-armv7.excluded.txt` or smth. like that.


http://reviews.llvm.org/D13059