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

[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.

rdar://problem/50124489

[yln]

compiler-rt/cmake/config-ix.cmake

223

Nitpick: is_valid_apple_platform can return true/false, or error.
From the name I would not expect that it can error.
I would prefer:
is_valid_apple_platform -> true/false
[assert/check/ensure/...]_is_valid_apple_platform -> no return value, just assert.

[delcypher]

compiler-rt/cmake/config-ix.cmake

223

The error is a misuse of the API. Passing an empty string is almost certain a bug and we should stop immediately.

[delcypher]

@kubamracek @yln ping.

[delcypher]

@kubamracek @yln ping.

[yln]

Overall changes: LGTM, thanks!

As already noted, I would probably change the is_valid_apple_platform to:
is_apple_platform -> just yes/no, even for the empty string.
or rename to assert_valid_apple_platform() -> no return value, aborts if not.

An is_xxx "getter" having 3 kinds of "results": true, false, or abort; is unexpected to me.
I don't feel too strongly strongly about this though, so land this at your discretion.

[delcypher]

No real update, just for fixing up commit message on my local machine.

[delcypher]

In D58578#1475863, @yln wrote:

Overall changes: LGTM, thanks!

As already noted, I would probably change the is_valid_apple_platform to:
is_apple_platform -> just yes/no, even for the empty string.
or rename to assert_valid_apple_platform() -> no return value, aborts if not.

An is_xxx "getter" having 3 kinds of "results": true, false, or abort; is unexpected to me.

One way of looking at this is the empty platform name check being a precondition for the is_valid_apple_platform function.
It seems reasonable (to me at least) that if you violate the pre-condition that we should abort. In a language like C/C++ I probably
wouldn't have a precondition like this but in a language like CMake where it's incredibly easy to accidently pass the empty string
it seems worth having the check.

I don't feel too strongly strongly about this though, so land this at your discretion.

Okay. I've landed it as is. We can revisit this later if it really becomes a problem.

[juliehockett]

This patch has broken our bots -- when it's building the osx host runtime, it's now triggering an assert on the tsan arm64 test case that wasn't being triggered before. Would you be able to take a look?

Error:

CMake Error at /b/s/w/ir/k/llvm-project/compiler-rt/cmake/config-ix.cmake:177 (message):
  Unsupported architecture: arm64
Call Stack (most recent call first):
  /b/s/w/ir/k/llvm-project/compiler-rt/cmake/config-ix.cmake:216 (get_target_flags_for_arch)
  /b/s/w/ir/k/llvm-project/compiler-rt/test/tsan/CMakeLists.txt:78 (get_test_cflags_for_apple_platform)

Full log: https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8915104112618242800/+/steps/clang/0/steps/build/0/stdout

[delcypher]

In D58578#1480691, @juliehockett wrote:

This patch has broken our bots -- when it's building the osx host runtime, it's now triggering an assert on the tsan arm64 test case that wasn't being triggered before. Would you be able to take a look?

Error:

CMake Error at /b/s/w/ir/k/llvm-project/compiler-rt/cmake/config-ix.cmake:177 (message):
  Unsupported architecture: arm64
Call Stack (most recent call first):
  /b/s/w/ir/k/llvm-project/compiler-rt/cmake/config-ix.cmake:216 (get_target_flags_for_arch)
  /b/s/w/ir/k/llvm-project/compiler-rt/test/tsan/CMakeLists.txt:78 (get_test_cflags_for_apple_platform)

Full log: https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8915104112618242800/+/steps/clang/0/steps/build/0/stdout

Sorry about this. I'll revert the patch now and I'll take a look at re-landing it when time permits.

Could you clarify what you mean by "assert on the tsan arm64 test case", all I see in the log is a CMake error, relating to arm64. I don't see any lit tests being executed.

[juliehockett]

In D58578#1480711, @delcypher wrote:

In D58578#1480691, @juliehockett wrote:

This patch has broken our bots -- when it's building the osx host runtime, it's now triggering an assert on the tsan arm64 test case that wasn't being triggered before. Would you be able to take a look?

Error:

CMake Error at /b/s/w/ir/k/llvm-project/compiler-rt/cmake/config-ix.cmake:177 (message):
  Unsupported architecture: arm64
Call Stack (most recent call first):
  /b/s/w/ir/k/llvm-project/compiler-rt/cmake/config-ix.cmake:216 (get_target_flags_for_arch)
  /b/s/w/ir/k/llvm-project/compiler-rt/test/tsan/CMakeLists.txt:78 (get_test_cflags_for_apple_platform)

Full log: https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8915104112618242800/+/steps/clang/0/steps/build/0/stdout

Sorry about this. I'll revert the patch now and I'll take a look at re-landing it when time permits.

Could you clarify what you mean by "assert on the tsan arm64 test case", all I see in the log is a CMake error, relating to arm64. I don't see any lit tests being executed.

Sure -- to clarify, it's not a test failure, it's a build one. The error was triggered during the CMake configuration step for host runtimes, where we have OSX supported arches: x86_64 only. test/tsan/CMakeLists.txt:78 now calls into the new function on the arm64 case, which down the line triggers a CMake assert that arm64 is in the list of supported arches when it isn't.

Seems like prior to this change, the lit tests were being configured without doing any checks on that list of supported arches. I'm not as familiar with how this code is set up, so I can't really say if it's a problem with this patch or if this patch is exposing another bug. Thanks for taking a look!

[delcypher]

@juliehockett

Sorry about this. I'll revert the patch now and I'll take a look at re-landing it when time permits.

Reverted in r359327. Do you have a script (or something better?) that would let me easily replicate your set up. I suspect your use of LLD + Clang is probably what is different from my set up and leads to the CMake failure. You're building compiler-rt with open source Clang rather than AppleClang (i.e. from Xcode or Xcode command line tools) right?

[phosek]

In D58578#1480757, @delcypher wrote:

@juliehockett

Sorry about this. I'll revert the patch now and I'll take a look at re-landing it when time permits.

Reverted in r359327. Do you have a script (or something better?) that would let me easily replicate your set up. I suspect your use of LLD + Clang is probably what is different from my set up and leads to the CMake failure. You're building compiler-rt with open source Clang rather than AppleClang (i.e. from Xcode or Xcode command line tools) right?

To clarify, this is failing when building runtimes for Darwin so shouldn't be Fuchsia specific, but it could be affected by some of the Clang defaults we set in our CMake build. We use lld as the default linker for Fuchsia but not for Darwin where use the system linker.

You can see the CMake invocation at top of this page: https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8915104112618242800/+/steps/clang/0/steps/configure/0/stdout. It's also worth noting that this was only failing in case of the 2-stage build, the single stage build remained green. I can try to reproduce this locally on my Mac as well if that's helpful.

[delcypher]

To clarify, this is failing when building runtimes for Darwin so shouldn't be Fuchsia specific, but it could be affected by some of the Clang defaults we set in our CMake build. We use lld as the default linker for Fuchsia but not for Darwin where use the system linker.

Based on the log the https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket.appspot.com/8915104112618242800/+/steps/clang/0/steps/build/0/stdout log that @juliehockett pointed to the CMake output suggests that LLD is being used when building the compiler-rt runtime

[3397/3401] Performing configure step for 'runtimes'
-- The C compiler identification is Clang 9.0.0
-- The CXX compiler identification is Clang 9.0.0
-- The ASM compiler identification is Clang
-- Found assembler: /b/s/w/ir/k/recipe_cleanup/clangp5sgXt/llvm_build_dir/./bin/clang
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
LLD 9.0.0 (https://fuchsia.googlesource.com/a/third_party/llvm-project 41327e352285df0f437affc5d6b1522ad650f750) (compatible with GNU linkers)
-- Linker detection: unknown
...
LLD 9.0.0 (https://fuchsia.googlesource.com/a/third_party/llvm-project 41327e352285df0f437affc5d6b1522ad650f750) (compatible with GNU linkers)
CMake Warning at /b/s/w/ir/k/llvm-project/compiler-rt/cmake/Modules/CompilerRTDarwinUtils.cmake:57 (message):
  Detecting supported architectures from 'ld -v' failed.  Returning default
  set.
Call Stack (most recent call first):
  /b/s/w/ir/k/llvm-project/compiler-rt/cmake/config-ix.cmake:374 (darwin_get_toolchain_supported_archs)
  /b/s/w/ir/k/llvm-project/compiler-rt/CMakeLists.txt:280 (include)

This isn't actually the source of the problem though so I'll ignore this for now.

[delcypher]

Sure -- to clarify, it's not a test failure, it's a build one. The error was triggered during the CMake configuration step for host runtimes, where we have OSX supported arches: x86_64 only. test/tsan/CMakeLists.txt:78 now calls into the new function on the arm64 case, which down the line triggers a CMake assert that arm64 is in the list of supported arches when it isn't.

Seems like prior to this change, the lit tests were being configured without doing any checks on that list of supported arches. I'm not as familiar with how this code is set up, so I can't really say if it's a problem with this patch or if this patch is exposing another bug. Thanks for taking a look!

Yep it's this patch exposing another bug. Your configuration only seems to allow x86_64 as an architecture (any idea why?) and the CMake code in test/tsan/CMakeLists.txt just assumes that arm64 is supported. I'll just hack my build so it's as if x86_64 was the only supported architecture to replicate this failure, then I'll fix the bug and submit a new patch for review.

compiler-rt/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)

compiler-rt/test/asan/CMakeLists.txt

if(APPLE)
   set(EXCLUDE_FROM_ALL ON)
 
   set(ASAN_TEST_TARGET_CC ${COMPILER_RT_TEST_COMPILER})
   set(ASAN_TEST_DYNAMIC True)
 
   foreach(arch ${DARWIN_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.in
       ${CMAKE_CURRENT_BINARY_DIR}/${CONFIG_NAME}/lit.site.cfg
       )
     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})
     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.in
       ${CMAKE_CURRENT_BINARY_DIR}/${CONFIG_NAME}/lit.site.cfg
       )

