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

[Chris Bieneman via llvm-commits]

beanz added a comment.

I'll send updated patches shortly. Comments inline below.


================
Comment at: cmake/Modules/CompilerRTDarwinUtils.cmake:31
@@ +30,3 @@
+
+  string(REGEX MATCH "configured to support archs: (.*)\nLTO support.*"
+         ARCHES_MATCHED "${LINKER_VERSION}")
----------------
filcab wrote:
> Nit: Do we care about having the "LTO support" string?
The "LTO Support" string is needed to escape the end of the regex. Since .* matches everything and nothing, and the behavior is to match as much as possible without the "LTO Support" string it matches the whole rest of the output.

I'm no regex expert though, so if you have a better suggestion I'm open.

================
Comment at: cmake/Modules/CompilerRTDarwinUtils.cmake:63
@@ +62,3 @@
+    list(REMOVE_ITEM archs "x86_64h")
+  endif()
+
----------------
filcab wrote:
> "will build", but then you remove that arch?
> Why is the `iossim` different from OS X, here?
I should explain better. The trivial test program does compile for x86_64h because it falls back to using the x86_64 libraries, but x86_64h isn't actually a valid simulator architecture, so other things will fail. I've had trouble finding a non-hacky way to make that detection work.

================
Comment at: cmake/config-ix.cmake:318
@@ +317,3 @@
+  
+  if(NOT MACOSX_VERSION_MIN_FLAG)
+    darwin_test_archs(osx
----------------
filcab wrote:
> What's the behavior you want if `MACOSX_VERSION_MIN_FLAG` is set? I'm guessing you're going with "just assume the user just wants OS X, since they passed that flag in the CXX flags".
> Shouldn't we at least test the OS X archs?
> 
> Why are we skipping iOS and iOS Simulator when we have a specific minimum OS X version? (I'm good with "so we don't change the user's CXX flags", but want to double-check)
The behavior if the user set macos-version-min explicitly is kinda complicated and probably broken. As it is now, I'm pretty sure we just don't build the sanitizers if you specify an explicit version min.

The rationale for skipping iOS and the simulator I assume is because we don't want to change the user's CXX flags, and there may be other incompatibilities. I don't know for sure though because that code exists in-tree today. This patch just moves it from the root CMakeLists.txt to config-ix as per samsonov's comments on previous reviews.

================
Comment at: cmake/config-ix.cmake:335
@@ +334,3 @@
+        ${DARWIN_COMMON_LINKFLAGS}
+        -Wl,-ios_simulator_version_min,7.0.0
+        -mios-simulator-version-min=7.0
----------------
filcab wrote:
> Does clang not pass the proper flag to the linker for the iOS Simulator (when the flag below is passed)?
> If not, are they both needed, still? Since clang should only be invoking the linker.
> Why didn't we need this for OS X? Why are the versions slightly different?
You're right. Since the driver should be passing this through we don't need it explicitly set.


http://reviews.llvm.org/D11820