[Driver] Have getTargetSubDirPath better match get_compiler_rt_target #100091
Conversation
As explained in [[Driver] Support non-canonical triples with new runtime lib layout](https://reviews.llvm.org/D133407), there are massive problems if `clang`'s and `compiler-rt`'s ideas of the target triple don't match. This can happen e.g. on Solaris/amd64 (`amd64-pc-solaris2.11`, `amd64` being the customary name of 64-bit x86 on that target) or Linux/sparc64 (`sparc64-unknown-linux-gnu`, where `config.sub` returns the `sparc64` form by default). While some adjustments have been made in `compiler-rt` ([[compiler-rt] Handle non-canonical triples with new runtime lib layout](https://reviews.llvm.org/D133406)), the `clang` side is still unfixed and the previous patch doesn't work any longer. As the stop-gap measure, this patch takes two of the adjustments made in `CompilerRTUtils.cmake` (`get_compiler_rt_target`) and applies them to `ToolChain.cpp` (`getTargetSubDirPath`) which currently takes the target triple as is, creating a mismatch between `clang`s idea of the layout of `lib/clang/<vers>/<triple>` and where `compiler-rt` actually installs the runtime libs. The tests had to be trimmed massively because `clang -print-runtime-dir` lies since PR llvm#87866: when the runtime libs are installed in the projects/classic layout, `clang` still emits the nonexisting runtimes layout. Tested on `amd64-pc-solaris2.11` and `x86_64-pc-linux-gnu`.
rorth
added
clang
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Rainer Orth (rorth) ChangesAs explained in [Driver] Support non-canonical triples with new runtime lib layout, there are massive problems if While some adjustments have been made in As the stop-gap measure, this patch takes two of the adjustments made in The tests had to be trimmed massively because Tested on Full diff: https://github.com/llvm/llvm-project/pull/100091.diff 18 Files Affected:
diff --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index 20a555afb8092..10434b987ae1d 100644
--- a/clang/lib/Driver/ToolChain.cpp
+++ b/clang/lib/Driver/ToolChain.cpp
@@ -766,9 +766,19 @@ ToolChain::getTargetSubDirPath(StringRef BaseDir) const {
return {};
};
- if (auto Path = getPathForTriple(getTriple()))
+ llvm::Triple Triple = getTriple();
+
+ // Try triple as is.
+ if (auto Path = getPathForTriple(Triple))
return *Path;
+ // Match transformations in CompilerRTUtils.cmake:get_compiler_rt_target.
+ if (Triple.getArchName() == "amd64")
+ Triple.setArch(Triple::x86_64);
+
+ if (Triple.getArchName() == "sparc64")
+ Triple.setArch(Triple::sparcv9);
+
// When building with per target runtime directories, various ways of naming
// the Arm architecture may have been normalised to simply "arm".
// For example "armv8l" (Armv8 AArch32 little endian) is replaced with "arm".
@@ -784,16 +794,15 @@ ToolChain::getTargetSubDirPath(StringRef BaseDir) const {
//
// M profile Arm is bare metal and we know they will not be using the per
// target runtime directory layout.
- if (getTriple().getArch() == Triple::arm && !getTriple().isArmMClass()) {
- llvm::Triple ArmTriple = getTriple();
- ArmTriple.setArch(Triple::arm);
- if (auto Path = getPathForTriple(ArmTriple))
- return *Path;
- }
+ if (Triple.getArch() == Triple::arm && !Triple.isArmMClass())
+ Triple.setArch(Triple::arm);
- if (getTriple().isAndroid())
+ if (Triple.isAndroid())
return getFallbackAndroidTargetPath(BaseDir);
+ if (auto Path = getPathForTriple(Triple))
+ return *Path;
+
return {};
}
diff --git a/clang/test/Driver/Inputs/debian_sparc64_tree/lib/sparc64-linux-gnu/Scrt1.o b/clang/test/Driver/Inputs/debian_sparc64_tree/lib/sparc64-linux-gnu/Scrt1.o
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/clang/test/Driver/Inputs/debian_sparc64_tree/lib/sparc64-linux-gnu/crti.o b/clang/test/Driver/Inputs/debian_sparc64_tree/lib/sparc64-linux-gnu/crti.o
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/clang/test/Driver/Inputs/debian_sparc64_tree/lib/sparc64-linux-gnu/crtn.o b/clang/test/Driver/Inputs/debian_sparc64_tree/lib/sparc64-linux-gnu/crtn.o
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/gcc/sparc64-linux-gnu/12/crtbeginS.o b/clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/gcc/sparc64-linux-gnu/12/crtbeginS.o
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/gcc/sparc64-linux-gnu/12/crtendS.o b/clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/gcc/sparc64-linux-gnu/12/crtendS.o
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/gcc/sparc64-linux-gnu/12/libgcc.a b/clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/gcc/sparc64-linux-gnu/12/libgcc.a
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/gcc/sparc64-linux-gnu/12/libgcc_s.so b/clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/gcc/sparc64-linux-gnu/12/libgcc_s.so
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/sparc64-linux-gnu/libc.so b/clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/sparc64-linux-gnu/libc.so
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/sparc64-linux-gnu/libdl.so b/clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/sparc64-linux-gnu/libdl.so
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/sparc64-linux-gnu/libm.so b/clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/sparc64-linux-gnu/libm.so
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/sparc64-linux-gnu/libpthread.so b/clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/sparc64-linux-gnu/libpthread.so
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/sparc64-linux-gnu/libresolv.so b/clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/sparc64-linux-gnu/libresolv.so
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/sparc64-linux-gnu/librt.so b/clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/sparc64-linux-gnu/librt.so
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/sparcv9-unknown-linux-gnu/libclang_rt.ubsan_standalone.a b/clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/sparcv9-unknown-linux-gnu/libclang_rt.ubsan_standalone.a
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/x86_64-pc-solaris2.11/libclang_rt.ubsan_standalone.a b/clang/test/Driver/Inputs/resource_dir_with_per_target_subdir/lib/x86_64-pc-solaris2.11/libclang_rt.ubsan_standalone.a
new file mode 100644
index 0000000000000..e69de29bb2d1d
diff --git a/clang/test/Driver/noncanon-per-target-runtime-dir.c b/clang/test/Driver/noncanon-per-target-runtime-dir.c
new file mode 100644
index 0000000000000..79ebd6bfbb11b
--- /dev/null
+++ b/clang/test/Driver/noncanon-per-target-runtime-dir.c
@@ -0,0 +1,20 @@
+/// Check that clang's and compiler-rt's ideas of per-target runtime dirs match.
+
+// RUN: %clang -### %s 2>&1 \
+// RUN: --target=amd64-pc-solaris2.11 -fsanitize=undefined \
+// RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
+// RUN: --sysroot=%S/Inputs/solaris_x86_tree \
+// RUN: | FileCheck --check-prefix=CHECK-SOLARIS-AMD64 %s
+
+// CHECK-SOLARIS-AMD64: x86_64-pc-solaris2.11/libclang_rt.ubsan_standalone.a
+// CHECK-SOLARIS-AMD64-NOT: lib/sunos/libclang_rt.ubsan_standalone-x86_64.a"
+
+// RUN: %clang -### %s 2>&1 \
+// RUN: --target=sparc64-unknown-linux-gnu -fsanitize=undefined \
+// RUN: -resource-dir=%S/Inputs/resource_dir_with_per_target_subdir \
+// RUN: --sysroot=%S/Inputs/debian_sparc64_tree \
+// RUN: | FileCheck --check-prefix=CHECK-DEBIAN-SPARC64 %s
+
+// CHECK-DEBIAN-SPARC64: sparcv9-unknown-linux-gnu/libclang_rt.ubsan_standalone.a
+// CHECK-DEBIAN-SPARC64-NOT: lib/linux/libclang_rt.ubsan_standalone-sparcv9.a"
+
diff --git a/clang/test/Driver/runtime-layout.c b/clang/test/Driver/runtime-layout.c
new file mode 100644
index 0000000000000..55021d45d35e4
--- /dev/null
+++ b/clang/test/Driver/runtime-layout.c
@@ -0,0 +1,35 @@
+/// Check that clang's idea of runtime dir layout matches e.g. compiler-rt's.
+
+/// Classical runtime layout.
+///
+/// Cannot be tested: clang -print-runtime-dir always prints the new path even
+/// if only the old directories exist.
+
+/// New runtime layout.
+
+// RUN: mkdir -p %t-runtime/lib/x86_64-pc-solaris2.11
+
+/// Canonical triple, 64-bit.
+// RUN: %clang -print-runtime-dir --target=x86_64-pc-solaris2.11 \
+// RUN: -resource-dir=%t-runtime \
+// RUN: | FileCheck --check-prefix=RUNTIME-DIR-X86_64 %s
+
+/// Non-canonical triple, 64-bit.
+// RUN: %clang -print-runtime-dir --target=amd64-pc-solaris2.11 \
+// RUN: -resource-dir=%t-runtime \
+// RUN: | FileCheck --check-prefix=RUNTIME-DIR-X86_64 %s
+
+// RUNTIME-DIR-X86_64: {{.*}}/lib/x86_64-pc-solaris2.11
+
+// RUN: mkdir -p %t-runtime/lib/i386-pc-solaris2.11
+
+/// Canonical triple, 32-bit.
+// RUN: %clang -print-runtime-dir --target=i386-pc-solaris2.11 \
+// RUN: -resource-dir=%t-runtime \
+// RUN: | FileCheck --check-prefix=RUNTIME-DIR-I386 %s
+
+/// Non-canonical triple, 32-bit.
+/// clang doesn't normalize --target=i686-pc-solaris2.11 to i386-pc-solaris2.11
+/// subdir.
+
+// RUNTIME-DIR-I386: {{.*}}/lib/i386-pc-solaris2.11
|
|
Ping? It's been a week and it would be nice to get this into the 19.x branch. |
| return *Path; | ||
|
|
||
| // Match transformations in CompilerRTUtils.cmake:get_compiler_rt_target. |
There was a problem hiding this comment.
https://reviews.llvm.org/D133406
Is this transformation only needed by Solaris? It seems that other OSes are happy with always using x86_64 and not bothering with amd64.
Can you make this Solaris specific?
There was a problem hiding this comment.
It's not, some BSDs prefer the amd64 form, too. Besides, get_compiler_rt_target does it unconditionally, so clang should match.
There was a problem hiding this comment.
I suspect that some BSDs used amd64 but now switched to x86_64, e.g.
x86_64-unknown-openbsd
It's fine that Solaris is different, but I think other OSes don't think this change.
Perhaps get_compiler_rt_target should be changed to affect only Solaris as well, but that change should not be merged to release/19.x to prevent potential disruption.
There was a problem hiding this comment.
How would this change cause harm? If a target uses e.g. an x86_64-* or sparcv9-* triple, it matches compiler-rt out of the box and this change doesn't make a difference. If on the other hand a target uses the equivalent amd64-* or sparc64-* forms, they need to be transformed to the form that compiler-rt uses unconditionally, otherwise clang won't find the runtime libs. This is what this patch does: bring clang and compiler-rt a little bit more in sync, nothing more, nothing less. Where do you see any disruption in that? Fixing tons of link failures in the testsuite isn't my definition of disruption, actually.
There was a problem hiding this comment.
I don't see this as disruption, but I see amd64-* sparcv9 as workarounds. For workarounds, we generally keep their scope as narrow as possible. We don't encourage current or future use cases.
I am concerned if supporting amd64-* opens the door for incorrect amd64-linux usage.
There was a problem hiding this comment.
And this is where I think you're fundamentally wrong.
Let me explain, first from a user perspective.
- As long as runtime libs aren't involved, you can use any equivalent form of the triple, both with
x86_64andamd64just as you see fit, and get identical results. It doesn't matter here if the triple was determined automatically byconfig.guessat LLVM build time (as is the case forsparc64-unknown-linux-gnu), specified by the user with-DLLVM_HOST_TRIPLE, or given at compile time with-target. - Even when runtime libs come into play, things still work with both/all forms when the old/classic/
projectslayout is used. - Only when users are unlucky enough (either because it's the build time default on certain OSes or they selected it explicitly) to use the new/
runtimeslayout, the link fails because the runtime libs aren't found with such alternative triples. Note that the user has never been given any indication (warning or error) that there's a problem with that triple, just a link error. This strongly suggests thatclangis confused in not being able to find it's own runtime libs. That's a bug incllang, clear as can be.
Those alternative forms have worked for years if not decades without any issues, and now they stop only in very specific circumstances. As far as I know, there's never been any indication that/why some forms of triples are better than others.
For comparison's sake, see what GCC (or the GNU toolchain in general) do instead. While they don't take triples at runtime, the configure time handling gives a good indication how this can be done without all those problems:
While users can easily use alternative forms of triples, GCC uses the AC_CANONICAL_TARGET/ACX_CANONICAL_TARGET autoconf macros and only uses the resulting canonicalized triples internally, e.g. when matching in configure scripts. Only in user-facing cases (like gcc -v output) are the pretty (user-specified) forms used at all.
I think this is what LLVM should do, too: canonicalize triples at some point and stick to those canonical forms. The current duplication of (inconsistent) canonicalizions in getTargetSubDirPath and get_compiler_rt_target needs to go. IMO there should be something like clang -print-canonical-target or some such, emitting clang's idea of the canonical form, and compiler-rt should just use that rather than second-guessing clang (or vice versa).
This patch is nothing more than a baby step in this direction, removing just one inconsistency we have today.
There was a problem hiding this comment.
I don't agree with the third bullet point.
As is, people are fine with not using the amd64 triples on all non-Solaris OSes.
We strive not to add workarounds that could lure users to do the wrong thing.
Note: We perform basic triple canonicalization (Triple::normalize), as you can see from "unknown":
% clang --target=x86_64-linux-gnu -dumpmachine
x86_64-unknown-linux-gnu
That said, we try to keep the code simple. We cannot really do every triple normalization GCC performs.
That's a lot of complexity, workarounds for old systems.
Instead, we should assess every extra rule. It seems that Solaris, a supported OS, still uses amd64, so we accept it (with Solaris specific condition so that we know the difference from the generic behavior; and we can drop the behavior once it becomes unneeded)
but it's not sufficient justification to extend this to all OSes.
I probably should only accepted https://reviews.llvm.org/D133406 when it was made Solaris specific.
There was a problem hiding this comment.
You still don't give any indication why one form of a triple is right and another one wrong, especially when it works in 95+% of the cases. Besides, there's no way a user can even determine what's right and and what's wrong other than spurious link failures.
We're getting absolutely nowhere with this: you keep making assertions without justification. This is a total waste of my time, the whole issue having dragged on for about two years in various forms, amounting to nothing.
|
Do we still need to fix this for 19.1.0-final? |
I think so, yes. E.g. when building on LLVM without specifying The situation on Solaris/amd64 is similar. |
|
I'm out of this: this isn't good for either my time, my temper or my sanity. |
As explained in [Driver] Support non-canonical triples with new runtime lib layout, there are massive problems if
clang's andcompiler-rt's ideas of the target triple don't match. This can happen e.g. on Solaris/amd64 (amd64-pc-solaris2.11,amd64being the customary name of 64-bit x86 on that target) or Linux/sparc64 (sparc64-unknown-linux-gnu, whereconfig.subreturns thesparc64form by default).While some adjustments have been made in
compiler-rt([compiler-rt] Handle non-canonical triples with new runtime liblayout), the
clangside is still unfixed and the previous patch doesn't work any longer.As the stop-gap measure, this patch takes two of the adjustments made in
CompilerRTUtils.cmake(get_compiler_rt_target) and applies them toToolChain.cpp(getTargetSubDirPath) which currently takes the target triple as is, creating a mismatch betweenclangs idea of the layout oflib/clang/<vers>/<triple>and wherecompiler-rtactually installs the runtime libs.The tests had to be trimmed massively because
clang -print-runtime-dirlies since PR #87866: when the runtime libs are installed in the projects/classic layout,clangstill emits the nonexisting runtimes layout.Tested on
amd64-pc-solaris2.11andx86_64-pc-linux-gnu.