[CMake] Fix the value of `config.target_cflags` for non-macOS Apple platforms. Attempt #3.

[delcypher]

Summary

The main problem here is that -*-version_min= was not being passed to
the compiler when building test cases. This can cause problems when
testing on devices running older OSs because Clang would previously
assume the minimum deployment target is the the latest OS in the SDK
which could be much newer than what the device is running.

Previously the generated value looked like this:

`-arch arm64 -isysroot
<path_to_xcode>/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS12.1.sdk`

With this change it now looks like:

`-arch arm64 -stdlib=libc++ -miphoneos-version-min=8.0 -isysroot
<path_to_xcode>/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS12.1.sdk`

This mirrors the setting of config.target_cflags on macOS.

This change is made for ASan, LibFuzzer, TSan, and UBSan.

To implement this a new get_test_cflags_for_apple_platform() function
has been added that when given an Apple platform name and architecture
returns a string containing the C compiler flags to use when building
tests. This also calls a new helper function is_valid_apple_platform()
that validates Apple platform names.

This is the third attempt at landing the patch.

The first attempt (r359305) had to be reverted (r359327) due to a buildbot
failure. The problem was that calling get_test_cflags_for_apple_platform()
can trigger a CMake error if the provided architecture is not supported by the
current CMake configuration. Previously, this could be triggered by passing
-DCOMPILER_RT_ENABLE_IOS=OFF to CMake. The root cause is that we were
generating test configurations for a list of architectures without checking if
the relevant Sanitizer actually supported that architecture. We now intersect
the list of architectures for an Apple platform with
<SANITIZER>_SUPPORTED_ARCH (where <SANITIZER> is a Sanitizer name) to
iterate through the correct list of architectures.

The second attempt (r363633) had to be reverted (r363779) due to a build
failure. The failed build was using a modified Apple toolchain where the iOS
simulator SDK was missing. This exposed a bug in the existing UBSan test
generation code where it was assumed that COMPILER_RT_ENABLE_IOS implied that
the toolchain supported both iOS and the iOS simulator. This is not true. This
has been fixed by using the list SANITIZER_COMMON_SUPPORTED_OS for the list
of supported Apple platforms for UBSan. For consistency with the other
Sanitizers we also now intersect the list of architectures with
UBSAN_SUPPORTED_ARCH.

rdar://problem/50124489

[delcypher]

@hans @thakis Could one of you check that this patch doesn't break your build?

[hans]

In D63674#1554533, @delcypher wrote:

@hans @thakis Could one of you check that this patch doesn't break your build?

We don't get a CMake error anymore, but the build no longer produces lib/clang/9.0.0/lib/darwin/libclang_rt.asan_iossim_dynamic.dylib after this patch.

[delcypher]

@hans

In D63674#1555149, @hans wrote:

In D63674#1554533, @delcypher wrote:

@hans @thakis Could one of you check that this patch doesn't break your build?

We don't get a CMake error anymore,

Great!

but the build no longer produces lib/clang/9.0.0/lib/darwin/libclang_rt.asan_iossim_dynamic.dylib after this patch.

This patch doesn't change (AFAICT) which Apple OSs get built for. It only changes the code that generates test configurations for the different sanitizers. AFAICT it is code already in trunk (cmake/config-ix.cmake) that decides if compiler-rt will build for the iossim OS. The code requires the iOS simulator SDKs to be present in your toolchain. If you don't have the iOS simulator SDK I don't think the ASan iossim dylib will be created. I don't understand how you managed to produce the lib/clang/9.0.0/lib/darwin/libclang_rt.asan_iossim_dynamic.dylib file previously because it should not be possible without the iOS simulator SDK.

The only thing I can think of is your bot was doing an incremental build and your lib/clang/9.0.0/lib/darwin/libclang_rt.asan_iossim_dynamic.dylib file is a stale file from an old build where creation of lib/clang/9.0.0/lib/darwin/libclang_rt.asan_iossim_dynamic.dylib was possible.

I don't think lib/clang/9.0.0/lib/darwin/libclang_rt.asan_iossim_dynamic.dylib being missing is a bug here so I think we should try to land this change to see if it breaks anyone else.

