Always search sysroot for GCC installs

[greened]

Summary

Previously, if clang was configured with -DGCC_INSTALL_PREFIX, then it would not search a provided sysroot for a gcc install. This caused a number of regression tests to fail. Add sysroot to the list of paths to search, even if a gcc toolchain is provided with cmake or a command-line option. Search the sysroot after the provided toolchain.

[greened]

Ping.

[greened]

Ping.

[greened]

Ping.

[greened]

Ping...

[greened]

Ping?

[greened]

Ping?

[greened]

Ping. It's been well over a month now. What's the best way to get this reviewed?

[rsmith]

I think -gcc-toolchain, if specified, should simply be taken as the location of the GCC toolchain; we should never go looking for it anywhere else if -gcc-toolchain is specified. So I think the patch is not quite right as-is, as it also affects that case. I think these alternatives both seem reasonable:

• if -gcc-toolchain is not specified, and --sysroot is specified, then also look in the sysroot for GCC as normal after checking in GCC_INSTALL_PREFIX
• if --sysroot is specified, ignore GCC_INSTALL_PREFIX when computing the GCC toolchain directory

(The difference would be that in the former case we'd use a GCC that's not within the sysroot if GCC_INSTALL_PREFIX names a suitable toolchain, and in the latter case we would not.)

I think my preference is the second option: GCC_INSTALL_PREFIX should only be taken as specifying the installation prefix for the default sysroot specified at configure time. If --sysroot is explicitly specified, GCC_INSTALL_PREFIX should be ignored (but -gcc-toolchain should still be respected, and if specified we should not go looking for a toolchain in the sysroot ourselves.)

[greened]

I agree that option 2 seems best. It's consistent with options overriding more general configuration/environment variables and more specific options overriding more general options. If it turns out option 1 is needed in the future, it should be a simple change given an option 2 implementation.

[greened]

Updated to implement option 2. I'm not totally happy with passing a StringRef just to check if it's non-empty but opted to reduce the size of the diff rather than refactor a bunch of stuff.

[greened]

Ping. This is impacting our ability to get clang functioning.

[rsmith]

Hmm, I'd really like a test for this but I'm not sure it's really testable as-is because it depends on a non-default configuration value. :(

[greened]

Fixed in r344901.

lib/Driver/ToolChains/Gnu.cpp

if (size_t EndNumber = PatchText.find_first_not_of("0123456789")) {
         return BadVersion;
       GoodVersion.PatchSuffix = PatchText.substr(EndNumber);
     }
   }
 
   return GoodVersion;
 }
 
-static llvm::StringRef getGCCToolchainDir(const ArgList &Args) {
+static llvm::StringRef getGCCToolchainDir(const ArgList &Args,
+                                          llvm::StringRef SysRoot) {
   const Arg *A = Args.getLastArg(clang::driver::options::OPT_gcc_toolchain);
   if (A)
     return A->getValue();
+
+  // If we have a SysRoot, ignore GCC_INSTALL_PREFIX.
+  // GCC_INSTALL_PREFIX specifies the gcc installation for the default
+  // sysroot and is likely not valid with a different sysroot.
+  if (!SysRoot.empty())
+    return "";
+
   return GCC_INSTALL_PREFIX;
 }
 
 /// Initialize a GCCInstallationDetector from the driver.
 ///
 /// This performs all of the autodetection and sets up the various paths.
 /// Once constructed, a GCCInstallationDetector is essentially immutable.
 ///
void Generic_GCC::GCCInstallationDetector::init(
   CollectLibDirsAndTriples(TargetTriple, BiarchVariantTriple, CandidateLibDirs,
                            CandidateTripleAliases, CandidateBiarchLibDirs,
                            CandidateBiarchTripleAliases);
 
   // Compute the set of prefixes for our search.
   SmallVector<std::string, 8> Prefixes(D.PrefixDirs.begin(),
                                        D.PrefixDirs.end());
 
-  StringRef GCCToolchainDir = getGCCToolchainDir(Args);
+  StringRef GCCToolchainDir = getGCCToolchainDir(Args, D.SysRoot);
   if (GCCToolchainDir != "") {
     if (GCCToolchainDir.back() == '/')
       GCCToolchainDir = GCCToolchainDir.drop_back(); // remove the /
 
     Prefixes.push_back(GCCToolchainDir);
   } else {
     // If we have a SysRoot, try that first.
     if (!D.SysRoot.empty()) {