Page MenuHomePhabricator

Support for multiarch runtimes layout
ClosedPublic

Authored by phosek on Apr 12 2018, 6:44 PM.

Details

Summary

This change adds a support for multiarch style runtimes layout, so in
addition to the existing layout where runtimes get installed to:

lib/clang/$version/lib/$os

Clang now allows runtimes to be installed to:

lib/clang/$version/$target/lib

This also includes libc++, libc++abi and libunwind; today those are
assumed to be in Clang library directory built for host, with the
new layout it is possible to install libc++, libc++abi and libunwind
into the runtime directory built for different targets.

The use of new layout is enabled by setting the
LLVM_ENABLE_PER_TARGET_RUNTIME_DIR CMake variable and is supported by both
projects and runtimes layouts. The runtimes CMake build has been further
modified to use the new layout when building runtimes for multiple
targets.

Diff Detail

Event Timeline

phosek created this revision.Apr 12 2018, 6:44 PM

Currently everything is a single patch but I'd be happy to split into separate patches and submit those separately against each subproject if you're fine with the overall design.

rnk added a comment.Apr 16 2018, 9:53 AM

The overall goal makes sense to me, but I feel like this patch adds many CMake variables to accomplish it, which are only set from the Fuchsia CMake cache. I feel like it would be simpler to have one CMake flag that clang and the runtime libraries look at to decide if they want to use the old libclang_rt.$arch.$lib paths, or the new clang/$version/$triple/lib/$lib paths.

In other words, we should aim to make the CMake cache scripts as simple as possible. Most developers (so far as I know) don't use the cache scripts to configure their local builds. They directly invoke LLVM's CMake, usually with a shell script with their favorite options. Ideally, changing the runtime library layout this way would be adding a simple flag to that script, rather than requiring developers to switch to these cache scripts or duplicating the logic of the cache scripts.

In D45604#1068915, @rnk wrote:

The overall goal makes sense to me, but I feel like this patch adds many CMake variables to accomplish it, which are only set from the Fuchsia CMake cache. I feel like it would be simpler to have one CMake flag that clang and the runtime libraries look at to decide if they want to use the old libclang_rt.$arch.$lib paths, or the new clang/$version/$triple/lib/$lib paths.

In other words, we should aim to make the CMake cache scripts as simple as possible. Most developers (so far as I know) don't use the cache scripts to configure their local builds. They directly invoke LLVM's CMake, usually with a shell script with their favorite options. Ideally, changing the runtime library layout this way would be adding a simple flag to that script, rather than requiring developers to switch to these cache scripts or duplicating the logic of the cache scripts.

It's not necessarily Fuchsia-specific, but it relies on the runtimes build which is currently only used by Fuchsia and Android AFAIK, that build model makes some things easier, e.g. compiler-rt is always built for a single target only (list of targets is configured via cache files). I'm fine changing this to guard the layout used behind a single CMake variable (e.g. ENABLE_RUNTIME_TARGET_DIR), but it'll require more changes to compiler-rt build, is that fine with you?

phosek updated this revision to Diff 144819.May 1 2018, 6:23 PM

I've modified the patch following @rnk suggestion, the new layout is enabled by a single CMake option CLANG_ENABLE_PER_TARGET_RUNTIME_DIR. I'm still working on fixing tests, but I've tested the build and it's working for both runtimes/ and projects/ layout. Let me know what you think.

phosek added a comment.May 3 2018, 4:50 PM

I've modified the patch following @rnk suggestion, the new layout is enabled by a single CMake option CLANG_ENABLE_PER_TARGET_RUNTIME_DIR. I'm still working on fixing tests, but I've tested the build and it's working for both runtimes/ and projects/ layout. Let me know what you think.

One issue I ran into while working on getting tests to work is that in the current patch, enabling CLANG_ENABLE_PER_TARGET_RUNTIME_DIR enables the new layout in the driver and disables the old one (e.g. when constructing compiler-rt paths). However, there are plenty of tests that hardcode the libclang_rt.builtins-<arch>.a name and those would all have to be disabled when the new layout is enabled. Maybe it'd be better to only enable the new layout but support fallback back onto the old one (i.e. search both the new and the old path for compiler-rt libraries)?

I went on vacation and haven't had more time to look at this since I got back. Maybe @hans and @vitalybuka might be able to help review this.

niosHD added a subscriber: niosHD.May 15 2018, 1:26 AM
hans added a comment.May 16 2018, 7:12 AM

