This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Cleanup add_compiler_rt_object_library to be platform-agnostic
ClosedPublic

Authored by beanz on Jun 4 2015, 11:29 AM.

Details

Summary

This change takes darwin-specific goop that was scattered around CMakeLists files and spread between add_compiler_rt_object_library and add_compiler_rt_darwin_object_library and moves it all under add_compiler_rt_object_library.

The goal of this is to try to push platform handling as low in the utility functions as possible.

Diff Detail

Event Timeline

beanz updated this revision to Diff 27134.Jun 4 2015, 11:29 AM
beanz retitled this revision from to [CMake] Cleanup add_compiler_rt_object_library to be platform-agnostic.
beanz updated this object.
beanz edited the test plan for this revision. (Show Details)
beanz added a subscriber: Unknown Object (MLST).
rsmith added a subscriber: rsmith.Jun 4 2015, 1:47 PM
rsmith added inline comments.
cmake/Modules/AddCompilerRT.cmake
5–6

This comment needs to be updated.

lib/asan/CMakeLists.txt
79

It seems weird for the dynamic library target to have the name RTAsan for APPLE but to be named RTAsan_dynamic everywhere else. Is it possible to merge these two targets and move them out of the if here? (Not as part of this change, just curious if we can unify this further.)

lib/interception/CMakeLists.txt
14–19

Do you still need the if here? Both arms look the same (other than the OS part which is ignored for the non-APPLE case).

lib/lsan/CMakeLists.txt
20–27

Likewise, can you move this part out of the if?

beanz added a comment.Jun 4 2015, 2:05 PM

I will send updated patches in a minute once I finish testing them.

cmake/Modules/AddCompilerRT.cmake
5–6

Will do.

lib/asan/CMakeLists.txt
79

I hope to reduce this further in another patch. I really want to get to the point where there is no platform handling in the CMakeLists files for libraries.

lib/interception/CMakeLists.txt
14–19

Yea, I can get rid of the if here. I was going to do that in a later patch, because I didn't realize the OS parameter was the only difference.

lib/lsan/CMakeLists.txt
20–27

Yep, I can do that here too. That will get rid of the Apple side of this conditional.

beanz updated this revision to Diff 27146.Jun 4 2015, 2:08 PM

Updating patches based on feedback from Richard Smith.

  • Updated comments on add_compiler_rt_object_library
  • Moved a few calls to add_compiler_rt_object_library out of their 'if' blocks
rnk added a subscriber: rnk.Jun 4 2015, 4:10 PM

Seems like a nice win. :)

cmake/Modules/AddCompilerRT.cmake
5–6

I think this needs more updating, it's no longer adding a single object library, it actually creates an object library for each supported OS or architecture. add_compiler_rt_object_libraries might be a better name too.

8

Can you make this a function instead of a macro? Then you don't leak all these variables into the parent scope.

samsonov added inline comments.Jun 4 2015, 5:50 PM
lib/lsan/CMakeLists.txt
22

It's weird that we now use SANITIZER_COMMON_SUPPORTED_DARWIN_OS on non-Darwin platforms.

beanz added a comment.Jun 5 2015, 1:12 PM

I'll send updated patches shortly.

cmake/Modules/AddCompilerRT.cmake
5–6

Will do.

8

Will do.

lib/lsan/CMakeLists.txt
22

This is weird, but I see it as a temporary weirdness. I'm hoping this will all fold away. If you have a preference on how to make it less weird I'm open to suggestions.

My thought, was to eventually just change that to SANITIZER_COMMON_SUPPORTED_OS, and to set it appropriately for whatever OS you are targeting.

beanz updated this revision to Diff 27222.Jun 5 2015, 1:16 PM

Updating based on feedback from Reid.

rnk accepted this revision.Jun 8 2015, 12:53 PM
rnk added a reviewer: rnk.

lgtm

This revision is now accepted and ready to land.Jun 8 2015, 12:53 PM
samsonov accepted this revision.Jun 9 2015, 6:09 PM
samsonov edited edge metadata.

LGTM, but please address comments below.

lib/interception/CMakeLists.txt
15

Here as well. Or rename SANITIZER_COMMON_SUPPORTED_DARWIN_OS to smth. more appropriate first.

lib/lsan/CMakeLists.txt
22

I'd still use

if (APPLE)

clause here and avoid using reference to DARWIN_OS on Linux.

This revision was automatically updated to reflect the committed changes.