Currently we have two different runtime paths where clang searches for compiler-rt and other runtime libraries and headers.
We have the old path: <root>/lib/clang/X.Y.Z/lib/<os>/libclang_rt.builtins-<arch>.a
And the new path: <root>/lib/clang/X.Y.Z/lib/<target>/libclang_rt.builtins.a
If you build with -DLLVM_ENABLE_PROJECT=compiler-rt it defaults to the old path, if you build with -DLLVM_ENABLE_RUNTIMES=compiler-rt it defaults to the new path then there is the option to force which path to use with -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR.
The new path has some distinct advantages when you have a cross-compiler - so there is good reason to use it.
The current status quo has quite a few problems:
Complexity in the driver
Confusing diagnostic when the driver can’t find the runtime directory
Silent fall-back to the old location creates problems in certain setups.
I suggest we remove the old path option from CMake and the driver for LLVM 16.x to address these problems.
Happy to hear any reasons to keep the current setup.
If the legacy paths were dropped, the above situation would be much less confusing to diagnose, because it would be easy to see that a certain path need to be symlinked.
But there is another layer to it (namely the canonicalization mismatch between cmake and the driver). Ideally, the build shouldn’t require symlinking of runtime paths in any configuration.
Yeah I agree we need some kind of normalization as well. But removing the legacy path will be a good step in the right direction to remove the confusion around this right now.
Can you clarify what you mean by this? For these compiler-rt files, we have both os+arch in the old paths too, so they work well for cross compilation already. (I do admit that having a full triple gives more flexibility than just plain os+arch, but I’d like you to clarify what advantages you mean here.)
I totally disagree. If the new configuration isn’t working everywhere (if the purpose is to make it easier to diagnose issues with the new setup, if one is about to fix that with symlinks), we can’t just remove the old one and force migration that way (forcing everybody to try to fix the issue with symlinks). The new way should be working for everyone before we pull the plug on the old setup.
As noted, there are frequent issues with the per-target runtimes directories, with mismatches in triples wrt normalization, that haven’t really been sorted out yet, see e.g. ⚙ D107799 [CMake] Enable LLVM_ENABLE_PER_TARGET_RUNTIME_DIR by default on Linux for discussion of it as recent as yesterday, trying to sort out issues relating to it.
In particular, last I checked, Clang needed explicit support for each target, for it to find the per-target runtimes. (That’s the case for the libc++ include paths though, it’s possible that the compiler-rt runtimes paths are handled by generic code that enables it for all targets.) See e.g. ⚙ D107893 [clang] [MinGW] Consider the per-target libc++ include directory too - at the time (one year ago), only the Gnu and Fuchsia parts in Clang supported the per-target layout for libc++ (after that commit also the MinGW target).
I guess there are two different problems here - let’s talk about the first one, I have in the past been needed to target different configurations of the same os-arch like: aarch64-linux-android21 and arch-linux-android22 (these are just examples - we also had arm platforms with different asm optimizations) - so the target expresses more information here.
The second problem is as you note below libc++ - currently we cross-compile from windows to linux (in different flavors) and if we don’t use the per target runtime it actually puts the C++ lib and headers in the same dir.
I totally disagree. If the new configuration isn’t working everywhere (if the purpose is to make it easier to diagnose issues with the new setup, if one is about to fix that with symlinks), we can’t just remove the old one and force migration that way (forcing everybody to try to fix the issue with symlinks). The new way should be working for everyone before we pull the plug on the old setup.
This is of course correct - I don’t want to remove the old setup until everything is working as it should on the new layout. I was not aware of the problems being raised in D107799 [CMake] Enable LLVM_ENABLE_PER_TARGET_RUNTIME_DIR by default on Linux so thanks for that link. That also seems to be a step on what I want, with making it the default - which is probably also good to find the issues that are still lingering.
I don’t think we disagree that much here, of course the bugs needs to be fixed, and I was not aware of the ones you describe - because currently where we juggle two different layouts is probably the worst of the choices we have in-front of us.
The other issue to tackle there before it can be considered for libc++, is to make sure that all targets under lib/clang/Driver/ToolChains, which currently support libc++, are updated to use the per-target libc++ directories too.
Just for the record - this aspect hasn’t been an issue in the mingw configurations (where I don’t build libc++ as part of the main llvm build with LLVM_ENABLE_RUNTIMES, but building them standalone one arch at a time), as I build each arch version of those runtimes individually with -DCMAKE_INSTALL_PATH=<toolchain>/<arch-triple>, so that they’re already in their own arch-specific sysroot subdirectory, e.g. <toolchain>/x86_64-w64-mingw32/lib/libc++.a. With the per-target runtime directories (and switching to building with -DCMAKE_INSTALL_PATH=<toolchain>), this becomes <toolchain>/lib/x86_64-w64-mingw32/libc++.a.
The main difference there is that the mingw driver in clang has its own logic for looking for sysroots in <clang-exe>/../<triple>, and with mingw specific normalization of the triples.
So for those configurations, the new per-target runtime directories doesn’t bring anything new, but it switches it from target specific behaviours towards more common/standardized behaviours.
One thing we could do is make the per-target runtime dirs the default in all configurations, and then we can keep the old behavior around until the remaining issues have been resolved. That would at least get the ball rolling on what I think we all agree is the direction we all want going forward.
Yeah I think this is a good way forward - my first message was probably a little optimistic. But I think it’s reasonable to make the new layout default in 16 and see if that leads us to the possibility to remove it in 17 or later.
I haven’t read the whole thread, but as a first step, we should change the clang driver to use an ifdef rather than FS detection. This should help address two of the issues in your list, driver complexity and issues falling back to the old layout.
Removing the CMake option should be a separate change, which is risky, and will likely need to be progressively landed, reverted, adjusted for some platformed, and generally haggled over until the end of time.
In general, the per-triple layout is really nice in theory, but you run into all sorts of issues with matching against the triple in practice, and like others have said, I’d be opposed to removing the old layout until we have all the issues with the new layout sorted.
This has been my long-term goal so thank you for proposing this. I’m in favor but I agree that we would need to address the existing issues that have already been raised in this thread.
I’ve been doing some local prototyping. Concretely, two design directions I’ve been exploring:
Give driver toolchain subclasses control over target spelling/directory names with a generic fallback for platforms that don’t need special handling (which I expect is going to be most of them). This is not easily doable today because the target-dependent path handling is done in the constructor where you cannot call the overridden methods. I think we should refactor the implementation of existing toolchain subclasses, making constructors trivial and move the path handling into a separate initialization method. This would also let us move some of the existing target-specific logic that currently lives inside the Driver class.
Teach the runtimes build to query the paths from Clang rather than constructing them manually. This would ensure that paths match and would let us remove some of the complexity from our build where we duplicate the path construction logic both in CMake and Clang.
If you think that these directions make sense, I can try to find some time to cleanup my changes and upload them for review in the coming weeks.
This is already supported when LLVM_ENABLE_PER_TARGET_RUNTIME_DIR is enabled for Gnu, Fuchsia and soon also Windows toolchains. We install the library in <toolchain>/lib/<triple>/libc++.a and __config_site header in <toolchain>/include/<triple>/c++/v1 (__config_site is the only header that differs across targets). In the future, this would ideally be supported for all toolchains.
FWIW, one of the practical problems with change like these, is getting enough feedback during the development cycle. Like changes with building runtimes with LLVM_ENABLE_RUNTIMES, which was merged before the 14 release, but where parts of it had to be reverted/reversed during the 14 release cycle (due to e.g. issues with how to run the tests for the runtimes, and iirc some release configurations were made by moving the runtimes back to LLVM_ENABLE_PROJECTS). Now during the 15 release cycle, those issues still do seem to be unsolved still, and it’s unclear if we’ll have a good final solution in place during the time window for this release.
So all these procedures, be that test-release.sh or other downstream packaging, plus testing of the packaged versions in each vendor’s environment where it’s actually used in the wild, would need to be done semi-continuously on the main branch inbetween releases too. Once downstreams/vendors start trying these things out on RC1, it’s often practically too late to sort out any big issues.
I totally agree with this - but considering the project size and organization I am not sure what the best way to fix this is and it probably warrants another discussion thread.
If you build with -DLLVM_ENABLE_PROJECT=compiler-rt it defaults to the old path, if you build with -DLLVM_ENABLE_RUNTIMES=compiler-rt it defaults to the new path then there is the option to force which path to use with -DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR .
Correction: main and release/15.x enable LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=on by default on non-arm Linux.
@tobiashieta Just for awareness, I saw a lot of build breakage recently with OpenMP (Build issues and limitations with OpenMP on Arch Linux) when going the LLVM_ENABLE_RUNTIMES route, which I did not get when using the LLVM_ENABLE_PROJECTS route. Due to the amount of breakage it also seems to me that this configuration is not well tested or else it would have caught the eyes of the OpenMP devs sooner. Since I wrote that entry LLVM_ENABLE_RUNTIMES broke yet again on main. From a end user perspective, please make sure that everything works with using the new method before phasing out the old way.
More background on this: we use remote builders where we don’t ship compiler-rt libraries during compiles to save some space/time. Clang checks the old and new runtime paths and falls back to the old runtime path if it doesn’t see either, then at link time it can’t find compiler-rt libs at the old path.
More details about this issue in this bug. Getting this fixed before flipping the default would be nice. (in Chrome we’ve turned on LLVM_ENABLE_PER_TARGET_RUNTIME_DIR everywhere except Windows due to this bug, and Android due to the API version numbers in triples issue)
D107799 [CMake] Enable LLVM_ENABLE_PER_TARGET_RUNTIME_DIR by default on Linux