Page MenuHomePhabricator

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

Authored by delcypher on Apr 28 2019, 9:29 AM.

Details

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

rdar://problem/50124489

Diff Detail

Repository
rL LLVM

Event Timeline

delcypher created this revision.Apr 28 2019, 9:29 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 28 2019, 9:29 AM
Herald added subscribers: Restricted Project, kristof.beyls, javed.absar, mgorny. · View Herald Transcript

@juliehockett @phosek This is a new version of https://reviews.llvm.org/D58578 . Would you be able to test it in your set up? I'd prefer to have the confidence of knowing it doesn't break your set up before landing this.

juliehockett accepted this revision.Apr 29 2019, 12:31 PM

It's building on my machine now with our configuration -- and made me properly set up my mac again for development, so many positives here. Thanks for the fix!

This revision is now accepted and ready to land.Apr 29 2019, 12:31 PM
This revision was automatically updated to reflect the committed changes.

@juliehockett Sorry I completely dropped the ball here and forgot to land this. I've rebased and committed the change. @thakis I noticed I had a merge conflict with your change. Hopefully this hasn't broken anything for you.

hans added a subscriber: hans.Jun 19 2019, 2:06 AM

@juliehockett Sorry I completely dropped the ball here and forgot to land this. I've rebased and committed the change. @thakis I noticed I had a merge conflict with your change. Hopefully this hasn't broken anything for you.

I don't have context here, but this change caused Chromium's clang package to stop building, failing with:

[...]
CMake Error at /work/llvm.monorepo/compiler-rt/cmake/config-ix.cmake:179 (message):

Unsupported architecture: i386

Call Stack (most recent call first):

/work/llvm.monorepo/compiler-rt/cmake/config-ix.cmake:218 (get_target_flags_for_arch)
/work/llvm.monorepo/compiler-rt/test/ubsan/CMakeLists.txt:116 (get_test_cflags_for_apple_platform)

See https://bugs.chromium.org/p/chromium/issues/detail?id=976593#c3 for the full cmake invocation and output.

I've reverted this in r363779 until it's more clear what's happening.

@juliehockett Sorry I completely dropped the ball here and forgot to land this. I've rebased and committed the change. @thakis I noticed I had a merge conflict with your change. Hopefully this hasn't broken anything for you.

I don't have context here, but this change caused Chromium's clang package to stop building, failing with:

[...]
CMake Error at /work/llvm.monorepo/compiler-rt/cmake/config-ix.cmake:179 (message):

Unsupported architecture: i386

Call Stack (most recent call first):

/work/llvm.monorepo/compiler-rt/cmake/config-ix.cmake:218 (get_target_flags_for_arch)
/work/llvm.monorepo/compiler-rt/test/ubsan/CMakeLists.txt:116 (get_test_cflags_for_apple_platform)

See https://bugs.chromium.org/p/chromium/issues/detail?id=976593#c3 for the full cmake invocation and output.

I've reverted this in r363779 until it's more clear what's happening.

@hans
Thanks for the link to reproducer. Unfortunately I've not been able to reproduce this issue. I reverted your revert of my patch and then tried running the cmake configure and it succeeds for me. I used AppleClang (LLVM version 9.1.0 (clang-902.0.39.1)) from Xcode 9 as the toolchain.

cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_ENABLE_PROJECTS="clang;compiler-rt;lld;libcxx" -DLLVM_TARGETS_TO_BUILD="AArch64;ARM;Mips;PowerPC;SystemZ;WebAssembly;X86" -DLLVM_ENABLE_PIC=OFF -DLLVM_ENABLE_UNWIND_TABLES=OFF -DLLVM_ENABLE_TERMINFO=OFF -DCLANG_PLUGIN_SUPPORT=OFF -DCLANG_ENABLE_STATIC_ANALYZER=OFF -DCLANG_ENABLE_ARCMT=OFF -DCOMPILER_RT_USE_LIBCXX=NO -DLIBCXX_CXX_ABI=libcxxabi -DLIBCXX_CXX_ABI_SYSTEM=1 -DLLVM_ENABLE_LIBXML2=FORCE_ON -DCOMPILER_RT_BUILD_CRT=OFF -DCOMPILER_RT_BUILD_LIBFUZZER=ON -DCOMPILER_RT_BUILD_PROFILE=ON -DCOMPILER_RT_BUILD_SANITIZERS=ON -DCOMPILER_RT_BUILD_XRAY=OFF -DCOMPILER_RT_BUILD_BUILTINS=ON -DCOMPILER_RT_ENABLE_IOS=ON -DCOMPILER_RT_ENABLE_WATCHOS=OFF -DCOMPILER_RT_ENABLE_TVOS=OFF -DDARWIN_ios_ARCHS="armv7;armv7s;arm64" -DDARWIN_iossim_ARCHS="i386;x86_64" -DDARWIN_osx_ARCHS=x86_64 -DLLVM_ENABLE_THREADS=OFF -DCOMPILER_RT_ENABLE_IOS=ON -DSANITIZER_MIN_OSX_VERSION=10.7 ../../llvm/llvm/

