Page MenuHomePhabricator

Adding experimental build support for building compiler-rt for iOS.
AbandonedPublic

Authored by beanz on Jun 24 2015, 2:05 PM.

Details

Summary

This patch updates and modifies some of the CMake support for building on Darwin platforms with the goal of adding support for iOS and making the code more managable. The following changes are present:

  • Updated add_compiler_rt_object_libraries to properly handle setting architectures
  • Added cmake/Modules/CompilerRTDarwinUtils.cmake to contain darwin-specific functions for detecting toolchain features
  • Added iOS-specific flags
  • Re-structured the code for setting platform-specific flags and enabling platforms
  • Updated call sites of add_compiler_rt_darwin_dynamic_runtime to use SANITIZER_COMMON_DARWIN_${os}_ARCHES
  • Added COMPILER_RT_ENABLE_IOS option (default value is Off) to control building for iOS

Diff Detail

Event Timeline

beanz updated this revision to Diff 28402.Jun 24 2015, 2:05 PM
beanz retitled this revision from to Adding experimental build support for building compiler-rt for iOS..
beanz updated this object.
beanz edited the test plan for this revision. (Show Details)
beanz added a subscriber: Unknown Object (MLST).
filcab added a subscriber: filcab.Jun 24 2015, 2:50 PM

Thanks for adding this!

CMakeLists.txt
349

Nit: If we're printing a status message, I think we could spare the bytes to print "Simulator" :-)

cmake/Modules/CompilerRTDarwinUtils.cmake
3

This seems misleading. xcodebuild doesn't seem to honour SDKROOT.

If I have 10.10 and 10.9 installed, and set SDKROOT=/.../Xcode.app/.../MacOSX10.9.sdk, I get:

[~]% xcodebuild -version -sdk macosx Path
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk

(This is really handy when you have an SDK available for 10.(X+1), but are running on 10.X.

lib/asan/CMakeLists.txt
116

Just to be sure, ASan will be supported on all of the arch + OS that sanitizer_common will run on? (at first sight, it almost looks like a copy-paste bug)

You're adding object_libraries (line 80) for ASAN_SUPPORTED_ARCH, but then generate dynamic_runtimes for SANITIZER_COMMON_..._ARCHES. Shouldn't these be the same, at least?

lib/ubsan/CMakeLists.txt
47

Just to be sure, UBSan will be supported on all of the arch + OS that sanitizer_common will run on?

Same comment as ASan (check line 42 here).

beanz added a comment.Jun 24 2015, 2:58 PM

I will update the patches shortly.

cmake/Modules/CompilerRTDarwinUtils.cmake
3

I'll reword. Xcodebuild doesn't respect SDKROOT if you specify -sdk, which is why you would see that behavior.

lib/asan/CMakeLists.txt
116

Anna would have to comment as to what ASan will or won't be supported on. I'm just trying to make it build, and ASAN_SUPPORTED_ARCH is set to i386;x86_64, which just doesn't work on iOS. For Darwin we always need platform-specific arch settings.

The line 80 thing is just an oddity of the moment. If you look at add_compiler_rt_object_libraries, it actually ignores the ARCH setting that comes in because it needs platform-specific archs.

lib/ubsan/CMakeLists.txt
47

Again, I can't really comment about official support. I'm just trying to make it build and you can't build UBSan for iOS targeting x86.

My goal in these patches was to identify up front the architectures that your host toolchain is capable of targeting for each target OS, and build everything for all of them.

beanz updated this revision to Diff 28412.Jun 24 2015, 3:01 PM

Updating based on feedback from filcab.

samsonov edited edge metadata.Jun 24 2015, 3:33 PM

I support the direction of this patch, but think it needs to be somewhat polished.
Most importantly, I'd suggest to split it into several smaller patches, that would be either to digest, review, and eventually land.
For example, substantial part of this changes is refactoring (introducing CompilerRTDarwinUtils, moving some functions there,
factoring out common flags to DARWIN_COMMON_CFLAGS, etc.), that can be committed first.

More comments inline.

CMakeLists.txt
300

This looks weird. It's just a coincidence that -stdlib=libc++ is both linker and compile flag. Not all compiler flags might be passed here.

307

Keep DARWIN_COMMON_LINKFLAGS first for consistency.

320

Do you actually need to do all this setup if IOSSIM_SDK_DIR is not specified? It would make sense to group flag setup and darwin_test_archs invocation together.

335

Print them after you calculated them?

361

This is confusing, we don't set anything below.

cmake/Modules/AddCompilerRT.cmake
23

This is really confusing now - you never use ARCHS input list on Darwin at all. The user should have the ability to restrict the set of architectures she targets - this is exactly the reason why we have ASAN_SUPPORTED_ARCH, UBSAN_SUPPORTED_ARCH and so on. There is no guarantee that we would be able to build ASan runtime for all architectures that our host toolchain can target for OS.

At the very least it means that you should calculate the intersection of ${LIB_ARCHS} and ${SANITIZER_COMMON_DARWIN_${os}_ARCHES}. But now all this gets complicated - different architectures are available on different OSes, so it turns out that the input parameters of modified add_compiler_rt_object_libraries don't work well. I see no "easy" fix for that - probably we should start to make complex intrusive changes and move from keeping the list of "supported architectures" (COMPILER_RT_SUPPORTED_ARCH) and "supported OSes" (SANITIZER_COMMON_SUPPORTED_OS) to a list of "supported triples"
(well, in fact, OS-arch pairs).

cmake/Modules/CompilerRTDarwinUtils.cmake
27

What if ld -v failed?

31

I think it's easier to scrape this information with regexp matching. For example, see the calculation of SANITIZER_MIN_OSX_VERSION from -mmacosx-version-min= in CMakeLists.txt. Then you will also be able to detect an error when you're unable to find configured to support archs: substring.

33

Why do you refer to the number of convex uniform honeycombs here?

65

Some of this code duplicates logic from cmake/config-ix.cmake that calculates COMPILER_RT_SUPPORTED_ARCH. I think you'd need to use the latter variable on Darwin (although that's tricky - see rant about triples above).

lib/asan/CMakeLists.txt
116

See note above.

beanz abandoned this revision.Jun 25 2015, 9:42 AM

Alexey,

Thanks for the feedback. I'll split this up and revise as you suggested. I'm going out of town this evening until 7/7, so I probably won't send updated patches until I get back in July.

Thanks,
-Chris