This refactoring moves much of the Apple-specific behavior into a function in AddCompilerRT. The next cleanup patch will remove more of the if(APPLE) checks in the outlying CMakeLists.
Details
- Reviewers
glider filcab samsonov kubamracek zaks.anna bogner - Commits
- rGd160260681a1: [CMake] merge add_compiler_rt_runtime and add_compiler_rt_darwin_runtime into a…
rCRT245970: [CMake] merge add_compiler_rt_runtime and add_compiler_rt_darwin_runtime into a…
rL245970: [CMake] merge add_compiler_rt_runtime and add_compiler_rt_darwin_runtime into…
Diff Detail
- Repository
- rL LLVM
Event Timeline
Hi Chris,
Have you tested this on Linux? Maybe some ARM/AArch64, too?
cheers,
--renato
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
In working on follow-up patches I did some cleanup that applied to these patches, and fixed an issue with hooking up dependencies.
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)? |
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. |
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). |
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
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 |
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. |
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. |
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. |
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:
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:
... 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. |