Looking at the output I can see when I run this there are some suspicious differences between my output and yours.

In my output there's

-- ios Simulator supported arches: i386;x86_64

Your output doesn't have that line at all.

My output also shows...

-- Compiler-RT supported architectures: x86_64;i386;armv7;armv7s;arm64

whereas yours shows

-- Compiler-RT supported architectures: x86_64;armv7;armv7s;arm64

I had a quick look around and the missing "-- ios Simulator supported arches: " line could be caused if DARWIN_iossim_SYSROOT was not set. In turn this would mean that COMPILER_RT_SUPPORTED_ARCH wouldn't get i386 added to it. This would explain some of the the symptoms we're seeing.

This is weird though because it suggests that the find_darwin_sdk_dir(DARWIN_iossim_SYSROOT iphonesimulator) call isn't finding the iOS simulator SDK in your toolchain. Is your toolchain a modified Apple toolchain? For the normal (included with Xcode) Apple toolchain our CMake code doesn't have a problem finding the simulator SDK.

hans added a comment.Jun 21 2019, 5:06 AM

This is weird though because it suggests that the find_darwin_sdk_dir(DARWIN_iossim_SYSROOT iphonesimulator) call isn't finding the iOS simulator SDK in your toolchain. Is your toolchain a modified Apple toolchain? For the normal (included with Xcode) Apple toolchain our CMake code doesn't have a problem finding the simulator SDK.

Hmm, yes that seems to be it. Our buildbots use an Xcode that's copied into the build environment, and it seems it doesn't include the iphonesimulator sdk?

$ export DEVELOPER_DIR=/work/chromium/src/build/mac_files/Xcode.app
$ xcodebuild -version -sdk iphonesimulator Path
xcodebuild: error: SDK "iphonesimulator" cannot be located.
hwennborg-macpro:~ hwennborg$ xcodebuild -version -sdk                     
iPhoneOS11.3.sdk - iOS 11.3 (iphoneos11.3)
SDKVersion: 11.3
Path: /work/chromium/src/build/mac_files/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS11.3.sdk
PlatformVersion: 11.3
PlatformPath: /work/chromium/src/build/mac_files/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform
BuildID: C689C3CA-2814-11E8-B440-EB6E943B87E0
ProductBuildVersion: 15E217
ProductCopyright: 1983-2018 Apple Inc.
ProductName: iPhone OS
ProductVersion: 11.3

MacOSX10.13.sdk - macOS 10.13 (macosx10.13)
SDKVersion: 10.13
Path: /work/chromium/src/build/mac_files/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.13.sdk
PlatformVersion: 1.1
PlatformPath: /work/chromium/src/build/mac_files/Xcode.app/Contents/Developer/Platforms/MacOSX.platform
ProductBuildVersion: 17E189
ProductCopyright: 1983-2018 Apple Inc.
ProductName: Mac OS X
ProductUserVisibleVersion: 10.13.4
ProductVersion: 10.13.4

Xcode 9.3.1
Build version 9E501

But somehow it was still able to produce an i386 version of libclang_rt.asan_iossim_dynamic.dylib before your change.

Maybe Nico (cc'd) who knows more about our Mac build environment has any ideas?

But somehow it was still able to produce an i386 version of libclang_rt.asan_iossim_dynamic.dylib before your change.

That doesn't sound right. It should not be possible to build the iOS simulator sanitizer libraries without the iOS simulator SDKs. If that works today, it's by accident.

@hans @thakis Commenting out find_darwin_sdk_dir(DARWIN_iossim_SYSROOT iphonesimulator) was enough for me to reproduce the issue. There's a bug in some existing CMake code I wrote. The code for UBSan test code assumes that if COMPILER_RT_ENABLE_IOS is true that the toolchain can build for iOS and the iOS simulator. This isn't true if one or more of the SDKs are missing. I've written a fix for this and I'll submit a patch for review soon.

@hans @thakis A new version of the patch is available for review at https://reviews.llvm.org/D63674