This is an archive of the discontinued LLVM Phabricator instance.

[clang][driver] Add gcc-toolset/devtoolset 12 to prefixes
ClosedPublic

Authored by tbaeder on May 18 2022, 1:50 AM.

Diff Detail

Event Timeline

tbaeder created this revision.May 18 2022, 1:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2022, 1:50 AM
tbaeder requested review of this revision.May 18 2022, 1:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2022, 1:50 AM

It may be time to add unittests to clang/unittests/Driver/ToolChainTest.cpp for /opt/rh detection. You may use TEST(ToolChainTest, VFSGCCInstallation) as an example creating a pseudo file hierarchy in memory.

In addition, if every new GCC release requires addition to /opt/rh/gcc-toolset-$major (if I understand correctly), we probably want to switch to llvm::vfs::directory_iterator iteration and using the largest version.

tbaeder updated this revision to Diff 430572.May 18 2022, 11:01 PM
MaskRay added inline comments.May 18 2022, 11:15 PM
clang/lib/Driver/ToolChains/Gnu.cpp
2144

delete blank line

2150

The two if can be combined

clang/unittests/Driver/ToolChainTest.cpp
624

Add a comment that these directories are ignored.

651

512 is arbitrary. It's good to test something close to the real.

tbaeder updated this revision to Diff 430597.May 19 2022, 12:49 AM
tbaeder marked 4 inline comments as done.
MaskRay accepted this revision.May 19 2022, 6:49 PM

Thanks!

clang/unittests/Driver/ToolChainTest.cpp
619

End full sentences with a period .

This revision is now accepted and ready to land.May 19 2022, 6:49 PM
This revision was landed with ongoing or failed builds.May 23 2022, 2:35 AM
This revision was automatically updated to reflect the committed changes.
MaskRay added a comment.EditedMay 23 2022, 3:35 PM

Harbormaster completed remote builds in B165047: Diff 430277.

You can get Windows bot results in the link.

tbaeder updated this revision to Diff 431609.May 24 2022, 1:16 AM

Harbormaster completed remote builds in B165047: Diff 430277.

You can get Windows bot results in the link.

Hm, the windows pre-merge checks don't fail. Just the buildbots. I wonder if the BuildCompilation() calls simply have the wrong parameters. The other calls in the file seem to always create a fake foo.cpp even if they only check the verbose info.

Also not great that printVerboseInfo() seems to print nothing in case of an error :/

tbaeder updated this revision to Diff 431928.May 25 2022, 3:20 AM

@MaskRay Ignored the new test on Windows hosts now like you suggested. Does this look good?

Ping. @MaskRay Can you take a quick look and check if this is what you had in mind?

Ping. @MaskRay Can you take a quick look and check if this is what you had in mind?

LG

clang/unittests/Driver/ToolChainTest.cpp
615

delete blank line

623

delete public.

Can you avoid using a derived DiagnosticConsumer?

698

This should be removed if we exclude Windows.

tbaeder marked 2 inline comments as done.Jun 2 2022, 12:35 AM
tbaeder added inline comments.
clang/unittests/Driver/ToolChainTest.cpp
623

I can use the SimpleDiagnosticConsumer class created earlier in the file.

tbaeder updated this revision to Diff 433673.Jun 2 2022, 12:39 AM

The original toolchain detection added root/usr to the paths which was certainly necessary on PowerPC. Since that suffix is removed in this patch, our Redhat buildbot is now broken (https://lab.llvm.org/buildbot/#/builders/57/builds/18577). @quinnp has posted an update to this at https://reviews.llvm.org/D127310.

ebevhan added a subscriber: ebevhan.Jun 9 2022, 2:15 AM

Hi! This test is failing in some of our downstream builds.

clang/unittests/Driver/ToolChainTest.cpp
647

There's a binary name missing here:

{"clang", "--gcc-toolchain="}

Driver strips the first argument, so the behavior of this test becomes dependent on GCC_INSTALL_PREFIX which can cause it to fail in some cases.

687

Here too.

uabelho added a subscriber: uabelho.Jun 9 2022, 6:07 AM
tbaeder marked 2 inline comments as done.Jun 9 2022, 6:15 AM
tbaeder added inline comments.
clang/unittests/Driver/ToolChainTest.cpp
647

Thanks, I pushed a commit adding the driver name.