[Driver] Verify GCCInstallation is valid

[nickdesaulniers]

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 <[e-mail]>"

[srhines]

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]

clang/lib/Driver/ToolChains/Linux.cpp

259–260

Do we need to add the same check here too?

[danielmentz]

clang/lib/Driver/ToolChains/Linux.cpp

259–260

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.

[nickdesaulniers]

@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]

• add unit test

[srhines]

Thanks for picking this up and finishing it.

clang/lib/Driver/ToolChains/Linux.cpp

Linux::Linux(const Driver &D, const llvm::Triple &Triple, const ArgList &Args)
   // least) put various tools in a triple-prefixed directory off of the parent
   // of the GCC installation. We use the GCC triple here to ensure that we end
   // up with tools that support the same amount of cross compiling as the
   // detected GCC installation. For example, if we find a GCC installation
   // targeting x86_64, but it is a bi-arch GCC installation, it can also be
   // used to target i386.
   // FIXME: This seems unlikely to be Linux-specific.
   ToolChain::path_list &PPaths = getProgramPaths();
+  if (GCCInstallation.isValid()) {
     PPaths.push_back(Twine(GCCInstallation.getParentLibPath() + "/../" +
                            GCCInstallation.getTriple().str() + "/bin")
                          .str());
+  }
 
   Distro Distro(D.getVFS());
 
   if (Distro.IsAlpineLinux() || Triple.isAndroid()) {
     ExtraOpts.push_back("-z");
     ExtraOpts.push_back("now");
   }
 
   if (Distro.IsOpenSUSE() || Distro.IsUbuntu() || Distro.IsAlpineLinux() ||
       Triple.isAndroid()) {
     ExtraOpts.push_back("-z");
     ExtraOpts.push_back("relro");
   }
 
   // The lld default page size is too large for Aarch64, which produces much
   // larger .so files and images for arm64 device targets. Use 4KB page size
   // for Android arm64 targets instead.
   if (Triple.isAArch64() && Triple.isAndroid()) {
     ExtraOpts.push_back("-z");
     ExtraOpts.push_back("max-page-size=4096");
   }
 
   if (GCCInstallation.getParentLibPath().find("opt/rh/devtoolset") !=
       StringRef::npos)
     // With devtoolset on RHEL, we want to add a bin directory that is relative
     // to the detected gcc install, because if we are using devtoolset gcc then
     // we want to use other tools from devtoolset (e.g. ld) instead of the
     // standard system tools.
     PPaths.push_back(Twine(GCCInstallation.getParentLibPath() +
                      "/../bin").str());
 
   if (Arch == llvm::Triple::arm || Arch == llvm::Triple::thumb)