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

[Chris Bieneman via llvm-commits]

beanz added a comment.

Comments inline. Updated patches coming shortly.


================
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)
----------------
samsonov wrote:
> 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)
You are right. I will fix this.

================
Comment at: cmake/Modules/CompilerRTUtils.cmake:66
@@ +65,3 @@
+    if( NOT (index EQUAL -1))
+      set(${output} ${${output}} ${it})
+    endif()
----------------
samsonov wrote:
> Consider using
>   list(APPEND ${output} ${it})
You are right. I will fix this.

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

================
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)
----------------
samsonov wrote:
> s/aarch64/${ARM64}
You are right. I will fix this.

================
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}")
----------------
samsonov wrote:
> 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).
You are right. I will fix this.

================
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
----------------
samsonov wrote:
> Why not
>   list_union(ASAN_SUPPORTED_ARCH ALL_ASAN_SUPPORTED_ARCH SANITIZER_COMMON_SUPPORTED_ARCH)
> ?
No particular reason. I will fix this.


http://reviews.llvm.org/D11820