Page MenuHomePhabricator

[CMake] [Darwin] Bug 21562 - Add a CMake equivalent for make/platform/clang_darwin.mk in compiler_rt
ClosedPublic

Authored by beanz on Sep 22 2015, 10:04 AM.

Details

Summary

Building the builtins on Darwin platforms is a bit complicated. This is a first-pass implementation of the functionality from clang_darwin.mk into CMake.

When building the builtins on Darwin we have layers of blacklists that we apply based on platform, architecture, and minimum supported OS version.

Diff Detail

Repository
rL LLVM

Event Timeline

beanz updated this revision to Diff 35381.Sep 22 2015, 10:04 AM
beanz retitled this revision from to [CMake] [Darwin] Bug 21562 - Add a CMake equivalent for make/platform/clang_darwin.mk in compiler_rt.
beanz updated this object.
beanz added reviewers: bogner, filcab, samsonov, bob.wilson.
beanz added a subscriber: llvm-commits.
beanz updated this revision to Diff 35406.Sep 22 2015, 1:26 PM

Minor fix to how paths are constructed for the lipo command. Should be using the target property TARGET_FILE, instead of hand mangling the path.

samsonov added inline comments.Sep 22 2015, 1:39 PM
cmake/Modules/CompilerRTDarwinUtils.cmake
137 ↗(On Diff #35381)

Any reason you can't delegate it to add_compiler_rt_runtime (with adding isysroot and builtin min-version flags to ${LIB_CFLAGS})?

144 ↗(On Diff #35381)

LIB_DEFS is not defined, there's no DEFS argument. Just delete this property.

155 ↗(On Diff #35381)

Shouldn't you take the list of OS passed in ${ARGN}. I'm fine with either way, but please be consistent.

161 ↗(On Diff #35381)

Variable instead of magic constant 6?

167 ↗(On Diff #35381)

So, the variable name here is confusing as well - it's actually a list of excluded builtins.

lib/builtins/CMakeLists.txt
20 ↗(On Diff #35381)

It's interesting these files were excluded from the build. Can you add them in a separate commit to check if all compiler-rt users are ok with that?

318 ↗(On Diff #35381)

This argument is not used.

lib/builtins/Darwin/README.TXT
9 ↗(On Diff #35381)

I would clarify that .txt files contain files that should be *excluded* in the filename - ios6-armv7.excluded.txt or smth. like that.

beanz added a comment.Sep 22 2015, 1:46 PM

I will revise and send updated patches shortly.

One more comment inline below.

-Chris

cmake/Modules/CompilerRTDarwinUtils.cmake
137 ↗(On Diff #35406)

The required customization to make add_compiler_rt_runtime work seemed a bit much. For example we don't actually want to install the library generated here because it should be lipo'd together per-os.

samsonov added inline comments.Sep 22 2015, 1:49 PM
cmake/Modules/CompilerRTDarwinUtils.cmake
137 ↗(On Diff #35406)

OK, makes sense.

beanz updated this revision to Diff 35420.Sep 22 2015, 2:32 PM

Updates based on feedback from samsonov:

  • Cleaned up unused variables
  • Documented new functions
  • Actually read the argument passed into the darwin_add_builtin_libraries call
  • Made the os version not a magic constant
  • Made variable and function naming more apparent that the lists are excluded
  • Moved filterd files to directory named Darwin-excludes so that the file path makes the purpose apparent
  • Broke out adding atomic builtins into D13069 (landed r248322)
beanz updated this revision to Diff 35421.Sep 22 2015, 3:01 PM

Missed a few path changes.

samsonov added inline comments.Sep 22 2015, 3:03 PM
cmake/Modules/CompilerRTDarwinUtils.cmake
107 ↗(On Diff #35420)

Please fix directory name to Darwin-excludes here and below. Or, better, hoist the directory name to a variable.

set(DARWIN_EXCLUDES_DIR ${COMPILER_RT_SOURCE_DIR}/lib/builtins/Darwin-excludes)

and use it from then on.

119 ↗(On Diff #35420)
if (VERSION_MATCHED AND NOT CMAKE_MATCH_1 VERSION_LESS min_version)

?

123 ↗(On Diff #35420)

I don't understand the logic in this function:

  • if I happened to have ios.txt, but not ios6-armv7.txt, this statement will never be executed, and ${arch}_${os}_EXCLUDED_BUILTINS will be empty.
  • if I have ios6-armv7.txt and ios7-armv7.txt, and I configure for armv7 and min_version=6.0, I will set the contents of ${arch}_${os}_EXCLUDED_BUILTINS twice to different values, that would depend on the order of files in glob.

Neither of this look correct to me.

beanz updated this revision to Diff 35426.Sep 22 2015, 3:13 PM
  • Moved exclude dir out to a separate named variable
  • Cleaned up if statement logic
  • Refactored function so that it works if the version-specific blacklists are missing
samsonov added inline comments.Sep 22 2015, 3:50 PM
cmake/Modules/CompilerRTDarwinUtils.cmake
112 ↗(On Diff #35426)

Instead of duplicating this logic several times and inventing tricky variable names, consider adding a function that would take a filename and a list, and would append all lines from a file to a list (if the file exists).

123 ↗(On Diff #35426)

So now if you have ios6-armv7.txt and ios7-armv7.txt, you will use the contents of a random file. Is this expected? Shouldn't you add both instead?

beanz added inline comments.Sep 22 2015, 3:55 PM
cmake/Modules/CompilerRTDarwinUtils.cmake
112 ↗(On Diff #35426)

Sure. Will do.

123 ↗(On Diff #35426)

It shouldn't be random. It should be the first alphabetically, which is the intent. You only want to exclude symbols in the lowest supported version. Symbols that are present in a later version should be included if you're supporting older versions.

Does that make sense?

beanz updated this revision to Diff 35433.Sep 22 2015, 4:03 PM

Cleanup to darwin_find_excluded_builtins_list based on feedback from samsonov.

beanz updated this revision to Diff 35434.Sep 22 2015, 4:07 PM

Sort builtin_lists before iterating. In practice GLOB returns a sorted list, but it isn't documented to do so.

beanz updated this revision to Diff 35435.Sep 22 2015, 4:19 PM

When describing the logic in darwin_find_excluded_builtins_list in my last email I realized it doesn't actually work in the face of 10.10, so I've refactored the code to do a search for the smallest version that is greater than the minimum.

samsonov accepted this revision.Sep 22 2015, 4:53 PM
samsonov edited edge metadata.

LGTM

cmake/Modules/CompilerRTDarwinUtils.cmake
132 ↗(On Diff #35435)

Consider calling it only if smallest_version is non-empty / defined.

This revision is now accepted and ready to land.Sep 22 2015, 4:53 PM
This revision was automatically updated to reflect the committed changes.