Page MenuHomePhabricator

[Gnu toolchain] Look at standard GCC paths for libstdcxx by default
ClosedPublic

Authored by sthibaul on Nov 2 2019, 4:52 PM.

Details

Summary

Linux' current addLibCxxIncludePaths and addLibStdCxxIncludePaths are
actually almost non-Linux-specific at all, and can be reused almost as such
for all gcc toolchains. Only keep Android/Freescale/Cray hacks in Linux'
version.

Diff Detail

Event Timeline

sthibaul created this revision.Nov 2 2019, 4:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2019, 4:52 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Added libcxx maintainers, would like one of them to sign off on this. I presume this is NFCI for Linux so it should be covered by existing regression tests?

clang/lib/Driver/ToolChains/Gnu.cpp
2693

No need for = "" here.

ldionne resigned from this revision.Nov 13 2019, 8:58 AM
sthibaul marked an inline comment as done.Nov 13 2019, 9:06 AM

Not sure what NFCI means, but for Linux the behavior should just not change at all, and the testsuite indeed behaved exactly the same on Linux indeed.

clang/lib/Driver/ToolChains/Gnu.cpp
2693

(it's the existing code just moved to here actually)

I'm afraid that a not-that-smart compiler would complain that it could be used uninitialized below, I have seen such cases produce warnings before because the compiler was not making the connection with MaxVersion being 0. Are we confident that this will not pose problem?

kristina added inline comments.Nov 13 2019, 12:14 PM
clang/lib/Driver/ToolChains/Gnu.cpp
2693

std::string has a default constructor, but the int needs to be initialized. I was only referring to the string.

sthibaul updated this revision to Diff 229192.Nov 13 2019, 2:43 PM
sthibaul marked 3 inline comments as done.
sthibaul added inline comments.
clang/lib/Driver/ToolChains/Gnu.cpp
2693

std::string has a default constructor

Ah, ok. Now fixed.

@kristina can we land this patch ? Thanks

sthibaul updated this revision to Diff 235644.Dec 30 2019, 2:32 PM
sthibaul marked an inline comment as done.

I have updated the patch to the latest git and re-ran checks. Could somebody have a look at it please?

sylvestre.ledru requested changes to this revision.Dec 30 2019, 2:50 PM

[Gnu toolchain]
maybe this can be removed in the title?

clang/lib/Driver/ToolChains/Linux.cpp
36

are using a multiarch

This revision now requires changes to proceed.Dec 30 2019, 2:50 PM
sthibaul marked an inline comment as done.Dec 30 2019, 2:57 PM

[Gnu toolchain]
maybe this can be removed in the title?

I thought that commits usually have such kind of title prefixes?

clang/lib/Driver/ToolChains/Linux.cpp
36

Well, I can change that, but it's unrelated to the other change of the patch?

kristina requested changes to this revision.Jan 5 2020, 1:10 AM

Sorry for responding late, was away. This seems to be tripping up a regression test:

FAIL: Clang :: Driver/cross-linux.c (5312 of 16608)
llvm-project/clang/test/Driver/cross-linux.c:62:26: error: CHECK-MULTI32-X86-64: expected string not found in input
// CHECK-MULTI32-X86-64: "crti.o" "[[gcc_install:.*/Inputs/multilib_32bit_linux_tree/usr/lib/gcc/i386-unknown-linux/4.6.0]]/64{{/|\\\\}}crtbegin.o"

Do you mind looking into it?

Which checksuite are you running? I had been using ninja check-all, and I do indeed get unexpected failures, but exactly the same set of failures as when building against master.

I ran check-clang on a x86_64 Ubuntu 18.04 machine (toolchain compiled with compiler-rt as the default CRT and libc++/libc++abi/libunwind_llvm). I think one of the driver tests for multilib stuff may be broken, in which case it should probably be investigated/fixed before this lands. And yes you are right, this test seems to fail with this configuration even without the patch applied. Is this the same test you're referring to (cross-linux.c)?

I am actually getting many failures with ninja check-all on my build box:

Expected Passes    : 56258
Expected Failures  : 203
Unsupported Tests  : 1485
Unexpected Failures: 164

I have attached the whole check log:

(I don't see anything looking like what you have, so can't investigate)

kristina accepted this revision.Jan 5 2020, 1:32 PM

On second look this seems to be caused by compiler-rt being used as the default runtime, which is not accounted for in that particular test.

Test is expecting a GCCesque crtbegin here which is missing with compiler-rt:

"crt1.o" "crti.o" "/o/b/llvm-test/v10.0.4010/bin/../lib/clang/10.0.4010/lib/linux/clang_rt.crtbegin-x86_64.o"

Which fails for obvious reasons:

llvm-project/clang/test/Driver/cross-linux.c:62:26: error: CHECK-MULTI32-X86-64: expected string not found in input
// CHECK-MULTI32-X86-64: "crti.o" "[[gcc_install:.*/Inputs/multilib_32bit_linux_tree/usr/lib/gcc/i386-unknown-linux/4.6.0]]/64{{/|\\\\}}crtbegin.o"

This seems unrelated, apologies for the false alarm, I'll have to look into fixing the test for when compiler-rt is the default CRT myself.

I'm happy to sign off on this. Let me know if you need this committed.

Let me know if you need this committed.

Yes, please commit.

This revision was not accepted when it landed; it landed in state Needs Revision.Jan 5 2020, 1:50 PM
This revision was automatically updated to reflect the committed changes.

For future - Since you seem to put up a lot of (well, almost all) patches related to Hurd support, may I suggest requesting commit access so you can land patches yourself after review?

MaskRay added a subscriber: MaskRay.Jan 5 2020, 3:08 PM

Patch by sthibaul (Samuel Thibault)

@kristina As a belated advice... since LLVM has moved from svn to git, we can ask contributors's names and emails, and then do git commit --amend --author 'name <email@address>'