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

[Justin Bogner via llvm-commits]

Chris Bieneman <beanz at apple.com> writes:
> 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.

Presumably "configured to support archs: ([^\n]*)\n" would do what you want.

>
> ================
> 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