This is an archive of the discontinued LLVM Phabricator instance.

[CMake] merge add_compiler_rt_runtime and add_compiler_rt_darwin_runtime into a single function
ClosedPublic

Authored by beanz on Aug 24 2015, 10:31 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

beanz updated this revision to Diff 32973.Aug 24 2015, 10:31 AM
beanz retitled this revision from to [CMake] merge add_compiler_rt_runtime and add_compiler_rt_darwin_runtime into a single function.
beanz updated this object.
beanz added a subscriber: llvm-commits.
beanz updated this revision to Diff 32978.Aug 24 2015, 11:04 AM

Forgot to save my latest changes to ubsan/CMakeLists.txt

Hi Chris,

Have you tested this on Linux? Maybe some ARM/AArch64, too?

cheers,
--renato

This comment was removed by rengolin.
beanz added a subscriber: beanz.Aug 24 2015, 1:34 PM

I have tested it on Linux. I have not tested Linux ARM/AArch64, as I don’t have hardware for that config. I have tested darwin configs.

-Chris

beanz updated this revision to Diff 32993.Aug 24 2015, 1:42 PM

In working on follow-up patches I did some cleanup that applied to these patches, and fixed an issue with hooking up dependencies.

samsonov added inline comments.Aug 24 2015, 2:24 PM
cmake/Modules/AddCompilerRT.cmake
59 ↗(On Diff #32993)

LIBS->LINK_LIBS (specify that it makes sense for shared libraries only)

63 ↗(On Diff #32993)

I would leave SHARED/STATIC as type argument of add_compiler_rt_runtime function. Otherwise you can specify both STATIC and SHARED, which makes no sense. You should also diagnose if none is specified.

77 ↗(On Diff #32993)

Looks like type_${libname} will be the same for all ${libname}, you don't really need it now.

101 ↗(On Diff #32993)

Note that currently we pass LIB_CFLAGS to as linkflags as well, you probably want to keep this behavior for now.

101 ↗(On Diff #32993)

There is no such thing as ${TARGET_${arch}_LINKFLAGS}

103 ↗(On Diff #32993)

You've already checked for that.

114 ↗(On Diff #32993)

You've already checked for that.

121 ↗(On Diff #32993)

You can probably do

if(NOT libnames)
  return()
endif()

and skip all the checks below.

123 ↗(On Diff #32993)

COMPONENT_OPTION

141 ↗(On Diff #32993)

This whole OUTPUT_NAME logic looks terrible now, especially assuming you can have several ${libname} libraries, but you pass a single OUTPUT_NAME for them. Ugh. Looks like it's only really used on Windows, maybe you will just adjust for naming difference on Windows and Linux at the place where you generate ${libname}, and get rid of this argument?

143 ↗(On Diff #32993)

Only if we're linking SHARED compiler-rt runtime?

lib/dfsan/CMakeLists.txt
25 ↗(On Diff #32993)

Why don't you use PARENT_TARGET here (and in similar places below)?

beanz added a comment.Aug 24 2015, 2:42 PM

A few comments inline. I'll work up patches to address your feedback and send them out shortly.

cmake/Modules/AddCompilerRT.cmake
63 ↗(On Diff #32993)

There are situations where we build both static and dynamic libs. tsan/dd/CMakeLists.txt is one example. I was hoping to collapse that all down to one call to add_compiler_rt_runtime

If you think it is better to support only one type I can do that. It will allow me to simplify some of the body of add_compiler_t_runtime too.

141 ↗(On Diff #32993)

Yea... This is actually broken in my patches, and I think the behavior is broken. I'll try and revise it to be more sane.

samsonov added inline comments.Aug 24 2015, 2:54 PM
cmake/Modules/AddCompilerRT.cmake
63 ↗(On Diff #32993)

Yes, let's just support one type for now. If absolutely necessary, we could just call add_compiler_rt_runtime twice (we most likely would need to do it anyway, because CFLAGS/DEFS would be different in that case).

beanz updated this revision to Diff 33014.Aug 24 2015, 3:23 PM

Updates based on feedback from samsonov.

  • Made add_compiler_rt_runtime only handle one type of library per call
  • General cleanup
  • Fixed a bunch of places where I was missing PARENT_TARGET settings
  • Removed OUTPUT_NAME and special cased WIN32
samsonov added inline comments.Aug 24 2015, 4:45 PM
cmake/Modules/AddCompilerRT.cmake
60 ↗(On Diff #33014)

There is no such argument now.

79 ↗(On Diff #33014)

you don't use LIB_CFLAGS in add_compiler_rt_darwin_runtime(.. SHARED ...).

99 ↗(On Diff #33014)

You should also set output_name for shared libraries on Unix - ${name}-${arch}${COMPILER_RT_OS_SUFFIX} (note: no dynamic here)... which probably means it's easier to set output_name_${libname} for *all* libraries here, in this loop, and then just remove special case from the loop below.

lib/asan/CMakeLists.txt
194 ↗(On Diff #33014)

Pass ${ASAN_DYNAMIC_LIBS} to add_compiler_rt_runtime as LINK_LIBS?

lib/profile/CMakeLists.txt
21 ↗(On Diff #33014)

Please leave the loop over PROFILE_SUPPORTED_ARCH here for this change, and let's collapse it here (and in similar places, like lsan) in subsequent changes.

lib/tsan/CMakeLists.txt
112 ↗(On Diff #33014)

PARENT_TARGET tsan

lib/tsan/dd/CMakeLists.txt
43 ↗(On Diff #33014)

Now this is named LINK_LIBS

beanz added a comment.Aug 24 2015, 5:39 PM

Updated patches coming. One comment below.

lib/profile/CMakeLists.txt
21 ↗(On Diff #33014)

Please make up your mind over whether or not you want me to adapt the CMakeLists.txt files in this patch. In your comments here you have both asked me to make more changes to the CMakeLists files, and to make less changes.

samsonov added inline comments.Aug 24 2015, 5:56 PM
lib/profile/CMakeLists.txt
21 ↗(On Diff #33014)

Sorry if I wasn't clear enough. I suggest to leave

foreach(arch ${XXX_SUPPORTED_ARCH})

in every lib/xxx/CMakeLists.txt as is: that is, pass a single arch to add_compiler_rt_runtime on non-Apple platforms for now.

beanz added inline comments.Aug 25 2015, 8:34 AM
lib/profile/CMakeLists.txt
21 ↗(On Diff #33014)

Why? Without this change there is no code testing the new loop.

I need you to be clear about what you want in patches. Your feedback has been very valuable, but inconsistent. Much of your feedback was calling out places where I wasn't using new functionality that these patches introduced, except this piece which is the opposite.

In writing my patches I'm trying to keep non-essential changes to a minimum, but I do want to make enough changes to exercise the new functionality. I didn't change every place to PARENT_TARGET or LINKLIBS because I figured those could be small easy follow-up patches. Your feedback has been to use those features, but not to use the multi-architecture support.

I need to understand why so that I can write my patches closer to how you want them the first time.

beanz updated this revision to Diff 33086.Aug 25 2015, 10:17 AM

Updates based on feedback from samsonov.

beanz updated this revision to Diff 33087.Aug 25 2015, 10:36 AM

Fixing missing '_' in LINK_LIBS

samsonov accepted this revision.Aug 25 2015, 11:36 AM
samsonov edited edge metadata.

LGTM

lib/asan/CMakeLists.txt
192 ↗(On Diff #33087)

LINK_LIBS

lib/profile/CMakeLists.txt
21–26 ↗(On Diff #33087)

Thank you for bringing this up, as it seems it to be the main source of confusion in the review thread (and sorry for not making my point clear). As I understand it now, you want to:

  1. implement and commit add_compiler_rt_runtime with all the features you find useful (LINK_LIBS, PARENT_TARGET, multi-arch support)
  2. exercise each of new features in 1-2 places to have them tested, but keep the change small.
  3. (in a follow-up change) migrate everyone else to use new features.

While I was more used to (and tried to push you in that direction) a different approach, which keeps the build rules for various compiler-rt runtimes consistent at every revision: if there's a feature in add_compiler_rt_runtime function they use, then either all of them make use of it, or none of them. In that case the path could be:

  1. add simple and short version of add_compiler_rt_runtime and migrage build rules to use it
  2. (in a separate change) add PARENT_TARGET argument, and use it where applicable
  3. (in a separate change) add LINK_LIBS argument, and use it where applicable.
  4. (in a separate change) add multi-arch support and use it

...

It could make the changes smaller and more granular, and (as mentioned earlier) the code would be consistent at every point. On a flip side, each change would touch multiple files across a lot of directories. It's a matter of taste, really, and it's probably not worth to split this change into several smaller ones at this point. I'll be happy either way as long as you will lay out the plan for further refactoring in a commit message.

This revision is now accepted and ready to land.Aug 25 2015, 11:36 AM
This revision was automatically updated to reflect the committed changes.