[thakis]

In D63674#1556504, @delcypher wrote:

@hans

In D63674#1555149, @hans wrote:

In D63674#1554533, @delcypher wrote:

@hans @thakis Could one of you check that this patch doesn't break your build?

We don't get a CMake error anymore,

Great!

but the build no longer produces lib/clang/9.0.0/lib/darwin/libclang_rt.asan_iossim_dynamic.dylib after this patch.

This patch doesn't change (AFAICT) which Apple OSs get built for. It only changes the code that generates test configurations for the different sanitizers. AFAICT it is code already in trunk (cmake/config-ix.cmake) that decides if compiler-rt will build for the iossim OS. The code requires the iOS simulator SDKs to be present in your toolchain. If you don't have the iOS simulator SDK I don't think the ASan iossim dylib will be created. I don't understand how you managed to produce the lib/clang/9.0.0/lib/darwin/libclang_rt.asan_iossim_dynamic.dylib file previously because it should not be possible without the iOS simulator SDK.

The only thing I can think of is your bot was doing an incremental build and your lib/clang/9.0.0/lib/darwin/libclang_rt.asan_iossim_dynamic.dylib file is a stale file from an old build where creation of lib/clang/9.0.0/lib/darwin/libclang_rt.asan_iossim_dynamic.dylib was possible.

