This is an archive of the discontinued LLVM Phabricator instance.

Clang/Gnu: Scan GCC with triple without vendor if vendor is unknown
ClosedPublic

Authored by wzssyqa on Aug 17 2023, 7:11 AM.

Details

Summary

If ScanLibDirForGCCTriple with target triple fail, let's try the triple
with vendor stripped if vendor is unknown.

Debian always uses triples without vendor section. In general,
triples without vendor section is the most similar aliases than any other
aliases.

To archive this, we add a private member TripleNoVendor to GCCInstallationDetector.

This modification makes testcase riscv32-toolchain.c and
riscv64-toolchain.c fails. And the real reason is that they are wrong:
--triple riscv64-unknown-elf tries to use riscv64-unknown-linux-gnu
first.

This patch accidentally fixes this problem.

We also drop the path delimiter pattern {{/|\\\\}}, as these 2 tests
are disabled on Windows, and in fact the positions of this pattern are
not correct.

Diff Detail

Event Timeline

wzssyqa created this revision.Aug 17 2023, 7:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2023, 7:11 AM
wzssyqa requested review of this revision.Aug 17 2023, 7:11 AM

*Triples* arrays are used much more than it designed to.

For mips, if we have g++-multiarch-mipsel-linux-gnu installed, while g++-multiarch-mips64el-linux-gnuabi64 not.

clang -target mips64el-linux-gnuabi64 xx.c -o xx will try to use the multilib in mipsel-linux-gnu,
and in fact it works.

Is it a bug or feature?

*Triples* arrays are used much more than it designed to.

For mips, if we have g++-multiarch-mipsel-linux-gnu installed, while g++-multiarch-mips64el-linux-gnuabi64 not.

clang -target mips64el-linux-gnuabi64 xx.c -o xx will try to use the multilib in mipsel-linux-gnu,
and in fact it works.

Is it a bug or feature?

I consider it a bug. GCC installation detection utilizing multilib should only kick in in scenarios like clang --target=x86_64-unknown-linux-gnu -m32 using x86_64-linux-gnu/32.

Unfortunately, clang --target=i686-unknown-linux-gnu may use a multilib directory under lib/gcc/x86_64-linux-gnu. It's possible that some users are relying on this behavior.
Perhaps we should keep workarounds for x86-32/x86-64 but fix other architectures. At the very least, don't add behaviors for newer ports (FYI @xen0n @SixWeining).

MaskRay added a comment.EditedAug 17 2023, 10:42 AM

This change adds more code for Debian and its derivatives. I think this patch should justify itself by removing some complexity, e.g. removing some elements from *Triple* arrays.

Is it possible for Debian to install some *-unknown-* symlinks (e.g. /usr/lib/gcc/x86_64-unknown-linux-gnu -> x86_64-linux-gnu) and eventually removing the no-vendor directories? (The executable names can keep /usr/bin/x86_64-linux-gnu-gcc and there should be no user-facing differences)

To the best of my knowledge, GCC upstream and other distributions always have the vendor part in their target triples. I don't wish that we forever keep workarounds in Clang Driver for newer Debian.

This change adds more code for Debian and its derivatives. I think this patch should justify itself by removing some complexity, e.g. removing some elements from *Triple* arrays.

If we consider the clang --target=i686-unknown-linux-gnu may use lib/gcc/x86_64-linux-gnu is a bug, then with this patch, we can remove the Debian's non-vendor triples from *Triple* arrays..

Is it possible for Debian to install some *-unknown-* symlinks (e.g. /usr/lib/gcc/x86_64-unknown-linux-gnu -> x86_64-linux-gnu) and eventually removing the no-vendor directories? (The executable names can keep /usr/bin/x86_64-linux-gnu-gcc and there should be no user-facing differences)

I have no idea. As the the whole system, including the file name of gcc/binutils commands, is using the non-vendor triples.
Anyway, add a symlink won't be a very difficult problem. I can request it.

To the best of my knowledge, GCC upstream and other distributions always have the vendor part in their target triples. I don't wish that we forever keep workarounds in Clang Driver for newer Debian.

In fact GCC doesn't care about it. It use whatever you passed it with --target=.
For config.guess, I does return the triple with vendor.

This change adds more code for Debian and its derivatives. I think this patch should justify itself by removing some complexity, e.g. removing some elements from *Triple* arrays.

If we consider the clang --target=i686-unknown-linux-gnu may use lib/gcc/x86_64-linux-gnu is a bug, then with this patch, we can remove the Debian's non-vendor triples from *Triple* arrays..

Is it possible for Debian to install some *-unknown-* symlinks (e.g. /usr/lib/gcc/x86_64-unknown-linux-gnu -> x86_64-linux-gnu) and eventually removing the no-vendor directories? (The executable names can keep /usr/bin/x86_64-linux-gnu-gcc and there should be no user-facing differences)

I have no idea. As the the whole system, including the file name of gcc/binutils commands, is using the non-vendor triples.
Anyway, add a symlink won't be a very difficult problem. I can request it.

Thanks.

To the best of my knowledge, GCC upstream and other distributions always have the vendor part in their target triples. I don't wish that we forever keep workarounds in Clang Driver for newer Debian.

In fact GCC doesn't care about it. It use whatever you passed it with --target=.
For config.guess, I does return the triple with vendor.

GCC normalizes xxx in ./configure --target=xxx. The generated lib/gcc/*vendor* directory always has the vendor part.

I have no idea. As the the whole system, including the file name of gcc/binutils commands, is using the non-vendor triples.
Anyway, add a symlink won't be a very difficult problem. I can request it.

Thanks.

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1049993

GCC normalizes xxx in ./configure --target=xxx. The generated lib/gcc/*vendor* directory always has the vendor part.

In fact, no.
When building gcc, config.guess detects the triple, which has vendor section.

When make install, the path is fellow what we give to configure.

Yes, if you just use ../configure, the path will has vendor, due to the value is from config.guess.
While, if we ../configure --target=x86_64-linux-gnu, the path will use x86_64-linux-gnu, instead of the value from config.guess.

Errr, the risc-v test case is wrong...

They use --target=riscv64-unknown-elf, while expect riscv64-unknown-linux-gnu sysroot will be used...

@anton-afanasyev in clang/test/Driver/riscv64-toolchain.c,
the separator of path, some use just /, and some others use {{/|\\\\}}.
Do you know about why?

If we need to consider Windows, I guess that, all of the path separator should be {{/|\\\\}}.

wzssyqa updated this revision to Diff 551456.Aug 18 2023, 3:52 AM
wzssyqa edited the summary of this revision. (Show Details)Aug 18 2023, 3:52 AM
MaskRay accepted this revision.Aug 18 2023, 12:40 PM

Since riscv64-toolchain.c has // UNSUPPORTED: system-windows, we are fine to remove {{/|\\\\}}. This was how I did with linux-cross.cpp to avoid dealing with Windows backslashes. We generally want to make tests as portable as possible, but when it's related to {{/|\\\\}} noise, I think it's fine to disable Windows. I think awhile ago I asked whether we can always use / and the answer is complex or no, but I cannot find the discussion now.

While, if we ../configure --target=x86_64-linux-gnu, the path will use x86_64-linux-gnu, instead of the value from config.guess.

Confirmed. Since this is generic GCC behavior, I think it makes sense to support the special case.

This revision is now accepted and ready to land.Aug 18 2023, 12:40 PM
This revision was landed with ongoing or failed builds.Aug 21 2023, 7:14 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2023, 7:14 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript