Page MenuHomePhabricator

[Driver] Verify GCCInstallation is valid
ClosedPublic

Authored by nickdesaulniers on Feb 7 2019, 4:40 PM.

Details

Summary

Values returned by GCCInstallation.getParentLibPath() and
GCCInstallation.getTriple() are not valid unless
GCCInstallation.isValid() returns true. This has previously been
ignored, and the former two values were used without checking whether
GCCInstallation is valid. This led to the bad path "/../bin" being added
to the list of program paths.

author: danielmentz "Daniel Mentz <danielmentz@google.com>"

Diff Detail

Repository
rC Clang

Event Timeline

danielmentz created this revision.Feb 7 2019, 4:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2019, 4:40 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
srhines added a subscriber: srhines.Feb 7 2019, 5:08 PM

Would it be reasonable to have a test for this with perhaps an invalid GCC installation? There is some mock GCC/sysroot testing in https://github.com/llvm/llvm-project/blob/master/clang/test/Driver/android-gcc-toolchain.c and https://github.com/llvm/llvm-project/blob/master/clang/test/Driver/android-ndk-standalone.cpp. I am not sure that it will be easy to trip this same bug that way, but I think it is possible.

tstellar added inline comments.Feb 8 2019, 6:52 PM
clang/lib/Driver/ToolChains/Linux.cpp
259–260 ↗(On Diff #185888)

Do we need to add the same check here too?

danielmentz marked an inline comment as done.Feb 11 2019, 4:28 PM
danielmentz added inline comments.
clang/lib/Driver/ToolChains/Linux.cpp
259–260 ↗(On Diff #185888)

I'd say no, we don't need it here. If GCCInstallation is not valid, then GCCInstallation.getParentLibPath() will probably return the empty string, and .find("opt/rh/devtoolset") on that empty string will probably return StringRef::npos, in which case the if body is not run.
I understand the concern, but I think, in this case, we got lucky.

@danielmentz thanks for this patch! Some teammates are running into issues that would be fixed by this patch.

I think @srhines is asking for a test similar to what I wrote in r344941 (https://github.com/llvm/llvm-project/commit/11dadac247e677de124328077d080fe086f14d47#diff-281bd5e7ab7578a5203aff1d1d001bdd), and shouldn't be too much work (basically test that --gcc-toolchain=<garbage> fails in an explicit way.

Would you mind adding such a test, so we can ship this useful fix?

nickdesaulniers commandeered this revision.May 21 2019, 1:47 PM
nickdesaulniers added a reviewer: danielmentz.
  • add unit test
nickdesaulniers edited the summary of this revision. (Show Details)May 21 2019, 1:50 PM
nickdesaulniers added a subscriber: danielmentz.
srhines accepted this revision.May 21 2019, 2:03 PM

Thanks for picking this up and finishing it.

This revision is now accepted and ready to land.May 21 2019, 2:03 PM
This revision was automatically updated to reflect the committed changes.
ormris removed a subscriber: ormris.May 21 2019, 3:08 PM