This is an archive of the discontinued LLVM Phabricator instance.

hurd: Detect libstdc++ include paths on Debian Hurd i386
ClosedPublic

Authored by sthibaul on Apr 26 2021, 3:51 PM.

Details

Summary

This is a follow-up of e92d2b80c6c9 ("[Driver] Detect libstdc++ include
paths for native gcc (-m32 and -m64) on Debian i386") for the Debian Hurd
case, which has the same multiarch name reduction from i686 to i386.
i386-linux-gnu is actually Linux-only, so this moves the code of that commit
to Linux.cpp, and adds the same to Hurd.cpp

Diff Detail

Event Timeline

sthibaul requested review of this revision.Apr 26 2021, 3:51 PM
sthibaul created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2021, 3:51 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MaskRay added inline comments.Apr 26 2021, 4:12 PM
clang/test/Driver/hurd.cpp
2

I think we need -NEXT patterns to prevent unintentional new paths.

IMO linux-cross.cpp uses a good style and can serve as a reference.

6

The warning negative pattern doesn't really work.

sthibaul marked 2 inline comments as done.Apr 26 2021, 5:01 PM
sthibaul added inline comments.
clang/test/Driver/hurd.cpp
2

Ok, fixed so.

6

You mean because it was misplaced?
Indeed, fixed so.

MaskRay added a subscriber: thakis.Apr 26 2021, 5:27 PM
MaskRay added inline comments.
clang/lib/Driver/ToolChains/Gnu.cpp
3014–3017

I'd be glad if we could use GCCInstallation.isValid().

However, @thakis has a chromium usage where lib/gcc/$triple/crtbegin.o is not provided while compile-only libstdc++ search paths are expected.

For now, use GCCInstallation.isValid() ? GCCInstallation.getTriple().str() : ""

3018

No need for a blank line

clang/test/Driver/hurd.cpp
6

-### cannot detect unused warning options: clang -Wfoobar '-###' a.cc -c => no warning

It is probably not the test's task to check it.

10

Windows may use backslashes in some places.
It is really difficult to tell where need backslashes, so I just add UNSUPPORTED: system-windows to linux-cross.cpp

22

{{^}} is important, otherwise a new addition cannot be detected.

sthibaul updated this revision to Diff 340700.Apr 26 2021, 5:47 PM
sthibaul marked 3 inline comments as done.

Fix GCCInstallation.isValid use in Generic_GCC::addLibStdCxxIncludePaths
Fix test further

sthibaul marked 4 inline comments as done.Apr 26 2021, 5:47 PM
sthibaul added inline comments.
clang/lib/Driver/ToolChains/Gnu.cpp
3014–3017

Ok, fixed so

3018

Ok, fixed so

clang/test/Driver/hurd.cpp
6

Ok, dropped it.

10

Ok, added so.

22

Ok, added so.

MaskRay added inline comments.Apr 26 2021, 5:56 PM
clang/test/Driver/hurd.cpp
5

The target should exactly match the hurd triple. Other aliases are supported but should be considered discouraged.

Driver testing is difficult. Good to test -DCLANG_DEFAULT_RTLIB=compiler-rt -DCLANG_DEFAULT_CXX_STDLIB=libc++ as well whether you need to fix some options.

If hurd.c has now duplicated testing for some properties, consider dropping it. We should try making tests orthogonal.

21

You probably missed that I used "-L as the first line value (also note that the thing after CHECK: is aligned). The ensures the test can catch the case when a "-L" is added to the start of the list.

sthibaul updated this revision to Diff 340703.Apr 26 2021, 6:21 PM
sthibaul marked 6 inline comments as done.

Move all tests to hurd.cpp

clang/test/Driver/hurd.cpp
5

The target should exactly match the hurd triple. Other aliases are supported but should be considered discouraged.

Well, i386-pc-gnu is the GCC triplet for hurd on x86. i386-pc-hurd-gnu is llvm-only. Alright anyway.

If hurd.c has now duplicated testing for some properties, consider dropping it.

Ok I have moved everything from hurd.c to hurd.cpp.

21

I noticed that afterwards indeed, fixed so.

MaskRay accepted this revision.Apr 26 2021, 7:53 PM

LGTM.

clang/test/Driver/hurd.cpp
22

Since we now unsupport Windows, {{/|\\\\}} -> /

Consider replacing gcc 4.6.0 with the actual gcc version which may be higher, just to reflect the truth.
We may want to unsupport too old GCC versions if nobody uses.

But perhaps these cleanups can be done separately.

50

Drop {{(.exe)?}}

I think "{{.*}}ld{{(.exe)?}}" can be cleaned as well. Is it i686-gnu-ld if you add a fake executable bin/i686-gnu-ld?
If it is exactly ld, will be good to test the exact form. linux-ld.c is a big messy and probably not a good reference.

This revision is now accepted and ready to land.Apr 26 2021, 7:53 PM
sthibaul updated this revision to Diff 340765.Apr 27 2021, 2:30 AM

Fix clang-tidy warning

sthibaul updated this revision to Diff 340871.Apr 27 2021, 9:13 AM
sthibaul marked 2 inline comments as done.

Rather use i686-gnu triplet

sthibaul updated this revision to Diff 340878.Apr 27 2021, 9:26 AM

Rebase on newer D101317

MaskRay updated this revision to Diff 340960.Apr 27 2021, 1:03 PM

simplify

clang/lib/Driver/ToolChains/Gnu.cpp
3014–3017

Sorry, I noticed that we can just ignore !GCCInstallation.isValid(). The chromium usage should still work.

This revision was landed with ongoing or failed builds.Apr 27 2021, 1:04 PM
This revision was automatically updated to reflect the committed changes.
sthibaul marked an inline comment as done.Apr 27 2021, 1:05 PM
sthibaul added inline comments.
clang/lib/Driver/ToolChains/Gnu.cpp
3014–3017

Ok!

clang/lib/Driver/ToolChains/Linux.cpp