I’d be fine switching from runtime detection to conditional compilation. The current behavior produces confusing error messages when the runtimes can’t be found which has caused issues for our users as well. I can make that change unless someone is already working on it.
I can totally agree about the confusing error messages. However, instead of having it as a hard ifdef in clang, I would like to have a runtime option for overriding it as well (with the default settable via e.g. a cmake option). All those kinds of hardcoded built-in defaults are problematic if using one clang binary for various cross compilation cases with varying configuration - e.g. if wanting to use a distro-provided prebuilt clang in a different setup.
We could introduce a -fexperimental-per-target-runtime-dir flag where the default would be controlled by a CMake option, ToolChain subclasses should also be able to set their own defaults.
I like this plan!
Yes the one sentence version is that the current implementation looks for a triple like arm.*-gnueabihf or arm.*-gnueabi then looks for either arm or armhf libraries (in the legacy layout that is).
For example armv8l-unknown-linux-gnueabihf would look for a armhf library.
So we (Arm folks and Linaro) need to deal with 2 things I think:
- We are still building with compiler-rt in PROJECTS instead of RUNTIMES. (this may turn out to be orthogonal)
- Are we ok having the libraries in a path matching the triple?
I think the answer to 2 is yes but I haven’t got into the ramifications of that yet. It may be a case of just removing the special case code that detects gnueabihf in the first place and letting the defaults do the work.
@tobiashieta The new scheme isn’t compatible with how the builtins and sanitizer runtime libraries (and maybe others too) are currently built for Apple platforms. Apple platforms currently build fat binaries. Fat binaries contain the binaries for multiple architectures within a single binary.
So currently there is something like.
<root>/lib/clang/X.Y.Z/lib/darwin/libclang_rt.osx.a<root>/lib/clang/X.Y.Z/lib/darwin/libclang_rt.asan_osx_dynamic.dylib
that contain multiple architectures.
We need to think carefully how we make the transition to continue supporting Apple platforms.
I have a few thoughts.
-
For builltins where the archive lives an internal implementation detail of the compiler only (i.e. there’s no runtime path to look up). So for builtins it’s conceivable that Clang could switch to the new path layout for the builtin libraries for Apple platforms without being too disruptive.
-
The dylibs are a different matter. These libraries need to be found at runtime so renaming them can cause problems at runtime and in Apple’s case the new naming scheme does not support the existing fat binary behavior. From a build system perspective one way out of this problem might be to actually switch everything to build with the new layout (e.g.
<root>/lib/clang/X.Y.Z/lib/arm64-apple-darwin/libclang_rt.asan_osx_dynamic.dylib) and then for Apple platforms a second build step would run that would combine the thin libraries into a fat library and put them in a location that clang knows to look for (e.g.<root>/lib/clang/X.Y.Z/darwin/libclang_rt.asan_osx_dynamic.dylib). This approach would- Unblock the transition to the new library path
- let us simplify the build system side of things (currently it’s a huge mess)
- allow the existing behavior we have for Apple platforms
However, this kind of change would likely be a lot of work and I’m not sure if there is really an appetite for it.
I will ping my other Apple colleagues about this as I don’t work much on the Sanitizers any more.
Thanks for the context! I think the best option we have seen so far is @petrhosek suggestion to move the logic to the toolchain subclasses. that way the Darwin toolchain could stay on the current layout until / if you guys want to redo it.
I’m aware of these constraints and I believe they could be addressed by the proposal I outlined above. Specifically, if we let ToolChain subclasses override the runtime paths and we’ll teach the CMake build to query Clang rather than constructing the paths, then we could preserve the existing behavior for Apple platforms while also removing the conditional behavior that exists today.
1 Like
That SGTM. Please do add some Apple folks to any patches put up so they can take a look. I’d suggest Alex Lorenz, Steven Wu, and Julian Lettner.
Feel free to add me too but don’t wait for my review as my response time is pretty slow these days.
At Homebrew, we configure LLVM with
-DRUNTIMES_CMAKE_ARGS=-DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=OFF
on Linux.
This was motivated by a desire to fix a build with -DLLVM_ENABLE_RUNTIMES=libc++. See llvm: Re-enable and fix libc++ on Linux. by chandlerc · Pull Request #23339 · Homebrew/linuxbrew-core · GitHub.
I’m not sure if this has been covered in the problems mentioned above that need resolving, or if the issues we were trying to fix have been resolved since then, so I’m tagging @chandlerc, who should be able to elaborate more on this.
I have landed ⚙ D140011 [clang][compiler-rt] Support LLVM_ENABLE_PER_TARGET_RUNTIME_DIR on Arm Linux and BSD which adds support for the new layout on Arm and flips the default to ON for 17.x onward.
I do not think changing the default for 16.x is worth the risk at this point so it will remain OFF there.
Thanks for working on this! I look forward to being able to harmonize these paths!
With that blocker removed for 17 - is there something else that needs to be fixed before we can deprecate the old runtime paths? Would be nice to get that done before 17 in that case.
Thanks,
Tobias
IIRC some aspects of the per-target runtime directory handling lies in the target specific files in clang/lib/Driver/ToolChains, and it is only handled in some of them, not in all of them. So for those targets that haven’t actively tried out the per-target runtime layout yet, chances are that it will require updates.
1 Like
Patch that makes the -<arch> optional in some tests: ⚙ D143666 [Clang] Fix clang_rt tests when LLVM_ENABLE_PER_TARGET_RUNTIME_DIR is ON