[PATCH] D11820: [CMake] Add experimental support for building compiler-rt for iOS

[Alexey Samsonov via llvm-commits]

samsonov added a comment.

Sorry for the late response! This actually looks mostly good, I've got several minor comments.


================
Comment at: cmake/Modules/AddCompilerRT.cmake:22
@@ -21,1 +21,3 @@
       set(extra_cflags_${libname} ${DARWIN_${os}_CFLAGS})
+      set(extra_linkflags_${libname} ${DARWIN_${os}_LINKFLAGS})
+      list_union(LIB_ARCHS_${libname} DARWIN_${os}_ARCHS LIB_ARCHS)
----------------
object library is just a collection of .o files. Why do you need link flags here?
(sorry if you've already replied me and I failed to notice it)

================
Comment at: cmake/Modules/CompilerRTDarwinUtils.cmake:80
@@ +79,1 @@
+endfunction()
\ No newline at end of file

----------------
Please fix

================
Comment at: cmake/Modules/CompilerRTUtils.cmake:66
@@ +65,3 @@
+    if( NOT (index EQUAL -1))
+      set(${output} ${${output}} ${it})
+    endif()
----------------
Consider using
  list(APPEND ${output} ${it})

================
Comment at: cmake/config-ix.cmake:176
@@ -175,3 +175,3 @@
   set(COMPILER_RT_OS_SUFFIX "-android")
 else()
   if("${LLVM_NATIVE_ARCH}" STREQUAL "X86")
----------------
Consider replacing it with
  elseif(NOT APPLE)
with a comment that `COMPILER_RT_SUPPORTED_ARCH` list for Apple platforms is generated below.

================
Comment at: cmake/config-ix.cmake:262
@@ +261,3 @@
+set(ALL_PROFILE_SUPPORTED_ARCH ${X86_64} i386 i686 ${ARM32} mips mips64
+    mipsel mips64el aarch64 powerpc64 powerpc64le)
+set(ALL_TSAN_SUPPORTED_ARCH ${X86_64} mips64 mips64el)
----------------
s/aarch64/${ARM64}

================
Comment at: cmake/config-ix.cmake:280
@@ +279,3 @@
+  set(SANITIZER_COMMON_SUPPORTED_OS osx)
+  string(REGEX MATCH "-mmacosx-version-min=([.0-9]+)"
+         MACOSX_VERSION_MIN_FLAG "${CMAKE_CXX_FLAGS}")
----------------
Looks like some part was lost in rebase. You need to put the block below under
  if(NOT SANITIZER_MIN_OSX_VERSION)
so that the user can manually specify `-DSANITIZER_MIN_OSX_VERSION` at configure time (I was specifically asked to add this feature by Chromium folks).

(you might still look for `MACOSX_VERSION_MIN_FLAG`, because you're using it below).

================
Comment at: cmake/config-ix.cmake:380
@@ +379,3 @@
+  set(UBSAN_COMMON_SUPPORTED_ARCH ${SANITIZER_COMMON_SUPPORTED_ARCH})
+  set(ASAN_SUPPORTED_ARCH ${SANITIZER_COMMON_SUPPORTED_ARCH})
+  list_union(DFSAN_SUPPORTED_ARCH
----------------
Why not
  list_union(ASAN_SUPPORTED_ARCH ALL_ASAN_SUPPORTED_ARCH SANITIZER_COMMON_SUPPORTED_ARCH)
?


http://reviews.llvm.org/D11820