compiler-rt/test/fuzzer/CMakeLists.txt

 endif()
 
 if (APPLE)
   set(EXCLUDE_FROM_ALL ON)
 
   foreach(arch ${DARWIN_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.in
       ${CMAKE_CURRENT_BINARY_DIR}/${CONFIG_NAME}/lit.site.cfg
       )
     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()

compiler-rt/test/tsan/CMakeLists.txt

 if(APPLE)
   set(EXCLUDE_FROM_ALL ON)
 
   set(TSAN_TEST_TARGET_CC ${COMPILER_RT_TEST_COMPILER})
 
   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.in
     ${CMAKE_CURRENT_BINARY_DIR}/${CONFIG_NAME}/lit.site.cfg
     )
   add_lit_testsuite(check-tsan-iossim-${arch} "ThreadSanitizer iOS Simulator ${arch} tests"
     ${CMAKE_CURRENT_BINARY_DIR}/${CONFIG_NAME}/
     DEPENDS ${TSAN_TEST_DEPS})
 
   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.in
     ${CMAKE_CURRENT_BINARY_DIR}/${CONFIG_NAME}/lit.site.cfg
     )
   add_lit_testsuite(check-tsan-ios-${arch} "ThreadSanitizer iOS Simulator ${arch} tests"

compiler-rt/test/ubsan/CMakeLists.txt

if(APPLE)
   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()
   foreach(platform ${UBSAN_APPLE_PLATFORMS})
     foreach(arch ${DARWIN_${platform}_ARCHS})
-      get_target_flags_for_arch(${arch} UBSAN_TEST_TARGET_ARCH_FLAGS_AS_LIST)
-      string(REPLACE ";" " " UBSAN_TEST_TARGET_ARCH_FLAGS "${UBSAN_TEST_TARGET_ARCH_FLAGS_AS_LIST}")
-      set(UBSAN_TEST_TARGET_CFLAGS
-        "${UBSAN_TEST_TARGET_ARCH_FLAGS} -isysroot ${DARWIN_${platform}_SYSROOT}")
+      get_test_cflags_for_apple_platform(
+        "${platform}"
+        "${arch}"
+        UBSAN_TEST_TARGET_CFLAGS
+      )
       if (";${UBSAN_SUPPORTED_ARCH};" MATCHES ";${arch};")
         add_ubsan_device_testsuite("Standalone" ubsan ${platform} ${arch})
       endif()
 
       if(COMPILER_RT_HAS_ASAN AND ";${ASAN_SUPPORTED_ARCH};" MATCHES ";${arch};")
         add_ubsan_device_testsuite("AddressSanitizer" asan ${platform} ${arch})
       endif()