This is an archive of the discontinued LLVM Phabricator instance.

Always search sysroot for GCC installs
ClosedPublic

Authored by greened on Jul 12 2018, 8:36 AM.

Details

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.

Diff Detail

Repository
rC Clang

Event Timeline

greened created this revision.Jul 12 2018, 8:36 AM
greened edited the summary of this revision. (Show Details)Jul 17 2018, 12:15 PM

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

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.)

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.

orivej added a subscriber: orivej.Sep 10 2018, 3:33 AM
greened updated this revision to Diff 168472.Oct 5 2018, 7:45 AM

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.

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

rsmith accepted this revision.Oct 12 2018, 10:02 AM

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. :(

This revision is now accepted and ready to land.Oct 12 2018, 10:02 AM
greened closed this revision.Oct 22 2018, 6:49 AM

Fixed in r344901.