This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Support multi-target runtimes build
ClosedPublic

Authored by phosek on May 3 2017, 10:34 AM.

Details

Summary

This changes adds support for building runtimes for multiple different targets using LLVM runtimes directory.

The implementation follow the model used already by the builtins build which already supports this option. To specify the runtimes targets to be built, use the LLVM_RUNTIME_TARGETS variable, where the valuae is the list of targets to build runtimes for. To pass a per target variable to the runtimes build, you can set RUNTIMES_<target>_<variable> where <variable> will be passed to the runtimes build for <target>.

Diff Detail

Repository
rL LLVM

Event Timeline

phosek created this revision.May 3 2017, 10:34 AM

Example use case of this feature is D32817.

phosek added inline comments.May 4 2017, 11:53 AM
cmake/modules/LLVMExternalProjectUtils.cmake
185 ↗(On Diff #97684)

I'm not sure on what's the best way to handle the install prefix. When building runtimes for multiple targets, only one of them can use the "default" install prefix, others have to use a different one otherwise they'll overwrite each other files.

phosek updated this revision to Diff 98224.May 8 2017, 3:29 PM
chapuni added a subscriber: chapuni.May 8 2017, 3:30 PM
aoli added a subscriber: aoli.May 24 2017, 10:36 AM
jroelofs added inline comments.
cmake/modules/LLVMExternalProjectUtils.cmake
185 ↗(On Diff #97684)

I'd shove them all under: lib/clang-runtimes/$triple/{lib,include}, and specifically not use a "default" install prefix.

aoli added a comment.EditedMay 24 2017, 5:43 PM

Hi I did some tests about this patch and I got one question.

Due to include(HandleLLVMOptions) compiling runtimes will require libc++ for targets. If we don't have libc++ the test in cmake/modules/CheckCompilerVersion.cmake: 38 will fail. Since compiling compiler-rt does not require libc++ do we really need to include this file here? This not a problem for building runtimes for host because libc++ is always installed on hosts. But targets may not have libc++.

aoli added inline comments.May 25 2017, 10:36 AM
runtimes/CMakeLists.txt
314 ↗(On Diff #98224)

Shall we also pass CMAKE_BUILD_TYPE here?

jroelofs added inline comments.May 25 2017, 3:12 PM
runtimes/CMakeLists.txt
314 ↗(On Diff #98224)

@aoli making the runtimes have the same build type as the compiler? No. That should only be passed via RUNTIMES_${target}_CMAKE_BUILD_TYPE=

phosek updated this revision to Diff 100359.May 25 2017, 8:45 PM
phosek added inline comments.May 25 2017, 8:52 PM
cmake/modules/LLVMExternalProjectUtils.cmake
185 ↗(On Diff #97684)

Now they will be installed in $CMAKE_INSTALL_PREFIX/$target, but I'd be happy to change that to lib/runtimes/$target. One issue I've noticed is that sanitizer and xray headers end up in lib/runtimes/$target/lib/clang/5.0.0/include. I don't know if those headers are target specific, but if not it would be better to install them in lib/clang/5.0.0/include same as for compiler headers.

beanz added inline comments.May 31 2017, 10:44 AM
cmake/modules/LLVMExternalProjectUtils.cmake
185 ↗(On Diff #97684)

The sanitizer headers are not (to my knowledge) sanitizer specific, so we should be able to install them once to /lib/clang/$version/include.

For the general target library path I think we need a more complicated solution if we want the install rules to be useful.

Static archives from compiler-rt should install to /lib/clang/$version/lib/$sys/, and shared libraries (and static archives from other runtime projects) should install to /lib/$target/, except in the case where $target == host, in which case it should just be /lib.

I'm not entirely sure how complicated this would be to implement, but I expect it will require changes to compiler-rt's build system to differentiate install paths of static and shared libraries.

I realized that the current solution isn't going to work for compiler-rt since Clang driver expects the sanitizer libraries to end up in /lib/clang/$version/lib/$sys/. The issues is that compiler-rt already has it's own way of handling different targets, so we should use it rather than having to modify both the compiler-rt build and Clang driver. However, we need a solution for other libraries,: libunwind, libc++abi and libc++. One option would be to build compiler-rt runtimes separately, similarly to builtins build and then build the rest of libraries using the runtimes/CMakeLists.txt file (what this change does) and use the CMAKE_INSTALL_PREFIX to specify the installation target. Alternative would be to add a new variable to libunwind's, libc++abi's and libc++'s CMakeLists.txt that will allow overriding the installation path; compiler-rt already uses such variable: COMPILER_RT_INSTALL_PATH so we could probably use <RUNTIME>_INSTALL_PATH. Do you have any preference or opinion about any of these solutions?

I have implemented the latter solution, adding <RUNTIME>_INSTALL_PATH to libunwind, libc++abi, libc++ and it's working. The headers in this case are installed to /lib/$target/include and libraries to /lib/$target/lib, except for compiler-rt where headers are installed to /lib/clang/$version/include and libraries to /lib/clang/$version/lib as today. The changes required to Clang drivers are going to be similar to D32613. It's little strange for headers to be /lib, but this is already the case for compiler-rt today, so this solution follows the same pattern. I'd be happy to upload all the patches if this solution is fine with you.

Splitting libraries and headers, e.g. into /lib/$target and /include/$target, would require introducing pair of variables to each runtime, e.g. <RUNTIME>_HEADER_INSTALL_PATH and <RUNTIME>_LIBRARY_INSTALL_PATH. Unfortunately, I don't think we can shared headers between targets because libc++ generates target specific __config file (all other headers should be the same).

phosek updated this revision to Diff 100972.Jun 1 2017, 12:49 AM

Uploaded the changes to runtimes as D33760, D33761 and D33762.

jroelofs added a subscriber: clm.Jun 5 2017, 10:22 AM
jroelofs added inline comments.
cmake/modules/LLVMExternalProjectUtils.cmake
185 ↗(On Diff #97684)

@beanz what about shared libs from the builtins build? shouldn't those still end up in /lib/clang/$version/lib/$sys/, since they're tied to the specific version of the compiler, unlike the rest of the runtime bits?

phosek updated this revision to Diff 101999.Jun 9 2017, 12:35 AM
phosek updated this revision to Diff 102255.Jun 12 2017, 4:02 PM
beanz added inline comments.Jun 12 2017, 4:43 PM
cmake/modules/LLVMExternalProjectUtils.cmake
185 ↗(On Diff #97684)

Presently we don't support shared libs from the builtins build at all. If we did, you'd likely want them to behave like the shared libraries from other runtime projects because you'd want them to be part of the sysroot for the cross target. At least, that's how I would assume you'd want them to work.

I've added a proper handling of check- and install- targets from subcomponents which were broken in the previous version as @mcgrathr pointed out while testing this change. This version seems to be working fine in my testing when combined with D33760, D33761, D33762, D32817 and D32613.

jroelofs added inline comments.Jun 13 2017, 7:16 AM
cmake/modules/LLVMExternalProjectUtils.cmake
185 ↗(On Diff #97684)

good point.

phosek added inline comments.Jun 13 2017, 4:09 PM
cmake/modules/LLVMExternalProjectUtils.cmake
185 ↗(On Diff #97684)

I looked into different runtimes, libunwind, libc++abi and libc++ all define ${runtime}_LIBRARY_PATH which could be used to install libraries for different targets into /lib/$target. Most headers are not target-specific, the only exception being __config which for some targets is generated by concatenating generated __config_site with __config. There're ways around this. One would be to completely ignore such targets. The other would be to surround the generated __config_site content with #if defined(...)/#endif block. This is going to require some libc++ changes though. Separating static and shared libraries in compiler-rt should be relatively straightforward: all runtimes use add_compiler_rt_runtime CMake function which sets both ARCHIVE and LIBRARY destination in the install target to COMPILER_RT_LIBRARY_INSTALL_DIR, so we just need to introduce another variable, e.g. COMPILER_RT_ARCHIVE_INSTALL_DIR. We would also need to modify Clang ToolChain; getCompilerRT currently assumes that both static and shared libraries are in the same directory, so we would need to change that.

This solution would require more changes in different components than the current version, but most of them (except maybe the libc++ __config_site) are pretty straightforward. If you think this is a cleaner solution, I'd be happy to go ahead and implement those changes, but I'd like to finally make some progress on this change. Do you think it'd make sense to start a thread on llvm-dev to make sure everyone is on board?

@beanz do you have any comments about the CMake implementation bits?

beanz edited edge metadata.Jun 19 2017, 9:06 AM

One question inline below.

cmake/modules/LLVMExternalProjectUtils.cmake
198 ↗(On Diff #102255)

I'm a little confused why this is needed. Isn't ARG_EXTRA_TARGETS a list? This code looks like it is expecting ARG_EXTRA_TARGETS to be a list of lists where the inner list is colon-separated instead of semi-colon separated.

phosek added inline comments.Jun 19 2017, 11:44 AM
cmake/modules/LLVMExternalProjectUtils.cmake
198 ↗(On Diff #102255)

Yes, correct. The problem is that llvm_ExternalProject_Add adds a custom target with the ${target} name that triggers a build for external project's ${target} target. In a single target scenario, this is fine because both targets would be something like install-asan, but in multi-target case we need to use a different target name in the main project to avoid collision (e.g. x86_64-unknown-linux-gnu-install-asan) and different in the subproject (i.e. install-asan), so I use : to optionally separate subproject target name from the target name. I don't know what would be the best way to do this, I also considered using global variables (.e.g <target>_TARGET_NAME but that feels icky) or maybe an additional argument?

beanz accepted this revision.Jun 19 2017, 11:48 AM

CMake stuff all looks good to me!

cmake/modules/LLVMExternalProjectUtils.cmake
198 ↗(On Diff #102255)

Ok. I understand what you're doing here, and it makes sense.

This revision is now accepted and ready to land.Jun 19 2017, 11:48 AM

Thanks! Would it be also possible to look at D33760, D33761 and D33762 which are needed for this change?

phosek updated this revision to Diff 104062.Jun 26 2017, 6:14 PM

Updated the patch to use the relative <runtime>_INSTALL_PREFIX property.

pirama added a subscriber: pirama.Jul 6 2017, 3:52 PM
phosek updated this revision to Diff 105958.Jul 10 2017, 6:44 PM

Rebased on top of the latest runtimes build change.

This revision was automatically updated to reflect the committed changes.