Skip to content

[Driver] Have getTargetSubDirPath better match get_compiler_rt_target #100091

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

[Driver] Have getTargetSubDirPath better match get_compiler_rt_target #100091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions clang/lib/Driver/ToolChain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not, some BSDs prefer the amd64 form, too. Besides, get_compiler_rt_target does it unconditionally, so clang should match.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_64 and amd64 just as you see fit, and get identical results. It doesn't matter here if the triple was determined automatically by config.guess at LLVM build time (as is the case for sparc64-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/projects layout 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/runtimes layout, 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 that clang is confused in not being able to find it's own runtime libs. That's a bug in cllang, 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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".
Expand All @@ -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 {};
}

Expand Down
Empty file added clang/test/Driver/Inputs/debian_sparc64_tree/lib/sparc64-linux-gnu/Scrt1.o
View file
Open in desktop
Empty file.
Empty file added clang/test/Driver/Inputs/debian_sparc64_tree/lib/sparc64-linux-gnu/crti.o
View file
Open in desktop
Empty file.
Empty file added clang/test/Driver/Inputs/debian_sparc64_tree/lib/sparc64-linux-gnu/crtn.o
View file
Open in desktop
Empty file.
Empty file added clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/gcc/sparc64-linux-gnu/12/crtbeginS.o
View file
Open in desktop
Empty file.
Empty file added clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/gcc/sparc64-linux-gnu/12/crtendS.o
View file
Open in desktop
Empty file.
Empty file added clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/gcc/sparc64-linux-gnu/12/libgcc.a
View file
Open in desktop
Empty file.
Empty file added clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/gcc/sparc64-linux-gnu/12/libgcc_s.so
View file
Open in desktop
Empty file.
Empty file added clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/sparc64-linux-gnu/libc.so
View file
Open in desktop
Empty file.
Empty file added clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/sparc64-linux-gnu/libdl.so
View file
Open in desktop
Empty file.
Empty file added clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/sparc64-linux-gnu/libm.so
View file
Open in desktop
Empty file.
Empty file added clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/sparc64-linux-gnu/libpthread.so
View file
Open in desktop
Empty file.
Empty file added clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/sparc64-linux-gnu/libresolv.so
View file
Open in desktop
Empty file.
Empty file added clang/test/Driver/Inputs/debian_sparc64_tree/usr/lib/sparc64-linux-gnu/librt.so
View file
Open in desktop
Empty file.
Empty file added ...e_dir_with_per_target_subdir/lib/sparcv9-unknown-linux-gnu/libclang_rt.ubsan_standalone.a
View file
Open in desktop
Empty file.
Empty file added ...ource_dir_with_per_target_subdir/lib/x86_64-pc-solaris2.11/libclang_rt.ubsan_standalone.a
View file
Open in desktop
Empty file.
20 changes: 20 additions & 0 deletions clang/test/Driver/noncanon-per-target-runtime-dir.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"

35 changes: 35 additions & 0 deletions clang/test/Driver/runtime-layout.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Loading