[PATCH] D49244: Always search sysroot for GCC installs
greened closed this revision. greened added a comment. Fixed in r344901. Repository: rC Clang https://reviews.llvm.org/D49244 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49244: Always search sysroot for GCC installs
rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. 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. :( Repository: rC Clang https://reviews.llvm.org/D49244 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49244: Always search sysroot for GCC installs
greened added a comment. Ping. This is impacting our ability to get clang functioning. Repository: rC Clang https://reviews.llvm.org/D49244 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49244: Always search sysroot for GCC installs
greened updated this revision to Diff 168472.
greened added a comment.
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.
Repository:
rC Clang
https://reviews.llvm.org/D49244
Files:
lib/Driver/ToolChains/Gnu.cpp
Index: lib/Driver/ToolChains/Gnu.cpp
===
--- lib/Driver/ToolChains/Gnu.cpp
+++ lib/Driver/ToolChains/Gnu.cpp
@@ -1618,10 +1618,18 @@
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;
}
@@ -1653,7 +1661,7 @@
SmallVector 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 /
Index: lib/Driver/ToolChains/Gnu.cpp
===
--- lib/Driver/ToolChains/Gnu.cpp
+++ lib/Driver/ToolChains/Gnu.cpp
@@ -1618,10 +1618,18 @@
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;
}
@@ -1653,7 +1661,7 @@
SmallVector 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 /
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49244: Always search sysroot for GCC installs
greened added a comment. 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. Repository: rC Clang https://reviews.llvm.org/D49244 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49244: Always search sysroot for GCC installs
rsmith added a comment. 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.) Repository: rC Clang https://reviews.llvm.org/D49244 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49244: Always search sysroot for GCC installs
greened added a comment. Ping. It's been well over a month now. What's the best way to get this reviewed? Repository: rC Clang https://reviews.llvm.org/D49244 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49244: Always search sysroot for GCC installs
greened added a comment. Ping? Repository: rC Clang https://reviews.llvm.org/D49244 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49244: Always search sysroot for GCC installs
greened added a comment. Ping? Repository: rC Clang https://reviews.llvm.org/D49244 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49244: Always search sysroot for GCC installs
greened added a comment. Ping... Repository: rC Clang https://reviews.llvm.org/D49244 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49244: Always search sysroot for GCC installs
greened added a comment. Ping. Repository: rC Clang https://reviews.llvm.org/D49244 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49244: Always search sysroot for GCC installs
greened added a comment. Ping. Repository: rC Clang https://reviews.llvm.org/D49244 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49244: Always search sysroot for GCC installs
greened added a comment. Ping. Repository: rC Clang https://reviews.llvm.org/D49244 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D49244: Always search sysroot for GCC installs
greened created this revision.
greened added reviewers: rsmith, mcrosier, danalbert.
greened added a project: clang.
Herald added a subscriber: cfe-commits.
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. The sysroot
is searched after the provided install.
Repository:
rC Clang
https://reviews.llvm.org/D49244
Files:
lib/Driver/ToolChains/Gnu.cpp
Index: lib/Driver/ToolChains/Gnu.cpp
===
--- lib/Driver/ToolChains/Gnu.cpp
+++ lib/Driver/ToolChains/Gnu.cpp
@@ -1670,6 +1670,15 @@
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()) {
Index: lib/Driver/ToolChains/Gnu.cpp
===
--- lib/Driver/ToolChains/Gnu.cpp
+++ lib/Driver/ToolChains/Gnu.cpp
@@ -1670,6 +1670,15 @@
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()) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
