This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Add experimental support for building compiler-rt for iOS
ClosedPublic

Authored by beanz on Aug 6 2015, 2:13 PM.

Details

Summary

This is a reunification of three separate reviews D11073, D11082, D11083.

Having them separate was not constructive even though the patches were smaller because it led to fragmented conversations, and this is really all about one change.

This patch incorporates feedback from samsonov, and refactors the hacky darwin code out of the root CMakeLists.txt and int config-ix.cmake.

Diff Detail

Repository
rL LLVM

Event Timeline

beanz updated this revision to Diff 31477.Aug 6 2015, 2:13 PM
beanz retitled this revision from to [CMake] Add experimental support for building compiler-rt for iOS.
beanz updated this object.
beanz added a subscriber: llvm-commits.
beanz updated this revision to Diff 31483.Aug 6 2015, 3:43 PM

Updating formatting based on Feedback from bogner.

beanz added a comment.Aug 10 2015, 9:29 AM

samsonov, ping?

beanz updated this revision to Diff 31678.Aug 10 2015, 9:43 AM

Fixing a bug I found this morning building with COMPILER_RT_ENABLE_IOS=Off (the default behavior). My code for filtering unsupported architectures wasn't quite right.

filcab added a subscriber: filcab.Aug 10 2015, 11:55 AM

Minor nits and comments inline.
Thanks a lot for working on this!

cmake/Modules/CompilerRTDarwinUtils.cmake
31 ↗(On Diff #31678)

Nit: Do we care about having the "LTO support" string?

63 ↗(On Diff #31678)

"will build", but then you remove that arch?
Why is the iossim different from OS X, here?

65 ↗(On Diff #31678)

Nit: You've consistently kept local variable all_lowercase except for this one.

cmake/config-ix.cmake
318 ↗(On Diff #31678)

What's the behavior you want if MACOSX_VERSION_MIN_FLAG is set? I'm guessing you're going with "just assume the user just wants OS X, since they passed that flag in the CXX flags".
Shouldn't we at least test the OS X archs?

Why are we skipping iOS and iOS Simulator when we have a specific minimum OS X version? (I'm good with "so we don't change the user's CXX flags", but want to double-check)

335 ↗(On Diff #31678)

Does clang not pass the proper flag to the linker for the iOS Simulator (when the flag below is passed)?
If not, are they both needed, still? Since clang should only be invoking the linker.
Why didn't we need this for OS X? Why are the versions slightly different?

357 ↗(On Diff #31678)

Same as for iOS Simulator.

beanz added a comment.Aug 10 2015, 1:18 PM

I'll send updated patches shortly. Comments inline below.

cmake/Modules/CompilerRTDarwinUtils.cmake
31 ↗(On Diff #31678)

The "LTO Support" string is needed to escape the end of the regex. Since .* matches everything and nothing, and the behavior is to match as much as possible without the "LTO Support" string it matches the whole rest of the output.

I'm no regex expert though, so if you have a better suggestion I'm open.

63 ↗(On Diff #31678)

I should explain better. The trivial test program does compile for x86_64h because it falls back to using the x86_64 libraries, but x86_64h isn't actually a valid simulator architecture, so other things will fail. I've had trouble finding a non-hacky way to make that detection work.

cmake/config-ix.cmake
318 ↗(On Diff #31678)

The behavior if the user set macos-version-min explicitly is kinda complicated and probably broken. As it is now, I'm pretty sure we just don't build the sanitizers if you specify an explicit version min.

The rationale for skipping iOS and the simulator I assume is because we don't want to change the user's CXX flags, and there may be other incompatibilities. I don't know for sure though because that code exists in-tree today. This patch just moves it from the root CMakeLists.txt to config-ix as per samsonov's comments on previous reviews.

335 ↗(On Diff #31678)

You're right. Since the driver should be passing this through we don't need it explicitly set.

beanz updated this revision to Diff 31714.Aug 10 2015, 1:22 PM

Updates based on feedback from filcab.

beanz updated this revision to Diff 31720.Aug 10 2015, 2:10 PM

Fixing regex as per filcab's suggestion.

samsonov edited edge metadata.Aug 12 2015, 3:49 PM

Sorry for the late response! This actually looks mostly good, I've got several minor comments.

cmake/Modules/AddCompilerRT.cmake
22 ↗(On Diff #31720)

object library is just a collection of .o files. Why do you need link flags here?
(sorry if you've already replied me and I failed to notice it)

cmake/Modules/CompilerRTDarwinUtils.cmake
80 ↗(On Diff #31720)

Please fix

cmake/Modules/CompilerRTUtils.cmake
66 ↗(On Diff #31720)

Consider using

list(APPEND ${output} ${it})
cmake/config-ix.cmake
176 ↗(On Diff #31720)

Consider replacing it with

elseif(NOT APPLE)

with a comment that COMPILER_RT_SUPPORTED_ARCH list for Apple platforms is generated below.

262 ↗(On Diff #31720)

s/aarch64/${ARM64}

280 ↗(On Diff #31720)

Looks like some part was lost in rebase. You need to put the block below under

if(NOT SANITIZER_MIN_OSX_VERSION)

so that the user can manually specify -DSANITIZER_MIN_OSX_VERSION at configure time (I was specifically asked to add this feature by Chromium folks).

(you might still look for MACOSX_VERSION_MIN_FLAG, because you're using it below).

380 ↗(On Diff #31720)

Why not

list_union(ASAN_SUPPORTED_ARCH ALL_ASAN_SUPPORTED_ARCH SANITIZER_COMMON_SUPPORTED_ARCH)

?

Comments inline. Updated patches coming shortly.

cmake/Modules/AddCompilerRT.cmake
22 ↗(On Diff #31720)

You are right. I will fix this.

cmake/Modules/CompilerRTUtils.cmake
66 ↗(On Diff #31720)

You are right. I will fix this.

cmake/config-ix.cmake
176 ↗(On Diff #31720)

Sure. I'll update this.

262 ↗(On Diff #31720)

You are right. I will fix this.

280 ↗(On Diff #31720)

You are right. I will fix this.

380 ↗(On Diff #31720)

No particular reason. I will fix this.

beanz updated this revision to Diff 32088.Aug 13 2015, 1:13 PM
beanz edited edge metadata.

Updates based on feedback from samsonov.

samsonov accepted this revision.Aug 13 2015, 1:36 PM
samsonov edited edge metadata.

OK, let's move this in, so that you can experiment with iOS build. Please watch linux/freebsd bots after commit. Thank you!

This revision is now accepted and ready to land.Aug 13 2015, 1:36 PM
beanz added a comment.Aug 13 2015, 1:38 PM

Thank you! I will land the patches shortly and watch the bots.

This revision was automatically updated to reflect the committed changes.