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

SmallVector<std::string, 8> Prefixes(D.PrefixDirs.begin(),
                                        D.PrefixDirs.end());
 
   StringRef GCCToolchainDir = getGCCToolchainDir(Args);
   if (GCCToolchainDir != "") {
     if (GCCToolchainDir.back() == '/')
       GCCToolchainDir = GCCToolchainDir.drop_back(); // remove the /
 
     Prefixes.push_back(GCCToolchainDir);
+
+    // If we have a SysRoot, try that too as GCCToolchainDir may not
+    // have support for the target.  This can happen, for example, if
+    // clang was configured with -DGCC_INSTALL_PREFIX and we run
+    // regression tests with --sysroot.
+    if (!D.SysRoot.empty()) {
+      Prefixes.push_back(D.SysRoot);
+      AddDefaultGCCPrefixes(TargetTriple, Prefixes, D.SysRoot);
+    }
   } else {
     // If we have a SysRoot, try that first.
     if (!D.SysRoot.empty()) {
       Prefixes.push_back(D.SysRoot);
       AddDefaultGCCPrefixes(TargetTriple, Prefixes, D.SysRoot);
     }
 
     // Then look for gcc installed alongside clang.