[compiler-rt][macOS]: Disable iOS support if iOS SDK is not found

[thieta]

Summary

If you are missing the iOS SDK on your macOS (for example you don't have
full Xcode but just CommandLineTools) then CMake currently errors
out without a helpful message. This patch disables iOS support in
compiler-rt if the iOS SDK is not found. This can be overriden by
passing -DCOMPILER_RT_ENABLE_IOS=ON.

[thieta]

Removed whitespaces

[delcypher]

I don't think this is the right fix. Having a broken SDK setup is a real problem that should not be hidden by changing this to a warning.

As the original issue notes you can configure with -DCOMPILER_RT_ENABLE_IOS=OFF to explicitly not build compiler-rt for iOS.

If you wanted this to be more automatic then the right fix would be to compute the default for COMPILER_RT_ENABLE_IOS to be ON if the iOS SDK is present and OFF if it is not. So roughly something like

find_darwin_sdk_dir(has_ios_sdk ios)
if ("${has_ios_sdk}" STREQUAL "")
  set(COMPILER_RT_ENABLE_IOS_DEFAULT OFF)
else()
  set(COMPILER_RT_ENABLE_IOS_DEFAULT ON)
endif()

option(COMPILER_RT_ENABLE_IOS "Enable building for iOS" "${COMPILER_RT_ENABLE_IOS_DEFAULT}")

Doing it this way means that

• by default if there's no iOS SDK then the iOS compiler-rt build will be disabled
• if COMPILER_RT_ENABLE_IOS=ON is explicitly provided and there's no iOS SDK when we will still fail the build which is desirable.
• if COMPILER_RT_ENABLE_IOS=OFF is explicitly provided and there's an iOS SDK available then we will respect the explicit request to not build compiler-rt for iOS

[delcypher]

@yln @kubamracek @thetruestblue @rsundahl @wrotki Adding you as reviewers as I don't work on the Sanitizers much anymore.

[wrotki]

+1 to delcypher's suggestion

[thieta]

Addressed comments

[thieta]

A long time later, I finally got time to test this since it was marked as necessary to fix before 16.x release.

I tested this with both command line tools and xcode and it behaves correctly as far as I can figure out.

[delcypher]

@thieta Thanks for working on this. I don't think can land as is because I'm not convinced this works correctly. Please see my comments.

compiler-rt/cmake/base-config-ix.cmake

154

What is this ${DEFAULT} variable? Shouldn't this just be:

set(COMPILER_RT_ENABLE_IOS_DEFAULT ON)

?

158

Should this be

option(COMPILER_RT_ENABLE_IOS_DEFAULT "Enable building for iOS" ${COMPILER_RT_ENABLE_IOS_DEFAULT})

?

I.e.: Explicitly expand the variable?

Although if() will implicitly expand variables as noted here I'm not aware of such a behavior for option(). Even if this was supported I think we should explicitly expand the variables as is done elsewhere in this source file.

[thieta]

Whoops. Bad copy and paste job. I first made it into a macro and did it for all three SDKs and then realised watchos and tvos is off by default and don't need that probing. Then I probably messed it up when removing the macro. Pretty sure I tested it in this form though ... hmm. Anyway I agree with your comments and will address and retest it tomorrow.

[thetruestblue]

Thanks @delcypher for reviewing. Thanks @thieta for the work -- I'll keep an eye out for the updated diff, @delcypher doesn't work on Sanitizers as much anymore.

[thieta]

Addressed comments

[thieta]

I fixed the obvious errors and re-testing it and it seems to behave correctly now. Sorry for the noise.

[thetruestblue]

Nit: When you land this will you update commit message to reflect the new change?

This LGTM with the most recent changes. One additional Nit inline.

compiler-rt/cmake/base-config-ix.cmake

156

Nit: I'm wondering if there is any concern about this being a silent failure if there is no iOS SDK unexpectedly, as now we explicitly need to set COMPILER_RT_ENABLE_IOS=ON to catch a missing iOS SDK. Which is a change of behavior, correct?

Is it worth adding a compiler message or warning similar to the messages printed when looking for macosx sdk above to call out the change in what was previously "default".

message(WARNING "No iOS SDK found. Building for iOS not enabled.")

Seems like an inconvenient warning in the the small subset of cases where there is no iOS SDK, but could save a headache if there's an unexpectedly missing SDK.

[thieta]

Added warning when the iOS sdk is not found.
Reworded the commit message to reflect the current state of the patch

[thieta]

compiler-rt/cmake/base-config-ix.cmake

156

Seems like a warning might be good here. I'll add one.

[delcypher]

LGTM. Thanks for addressing my comments.

compiler-rt/cmake/base-config-ix.cmake

 # The CompilerRT build system requires CMake version 2.8.8 or higher in order
 # to use its support for building convenience "libraries" as a collection of
 # .o files. This is particularly useful in producing larger, more complex
 # runtime libraries.
 
 include(BuiltinTests)
 include(CheckIncludeFile)
 include(CheckCXXSourceCompiles)
 include(GNUInstallDirs)
 include(ExtendPath)
+include(CompilerRTDarwinUtils)
 
 check_include_file(unwind.h HAVE_UNWIND_H)
 
 # Used by sanitizer_common and tests.
 check_include_file(rpc/xdr.h HAVE_RPC_XDR_H)
 if (NOT HAVE_RPC_XDR_H)
   set(HAVE_RPC_XDR_H 0)
 endif()
if(APPLE)
   endif()
 
   try_compile_only(COMPILER_RT_HAS_DARWIN_TARGET_VARIANT_FLAG
                    FLAGS
                    "-target" "x86_64-apple-macos10.15"
                    "-darwin-target-variant" "x86_64-apple-ios13.1-macabi"
                    "-Werror")
   option(COMPILER_RT_ENABLE_MACCATALYST "Enable building for Mac Catalyst" ${COMPILER_RT_HAS_DARWIN_TARGET_VARIANT_FLAG})
-  option(COMPILER_RT_ENABLE_IOS "Enable building for iOS" On)
+
+  # Don't enable COMPILER_RT_ENABLE_IOS if we can't find the sdk dir.
+  # This can happen when you only have the commandline tools installed
+  # which doesn't come with the iOS SDK.
+  find_darwin_sdk_dir(HAS_IOS_SDK "iphoneos")
+  set(COMPILER_RT_ENABLE_IOS_DEFAULT On)
+  if("${HAS_IOS_SDK}" STREQUAL "")
+    message(WARNING "iOS SDK not found! Building compiler-rt without iOS support.")
+    set(COMPILER_RT_ENABLE_IOS_DEFAULT Off)
+  endif()
+  option(COMPILER_RT_ENABLE_IOS "Enable building for iOS" ${COMPILER_RT_ENABLE_IOS_DEFAULT})
+
   option(COMPILER_RT_ENABLE_WATCHOS "Enable building for watchOS - Experimental" Off)
   option(COMPILER_RT_ENABLE_TVOS "Enable building for tvOS - Experimental" Off)
 
 else()
   option(COMPILER_RT_DEFAULT_TARGET_ONLY "Build builtins only for the default target" Off)
 endif()
 
 if(WIN32 AND NOT MINGW AND NOT CYGWIN)