Page MenuHomePhabricator

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

Authored by delcypher on Fri, Jun 21, 2:55 PM.

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

Event Timeline

delcypher created this revision.Fri, Jun 21, 2:55 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFri, Jun 21, 2:55 PM
Herald added subscribers: Restricted Project, kristof.beyls, javed.absar, mgorny. · View Herald Transcript

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

hans added a comment.Mon, Jun 24, 1:52 AM

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

@hans

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

@hans

@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.)

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