Page MenuHomePhabricator

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

Authored by delcypher on Jun 21 2019, 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.Jun 21 2019, 2:55 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 21 2019, 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.Jun 24 2019, 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.

hans added a comment.Jul 22 2019, 2:38 PM

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

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

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 updated this revision to Diff 222722.Tue, Oct 1, 3:57 PM

Rebase on master.

This already landed in r373405

delcypher abandoned this revision.Thu, Oct 17, 11:00 AM