I only looked a little at the Clang parts (the compiler-rt and libcxx cmake stuff I'm not familiar with). It seems reasonable, and would make it easier to ship a cross-compiler I suppose, but I think folks with a higher-level Clang view need to take a look too.

clang/lib/Driver/ToolChain.cpp
383 ↗(On Diff #144819)

Why is it using the default target triple here and not the current one? (same for the ToolChain ctor).

And the #ifdefs seems unfortunate.. could Clang just look in the target-specific directory if it exists and fall back otherwise or something?

filcab added a subscriber: filcab.May 18 2018, 8:21 AM
filcab added inline comments.
clang/lib/Driver/ToolChain.cpp
90 ↗(On Diff #144819)

Maybe use cmakedefine01 and change this to a simple if?

392 ↗(On Diff #144819)

These last two lines can probably get hoisted out of the if and make it easier to notice the difference between the two paths. Same comment as above about using a "normal if" instead of #if.

clang/lib/Driver/ToolChains/Fuchsia.cpp
152 ↗(On Diff #144819)

Why "compute triple + set triple + return Triple.str()" instead of a plain "compute triple + return arch + '-' + os"?

clang/lib/Driver/ToolChains/Fuchsia.h
69 ↗(On Diff #144819)

Why the extra default argument? The patch doesn't seem to show any call to this function that requires a single arg.

beanz added a reviewer: bruno.May 18 2018, 9:57 AM
beanz added a subscriber: bruno.

Looping in @bruno in case he has feedback.

phosek added inline comments.May 23 2018, 11:19 PM
clang/lib/Driver/ToolChain.cpp
383 ↗(On Diff #144819)

getDefaultTargetTriple() returns either default triple or the current one if overriden via --target= option, I just reused the existing value. I think this should be called simply getTargetTriple(), that should be a simple refactoring in Driver (famous last words).

I could do the fallback, I had that implemented in the first version of the patch but then Reid suggested guarding this feature behind a flag. I can still keep the flag, but only use it for CMake and make the driver support both variants.

phosek updated this revision to Diff 148348.May 24 2018, 12:41 AM
phosek marked 6 inline comments as done.
phosek updated this revision to Diff 149839.Jun 4 2018, 12:59 PM
phosek edited the summary of this revision. (Show Details)

All tests now seem to be passing when ENABLE_PER_TARGET_RUNTIME_DIR is set and all prerequisites have landed.

rnk added inline comments.Jun 4 2018, 4:23 PM
compiler-rt/cmake/base-config-ix.cmake
72 ↗(On Diff #149839)

Does this usage still work in a standalone compiler-rt build? I'd expect it might warn about using an undefined variable. You might need to duplicate the option into each top-level cmake and do some ugly if (NOT DEFINED ) hacks, or use some of the existing "if standalone" checks.

phosek added inline comments.Jun 4 2018, 6:24 PM
compiler-rt/cmake/base-config-ix.cmake
72 ↗(On Diff #149839)

Yes, this expression is evaluated as "True if given a variable that is defined to a value that is not a false constant. False otherwise." I checked the standalone build and it's working as before.

Ping? Does anyone have anymore comments?

Does it have to be a single patch?

rnk accepted this revision.Jun 11 2018, 1:03 PM

Does it have to be a single patch?

I think it makes sense for it to be one. It's technically possible to split out the clang changes, but there would be no motivation without changing the installation layout of compiler-rt, libc++, and the rest.

Ping? Does anyone have anymore comments?

Let's land it. If we have anymore feedback, we can fix it in post.

This revision is now accepted and ready to land.Jun 11 2018, 1:03 PM

Should ENABLE_PER_TARGET_RUNTIME_DIR be declared at LLVM's top level CMakeLists.txt? If I read the patch correctly, it's in clang so llvm/runtimes/ relies on the default value being OFF (hard to tell because the patch doesn't follow the current layout of the SVN repository but rather a git monorepo?)

Should ENABLE_PER_TARGET_RUNTIME_DIR be declared at LLVM's top level CMakeLists.txt? If I read the patch correctly, it's in clang so llvm/runtimes/ relies on the default value being OFF (hard to tell because the patch doesn't follow the current layout of the SVN repository but rather a git monorepo?)

I'm fine moving that option to LLVM's CMakeLists.txt, it's more specific to Clang which is why I defined it in Clang's CMakeLists.txt, but it's also needed by all runtimes so LLVM's CMakeLists.txt is probably more appropriate.

phosek updated this revision to Diff 153183.Jun 27 2018, 2:38 PM
phosek edited the summary of this revision. (Show Details)
phosek updated this revision to Diff 153244.Jun 27 2018, 7:14 PM
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: Restricted Project. · View Herald TranscriptJun 27 2018, 8:16 PM