I would prefer a different solution.
Overall, I dislike cases where a Clang binary has this kind of decision hardcoded into it. Yes, we do have a precedent for such things with CMake options like CLANG_DEFAULT_LINKER=lld (and yes, I do use such configurations myself occasionally), but they generally hinder use of Clang, when it e.g. can be used for other targets than the default one it was originally configured for.
Runtime configurability (which can be controlled with a default config file - which can be overridden with a --no-default-config if using the clang binary for a different target) generally is preferrable to me. But for this case, I’m not sure if it is warranted to add yet another command line option to control this detail.
As for the suggested patch; I would prefer to not bake the choice of LLVM_ENABLE_PER_TARGET_RUNTIME_DIR into something visible in llvm-config.h. (And if we really do go that way, I think config.h which is private to the LLVM build, would be more applicable? But perhaps that doesn’t work correctly if LLVM and Clang are built in two separate build steps?)
To me, runtimes is something that not necessarily is built at the same time as Clang (I never do), so the fact that runtimes can be built at the same time is mostly a convenience.
Overall, I would prefer if we don’t stop looking for the old arch-suffixed form of libraries, even if the per-target runtime directory form might be the preferred one.
If the main issue is the error message is hard to reason about (which I certainly would agree with), I think we could achieve it by probing for the old filename, and using the per-target runtime directory as fallback (which gets printed in the error message).
We could achieve that with a change like this:
diff --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index 388030592b48..fe47d2d85092 100644
--- a/clang/lib/Driver/ToolChain.cpp
+++ b/clang/lib/Driver/ToolChain.cpp
@@ -655,20 +655,28 @@ std::string ToolChain::getCompilerRT(const ArgList &Args, StringRef Component,
// Check for runtime files in the new layout without the architecture first.
std::string CRTBasename =
buildCompilerRTBasename(Args, Component, Type, /*AddArch=*/false);
+ SmallString<128> Path;
for (const auto &LibPath : getLibraryPaths()) {
- SmallString<128> P(LibPath);
- llvm::sys::path::append(P, CRTBasename);
- if (getVFS().exists(P))
- return std::string(P);
+ Path = LibPath;
+ llvm::sys::path::append(Path, CRTBasename);
+ if (getVFS().exists(Path))
+ return std::string(Path);
}
- // Fall back to the old expected compiler-rt name if the new one does not
- // exist.
+ std::string FallbackPath = std::string(Path);
+
+ // Check the old expected compiler-rt name if the new one does not exist.
CRTBasename =
buildCompilerRTBasename(Args, Component, Type, /*AddArch=*/true);
- SmallString<128> Path(getCompilerRTPath());
+ Path = getCompilerRTPath();
llvm::sys::path::append(Path, CRTBasename);
- return std::string(Path);
+ if (getVFS().exists(Path))
+ return std::string(Path);
+
+ // If none is found, use a file name from the new layout, which may get
+ // printed in an error message, aiding users in knowing what Clang is
+ // looking for.
+ return FallbackPath;
}
(Untested, only compile tested.)
Overall, this issue feels more generally like something that I do feel appears in a number of places when trying to set up a toolchain with headers/libraries - Clang looks for things in a bunch of different places, but it’s not obvious which ones were considered but skipped. (Some paths do get added even if they don’t exist, so they appear in e.g. clang -v, listed as discarded as nonexistent, but many paths are just probed and skipped.) For that, I wonder if it would be good to have an option to Clang driver to request it to verbosely mention all the paths that it tests/evaluates. E.g. for headers, there’s lots of different combinations of sysroot, /usr/include, triples (some in the clang-normalized form, some in <arch>-linux-gnu form for debian multiarch) that do get tested.
) if we’d make the per-runtime target dir layout mandatory for compiler-rt. (But IIRC Apple targets have their own, different way of handling that anyway.) But I kinda don’t want to hard require 