This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Adding some utility functions for Darwin builds into a new CompilerRTDarwinUtils.cmake module
AbandonedPublic

Authored by beanz on Jul 9 2015, 1:21 PM.

Details

Summary

The new darwin utils module contains the following functions:

  • find_darwin_sdk_dir - finds SDK paths based on toolchain names (i.e. iphoneos), this is currently implemented as a macro in CMakeLists.txt.
  • darwin_get_toolchain_supported_archs - Queries ld for the architectures it supports linking for
  • darwin_test_archs - determines which architectures from a set are supported by the toolchain being used.

Diff Detail

Event Timeline

beanz updated this revision to Diff 29381.Jul 9 2015, 1:21 PM
beanz retitled this revision from to [CMake] Adding some utility functions for Darwin builds into a new CompilerRTDarwinUtils.cmake module.
beanz updated this object.
beanz added a subscriber: llvm-commits.
samsonov added inline comments.Jul 15 2015, 3:33 PM
cmake/Modules/CompilerRTDarwinUtils.cmake
4

Please actually use this function in CMakeLists.txt (and delete macro used there).

26

Looks like this should be the Darwin version of the complex code that generates COMPILER_RT_SUPPORTED_ARCH. Can you just call it from there?

79

please fix

I'll send updated patches shortly that fix some of the small issues. I have some big comments inline that regarding your suggestion about COMPILER_RT_SUPPORTED_ARCH. I think this may need more discussion before landing these patches.

Comments inline below.

Thanks,
Chris

cmake/Modules/CompilerRTDarwinUtils.cmake
4

Sure. I can move that into this patch. I was doing that in D11083.

26

Hooking this into where we setup COMPILER_RT_SUPPORTED_ARCH would be pretty complicated. Much of that code in config-ix.cmake is based around the idea of building compiler-rt for a single target. Also darwin_get_toolchain_supported_archs returns a long list of architectures we don't care about. On the latest Xcode the list is: armv6 armv7 armv7s arm64 i386 x86_64 x86_64h armv6m armv7m armv7em.

For Darwin we currently build for n architectures and m platforms generating an n x m configuration matrix. To take the architectures returned by ld -v and group them into platform-architecture pairs we'd also need all the compiler flags properly setup for each platform. Currently that is all done in CMakeLists.txt:272-315 well after config-ix is processed.

In order to do this work in config-ix, we would need to shift all of that around, which would make these patches quite substantial. I'd like to avoid doing that because I really want to get rid of all of that, but I don't yet have a sane transition strategy yet.

Thoughts?

79

Will do in my next update.

samsonov added inline comments.Jul 21 2015, 6:03 PM
cmake/Modules/CompilerRTDarwinUtils.cmake
26

Yes, I agree this would be complicated, but we'll have to do it anyway =/ I don't think that landing this patch before the rework would make things easier for us, the contrary is more likely.

The code in config-ix.cmake is actually based on the idea that we target a single platform/OS, and (possibly) several architectures. This is the "usual" case for 64-bit Linux, where we build runtimes for both x86_64 and i386. But this is n x 1 as opposed to n x m case on Mac. I think we should rework the code assuming that we build runtimes for "triples" instead (well, in our case that would be OS/arch pairs).

Now determining compile flags for target is already split between config-ix.cmake (TARGET_${arch}_FLAGS) and CMakeLists.txt (DARWIN_${os}_CFLAGS), it would be better to consolidate this part before moving forward. I have no transition strategy yet either, but can try to up-prioritize working on that.

If you want to proceed with this patch as quickly as possible anyway, I would still suggest to move the flag setup logic to config-ix, under

if("${LLVM_NATIVE_ARCH}" STREQUAL "X86")
  if(APPLE)

, as we don't care about building ASan on other Mac OS hosts, do we?

beanz added inline comments.Jul 22 2015, 9:04 AM
cmake/Modules/CompilerRTDarwinUtils.cmake
26

I actually don't think that the code I'm proposing in this patch should exist in the "fixed" compiler-rt build scripts. I see it as a temporary solution to get the CMake build system to be feature-for-feature compatible in open-source with the things we use autoconf for at Apple.

With Xcode 7 we have shipped asan support for iOS, so we need CMake to support it.

I actually think the Linux cmake code as it is today is MUCH closer to what we actually want than the Darwin code, so I'd really prefer not to hack bad behaviors into the existing linux code.

Once I've gotten the feature parity between CMake and Autoconf, the next step I want to take is to start putting in a new behavior for how CMake builds on Darwin (controlled by a variable), which will allow it to build for a single platform at a time. I expect that change will basically be following the Linux code paths with very few exceptions.

I know this really isn't a good justification because I'm trying to convince you to be ok with something that I think is a bad idea, but this is a temporary solution.

Does that make sense?

samsonov added inline comments.Jul 22 2015, 9:03 PM
cmake/Modules/CompilerRTDarwinUtils.cmake
26

Oh, I would love to have better support for CMake on Darwin, so that it would (finally) replace autoconf build, which keeps causing problems for us. Do you think you will be able to drop autoconf build soon after adding support for ios/iossim?

Thanks for clarification regarding your future plans. I'm actually in favor of this idea (building for one platform at a time, and having separate build trees for different OS). We do smth. similar for Android already.

I'm not convinced that it would be easier to land current patches, and then rework them, instead of building OS-specific trees upfront. I would agree on that only if you will delete the autoconf build system right after landing this patch set, so that we at least keep the level of horridness in the build system code :)

beanz added inline comments.Jul 22 2015, 9:50 PM
cmake/Modules/CompilerRTDarwinUtils.cmake
26

There is a lot more work than just these patches to be able to replace the autoconf build system. Before we can get rid of autoconf, we need to get to the final state where we build compiler-rt for one platform at a time. That is my goal.

The reason it will be easier for me to land these patches and then rework them is because we have internal out-of-tree patches that add functionality to the CMake build system. As long as those patches exist I have to merge any changes I make in open source into our internal branches without losing functionality.

What I'm trying to do in these patches is to land the features we have internally, so that I can clobber the internal diffs with the open-source tree. Once we have no out-of-tree patches I can work more freely on the substantial refactoring and cleanup.

I'm going to try and send out an RFC this week as a follow up to my RFC from June with more concrete details for how I think we can rework the compiler-rt build system.

samsonov added inline comments.Jul 28 2015, 3:47 PM
cmake/Modules/CompilerRTDarwinUtils.cmake
26

Sorry for the delayed response.

Alright, I see. If it would be more convenient for you to add support for targeting osx/ios/iossim in a single build tree on Mac OS X now, let's try this. But in this case let's make it clear that we essentially have a parallel build system for Apple. Probably we should clearly show that new code is an Apple alternative for code that determines supported architectures from cmake/config-ix.cmake.

How about this: move the new code to the config-ix.cmake, next to the place where we generate COMPILER_RT_SUPPORTED_ARCH. Then you can wrap all uses of this variable, so that it would either return COMPILER_RT_SUPPORTED_ARCH for non-Apple, or return OS-specific list of architectures for Apple.

beanz abandoned this revision.Aug 6 2015, 2:15 PM

I'm consolidating this review under D11820 because the fragmented conversation on multiple review threads was not constructive.

-Chris