(FWIW our bots all do clobber builds, so this can't be it.)

[delcypher]

@thakis @hans

(FWIW our bots all do clobber builds, so this can't be it.)

So how do we progress from here? What you're describing to me (no iOS simulator asan dylib if iOS simulator SDKs not present in toolchain) is expected behaviour. The issue you mention with the iOS simulator asan dylib no longer being produced does sound very curious. However you are using a modified toolchain that I do not have so I cannot reproduce the change in behaviour. Does this behaviour change matter to you?

If not, I think we should just land this patch. If it does matter then please be aware that this was the intended behaviour and the fact it used to work is an accident. If necessary some time could be invested into to figuring out why the behaviour changed but either you would need to investigate this yourself or provide a copy of the toolchain you are using so I can investigate.

[delcypher]

@thakis @hans Ping.

[hans]

I'm very sorry for the slow reply.

So how do we progress from here? What you're describing to me (no iOS simulator asan dylib if iOS simulator SDKs not present in toolchain) is expected behaviour. The issue you mention with the iOS simulator asan dylib no longer being produced does sound very curious. However you are using a modified toolchain that I do not have so I cannot reproduce the change in behaviour. Does this behaviour change matter to you?

Yes, because we use that ios simulator asan dylib.

If not, I think we should just land this patch. If it does matter then please be aware that this was the intended behaviour and the fact it used to work is an accident. If necessary some time could be invested into to figuring out why the behaviour changed but either you would need to investigate this yourself or provide a copy of the toolchain you are using so I can investigate.

I think the way forward is for us to just make sure we have the full xcode available when building our clang package. But we need to figure out how to coax our infrastructure into making that happen..

[delcypher]

I think the way forward is for us to just make sure we have the full xcode available when building our clang package. But we need to figure out how to coax our infrastructure into making that happen..

So is this fixed yet? There's no good reason for us to support your modified (broken) toolchain if there's no intention of fixing issues with it.

[thakis]

In D63674#1609148, @delcypher wrote:

I think the way forward is for us to just make sure we have the full xcode available when building our clang package. But we need to figure out how to coax our infrastructure into making that happen..

So is this fixed yet? There's no good reason for us to support your modified (broken) toolchain if there's no intention of fixing issues with it.

I'll try to look at this today.

It's not a modified toolchain, it's just Xcode without an iOS SDK.

In D63674#1609148, @delcypher wrote:

I think the way forward is for us to just make sure we have the full xcode available when building our clang package. But we need to figure out how to coax our infrastructure into making that happen..

So is this fixed yet? There's no good reason for us to support your modified (broken) toolchain if there's no intention of fixing issues with it.

We've now updated our clang-building mac bots to have both a mac and an ios sdk. Let's reland this and see what happens. Thank you very much for your patience.

[delcypher]

Rebase on master.

[delcypher]

This already landed in r373405

cmake/config-ix.cmake

else()
     get_target_flags_for_arch(${arch} ${cflags_out})
     if(APPLE)
       list(APPEND ${cflags_out} ${DARWIN_osx_CFLAGS})
     endif()
     string(REPLACE ";" " " ${cflags_out} "${${cflags_out}}")
   endif()
 endmacro()
 
+# Returns CFLAGS that should be used to run tests for the
+# specific apple platform and architecture.
+function(get_test_cflags_for_apple_platform platform arch cflags_out)
+  is_valid_apple_platform("${platform}" is_valid_platform)
+  if (NOT is_valid_platform)
+    message(FATAL_ERROR "\"${platform}\" is not a valid apple platform")
+  endif()
+  set(test_cflags "")
+  get_target_flags_for_arch(${arch} test_cflags)
+  list(APPEND test_cflags ${DARWIN_${platform}_CFLAGS})
+  string(REPLACE ";" " " test_cflags_str "${test_cflags}")
+  string(APPEND test_cflags_str "${COMPILER_RT_TEST_COMPILER_CFLAGS}")
+  set(${cflags_out} "${test_cflags_str}" PARENT_SCOPE)
+endfunction()
+
+function(is_valid_apple_platform platform is_valid_out)
+  set(is_valid FALSE)
+  if ("${platform}" STREQUAL "")
+    message(FATAL_ERROR "platform cannot be empty")
+  endif()
+  if ("${platform}" MATCHES "^(osx|((ios|watchos|tvos)(sim)?))$")
+    set(is_valid TRUE)
+  endif()
+  set(${is_valid_out} ${is_valid} PARENT_SCOPE)
+endfunction()
+
 set(ARM64 aarch64)
 set(ARM32 arm armhf)
 set(HEXAGON hexagon)
 set(X86 i386)
 set(X86_64 x86_64)
 set(MIPS32 mips mipsel)
 set(MIPS64 mips64 mips64el)
 set(PPC64 powerpc64 powerpc64le)

test/asan/CMakeLists.txt

 endforeach()
 
 # iOS and iOS simulator test suites
 # These are not added into "check-all", in order to run these tests, use
 # "check-asan-iossim-x86_64" and similar. They also require that an extra env
 # variable to select which iOS device or simulator to use, e.g.:
 # SANITIZER_IOSSIM_TEST_DEVICE_IDENTIFIER="iPhone 6"
 if(APPLE)
+  # FIXME(dliew): This logic should be refactored to the way UBSan Darwin
+  # testing is done.
   set(EXCLUDE_FROM_ALL ON)
 
   set(ASAN_TEST_TARGET_CC ${COMPILER_RT_TEST_COMPILER})
   set(ASAN_TEST_DYNAMIC True)
 
-  foreach(arch ${DARWIN_iossim_ARCHS})
+  list_intersect(ASAN_TEST_IOSSIM_ARCHS ASAN_SUPPORTED_ARCH DARWIN_iossim_ARCHS)
+  foreach(arch ${ASAN_TEST_IOSSIM_ARCHS})
     set(ASAN_TEST_APPLE_PLATFORM "iossim")
     set(ASAN_TEST_TARGET_ARCH ${arch})
-    set(ASAN_TEST_TARGET_CFLAGS "-arch ${arch} -isysroot ${DARWIN_iossim_SYSROOT} ${COMPILER_RT_TEST_COMPILER_CFLAGS}")
+    get_test_cflags_for_apple_platform(
+      "${ASAN_TEST_APPLE_PLATFORM}"
+      "${ASAN_TEST_TARGET_ARCH}"
+      ASAN_TEST_TARGET_CFLAGS
+      )
     set(ASAN_TEST_CONFIG_SUFFIX "-${arch}-${ASAN_TEST_APPLE_PLATFORM}")
     get_bits_for_arch(${arch} ASAN_TEST_BITS)
     string(TOUPPER ${arch} ARCH_UPPER_CASE)
     set(CONFIG_NAME "IOSSim${ARCH_UPPER_CASE}Config")
     configure_lit_site_cfg(
       ${CMAKE_CURRENT_SOURCE_DIR}/lit.site.cfg.py.in
       ${CMAKE_CURRENT_BINARY_DIR}/${CONFIG_NAME}/lit.site.cfg.py
       )
     add_lit_testsuite(check-asan-iossim-${arch} "AddressSanitizer iOS Simulator ${arch} tests"
       ${CMAKE_CURRENT_BINARY_DIR}/${CONFIG_NAME}/
       DEPENDS ${ASAN_TEST_DEPS})
   endforeach()
 
-  foreach (arch ${DARWIN_ios_ARCHS})
+  list_intersect(ASAN_TEST_IOS_ARCHS ASAN_SUPPORTED_ARCH DARWIN_ios_ARCHS)
+  foreach (arch ${ASAN_TEST_IOS_ARCHS})
     set(ASAN_TEST_APPLE_PLATFORM "ios")
     set(ASAN_TEST_TARGET_ARCH ${arch})
-    set(ASAN_TEST_TARGET_CFLAGS "-arch ${arch} -isysroot ${DARWIN_ios_SYSROOT} ${COMPILER_RT_TEST_COMPILER_CFLAGS}")
+    get_test_cflags_for_apple_platform(
+      "${ASAN_TEST_APPLE_PLATFORM}"
+      "${arch}"
+      ASAN_TEST_TARGET_CFLAGS)
     set(ASAN_TEST_CONFIG_SUFFIX "-${arch}-${ASAN_TEST_APPLE_PLATFORM}")
     get_bits_for_arch(${arch} ASAN_TEST_BITS)
     string(TOUPPER ${arch} ARCH_UPPER_CASE)
     set(CONFIG_NAME "IOS${ARCH_UPPER_CASE}Config")
     configure_lit_site_cfg(
       ${CMAKE_CURRENT_SOURCE_DIR}/lit.site.cfg.py.in
       ${CMAKE_CURRENT_BINARY_DIR}/${CONFIG_NAME}/lit.site.cfg.py
       )

test/fuzzer/CMakeLists.txt

if(TARGET cxx_shared)
     test_fuzzer("libc++" DEPS cxx_shared)
   endif()
   if(TARGET cxx_static)
     test_fuzzer("static-libc++" DEPS cxx_static)
   endif()
 endif()
 
 if (APPLE)
+  # FIXME(dliew): This logic should be refactored to the way UBSan Darwin
+  # testing is done.
   set(EXCLUDE_FROM_ALL ON)
 
-  foreach(arch ${DARWIN_ios_ARCHS})
+  list_intersect(FUZZER_TEST_IOS_ARCHS FUZZER_SUPPORTED_ARCH DARWIN_ios_ARCHS)
+  foreach(arch ${FUZZER_TEST_IOS_ARCHS})
     set(LIBFUZZER_TEST_APPLE_PLATFORM "ios")
     set(LIBFUZZER_TEST_TARGET_ARCH ${arch})
-    set(LIBFUZZER_TEST_FLAGS "-arch ${arch} -isysroot ${DARWIN_ios_SYSROOT} ${COMPILER_RT_TEST_COMPILER_CFLAGS}")
+    get_test_cflags_for_apple_platform(
+      "${LIBFUZZER_TEST_APPLE_PLATFORM}"
+      "${LIBFUZZER_TEST_TARGET_ARCH}"
+      LIBFUZZER_TEST_FLAGS
+      )
     set(LIBFUZZER_TEST_CONFIG_SUFFIX "-${arch}-${LIBFUZZER_TEST_APPLE_PLATFORM}")
     string(TOUPPER ${arch} ARCH_UPPER_CASE)
     set(CONFIG_NAME "IOS${ARCH_UPPER_CASE}Config")
     configure_lit_site_cfg(
       ${CMAKE_CURRENT_SOURCE_DIR}/lit.site.cfg.py.in
       ${CMAKE_CURRENT_BINARY_DIR}/${CONFIG_NAME}/lit.site.cfg.py
       )
     add_lit_testsuite(check-fuzzer-ios-${arch} "libFuzzer iOS ${arch} tests"
       ${CMAKE_CURRENT_BINARY_DIR}/${CONFIG_NAME}/
       DEPENDS ${LIBFUZZER_TEST_DEPS})
 
   endforeach()
 
   set(EXCLUDE_FROM_ALL OFF)
 endif()

test/tsan/CMakeLists.txt

 endforeach()
 
 # iOS and iOS simulator test suites
 # These are not added into "check-all", in order to run these tests, use
 # "check-tsan-iossim-x86_64" and similar. They also require an extra environment
 # variable to select which iOS device or simulator to use, e.g.:
 # SANITIZER_IOSSIM_TEST_DEVICE_IDENTIFIER="iPhone 6"
 if(APPLE)
+  # FIXME(dliew): This logic should be refactored to the way UBSan Darwin
+  # testing is done.
   set(EXCLUDE_FROM_ALL ON)
-
   set(TSAN_TEST_TARGET_CC ${COMPILER_RT_TEST_COMPILER})
 
+  list_intersect(TSAN_TEST_IOSSIM_ARCHS TSAN_SUPPORTED_ARCH DARWIN_iossim_ARCHS)
+  foreach(arch ${TSAN_TEST_IOSSIM_ARCHS})
     set(TSAN_TEST_APPLE_PLATFORM "iossim")
-  set(arch "x86_64")
     set(TSAN_TEST_TARGET_ARCH ${arch})
-  set(TSAN_TEST_TARGET_CFLAGS "-arch ${arch} -isysroot ${DARWIN_iossim_SYSROOT} ${COMPILER_RT_TEST_COMPILER_CFLAGS}")
+    get_test_cflags_for_apple_platform(
+      "${TSAN_TEST_APPLE_PLATFORM}"
+      "${TSAN_TEST_TARGET_ARCH}"
+      TSAN_TEST_TARGET_CFLAGS
+      )
     set(TSAN_TEST_CONFIG_SUFFIX "-${arch}-${TSAN_TEST_APPLE_PLATFORM}")
     string(TOUPPER ${arch} ARCH_UPPER_CASE)
     set(CONFIG_NAME "IOSSim${ARCH_UPPER_CASE}Config")
     configure_lit_site_cfg(
       ${CMAKE_CURRENT_SOURCE_DIR}/lit.site.cfg.py.in
       ${CMAKE_CURRENT_BINARY_DIR}/${CONFIG_NAME}/lit.site.cfg.py
       )
     add_lit_testsuite(check-tsan-iossim-${arch} "ThreadSanitizer iOS Simulator ${arch} tests"
       ${CMAKE_CURRENT_BINARY_DIR}/${CONFIG_NAME}/
       DEPENDS ${TSAN_TEST_DEPS})
+  endforeach()
 
+  list_intersect(TSAN_TEST_IOS_ARCHS TSAN_SUPPORTED_ARCH DARWIN_ios_ARCHS)
+  foreach(arch ${TSAN_TEST_IOS_ARCHS})
     set(TSAN_TEST_APPLE_PLATFORM "ios")
-  set(arch "arm64")
     set(TSAN_TEST_TARGET_ARCH ${arch})
-  set(TSAN_TEST_TARGET_CFLAGS "-arch ${arch} -isysroot ${DARWIN_ios_SYSROOT} ${COMPILER_RT_TEST_COMPILER_CFLAGS}")
+    get_test_cflags_for_apple_platform(
+      "${TSAN_TEST_APPLE_PLATFORM}"
+      "${TSAN_TEST_TARGET_ARCH}"
+      TSAN_TEST_TARGET_CFLAGS
+      )
     set(TSAN_TEST_CONFIG_SUFFIX "-${arch}-${TSAN_TEST_APPLE_PLATFORM}")
     string(TOUPPER ${arch} ARCH_UPPER_CASE)
     set(CONFIG_NAME "IOS${ARCH_UPPER_CASE}Config")
     configure_lit_site_cfg(
       ${CMAKE_CURRENT_SOURCE_DIR}/lit.site.cfg.py.in
       ${CMAKE_CURRENT_BINARY_DIR}/${CONFIG_NAME}/lit.site.cfg.py
       )
-  add_lit_testsuite(check-tsan-ios-${arch} "ThreadSanitizer iOS Simulator ${arch} tests"
+    add_lit_testsuite(check-tsan-ios-${arch} "ThreadSanitizer iOS ${arch} tests"
       ${CMAKE_CURRENT_BINARY_DIR}/${CONFIG_NAME}/
       DEPENDS ${TSAN_TEST_DEPS})
+  endforeach()
 
   set(EXCLUDE_FROM_ALL OFF)
 endif()
 
 if(COMPILER_RT_INCLUDE_TESTS)
   configure_lit_site_cfg(
     ${CMAKE_CURRENT_SOURCE_DIR}/Unit/lit.site.cfg.py.in
     ${CMAKE_CURRENT_BINARY_DIR}/Unit/lit.site.cfg.py)
   list(APPEND TSAN_TEST_DEPS TsanUnitTests)
   list(APPEND TSAN_TESTSUITES ${CMAKE_CURRENT_BINARY_DIR}/Unit)
 endif()
 
 add_lit_testsuite(check-tsan "Running ThreadSanitizer tests"
   ${TSAN_TESTSUITES}
   DEPENDS ${TSAN_TEST_DEPS})
 set_target_properties(check-tsan PROPERTIES FOLDER "Compiler-RT Tests")

test/ubsan/CMakeLists.txt

if(APPLE)
   endforeach()
 
   # Device and simulator test suites.
   # These are not added into "check-all", in order to run these tests, use
   # "check-asan-iossim-x86_64" and similar. They also require that an extra env
   # variable to select which iOS device or simulator to use, e.g.:
   # SANITIZER_IOSSIM_TEST_DEVICE_IDENTIFIER="iPhone 6"
   set(EXCLUDE_FROM_ALL ON)
-  set(UBSAN_APPLE_PLATFORMS "")
-  if (COMPILER_RT_ENABLE_IOS)
-    list(APPEND UBSAN_APPLE_PLATFORMS ios iossim)
-  endif()
-  if (COMPILER_RT_ENABLE_WATCHOS)
-    list(APPEND UBSAN_APPLE_PLATFORMS watchos watchossim)
-  endif()
-  if (COMPILER_RT_ENABLE_TVOS)
-    list(APPEND UBSAN_APPLE_PLATFORMS tvos tvossim)
-  endif()
+  set(UBSAN_APPLE_PLATFORMS ${SANITIZER_COMMON_SUPPORTED_OS})
   foreach(platform ${UBSAN_APPLE_PLATFORMS})
-    foreach(arch ${DARWIN_${platform}_ARCHS})
-      set(UBSAN_TEST_TARGET_CFLAGS "-arch ${arch} -isysroot ${DARWIN_${platform}_SYSROOT}")
-      if (";${UBSAN_SUPPORTED_ARCH};" MATCHES ";${arch};")
-        add_ubsan_device_testsuite("Standalone" ubsan ${platform} ${arch})
-      endif()
+    list_intersect(
+      UBSAN_TEST_${platform}_ARCHS
+      UBSAN_SUPPORTED_ARCH
+      DARWIN_${platform}_ARCHS
+      )
+    foreach(arch ${UBSAN_TEST_${platform}_ARCHS})
+      get_test_cflags_for_apple_platform(
+        "${platform}"
+        "${arch}"
+        UBSAN_TEST_TARGET_CFLAGS
+      )
 
       if(COMPILER_RT_HAS_ASAN AND ";${ASAN_SUPPORTED_ARCH};" MATCHES ";${arch};")
         add_ubsan_device_testsuite("AddressSanitizer" asan ${platform} ${arch})
       endif()
 
       if(COMPILER_RT_HAS_TSAN AND ";${TSAN_SUPPORTED_ARCH};" MATCHES ";${arch};")
         add_ubsan_device_testsuite("ThreadSanitizer" tsan ${platform} ${arch})
       endif()