[PATCH] D57930: [Driver] Verify GCCInstallation is valid
This revision was automatically updated to reflect the committed changes.
Closed by commit rC361314: [Driver] Verify GCCInstallation is valid (authored
by nickdesaulniers, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D57930?vs=200580=200586#toc
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57930/new/
https://reviews.llvm.org/D57930
Files:
lib/Driver/ToolChains/Linux.cpp
test/Driver/B-opt.c
Index: test/Driver/B-opt.c
===
--- test/Driver/B-opt.c
+++ test/Driver/B-opt.c
@@ -20,3 +20,8 @@
// RUN: -B %S/Inputs/B_opt_tree/dir2 2>&1 -fuse-ld=ld \
// RUN: | FileCheck --check-prefix=CHECK-B-OPT-MULT %s
// CHECK-B-OPT-MULT: "{{.*}}/Inputs/B_opt_tree/dir3{{/|}}prefix-ld"
+//
+// RUN: %clang -B %S/Inputs/does_not_exist -print-search-dirs \
+// RUN: -target aarch64-linux-gnu \
+// RUN: | FileCheck --check-prefix=CHECK-B-OPT-INVALID %s
+// CHECK-B-OPT-INVALID-NOT: /..//bin
Index: lib/Driver/ToolChains/Linux.cpp
===
--- lib/Driver/ToolChains/Linux.cpp
+++ lib/Driver/ToolChains/Linux.cpp
@@ -234,9 +234,11 @@
// used to target i386.
// FIXME: This seems unlikely to be Linux-specific.
ToolChain::path_list = getProgramPaths();
- PPaths.push_back(Twine(GCCInstallation.getParentLibPath() + "/../" +
- GCCInstallation.getTriple().str() + "/bin")
- .str());
+ if (GCCInstallation.isValid()) {
+PPaths.push_back(Twine(GCCInstallation.getParentLibPath() + "/../" +
+ GCCInstallation.getTriple().str() + "/bin")
+ .str());
+ }
Distro Distro(D.getVFS());
Index: test/Driver/B-opt.c
===
--- test/Driver/B-opt.c
+++ test/Driver/B-opt.c
@@ -20,3 +20,8 @@
// RUN: -B %S/Inputs/B_opt_tree/dir2 2>&1 -fuse-ld=ld \
// RUN: | FileCheck --check-prefix=CHECK-B-OPT-MULT %s
// CHECK-B-OPT-MULT: "{{.*}}/Inputs/B_opt_tree/dir3{{/|}}prefix-ld"
+//
+// RUN: %clang -B %S/Inputs/does_not_exist -print-search-dirs \
+// RUN: -target aarch64-linux-gnu \
+// RUN: | FileCheck --check-prefix=CHECK-B-OPT-INVALID %s
+// CHECK-B-OPT-INVALID-NOT: /..//bin
Index: lib/Driver/ToolChains/Linux.cpp
===
--- lib/Driver/ToolChains/Linux.cpp
+++ lib/Driver/ToolChains/Linux.cpp
@@ -234,9 +234,11 @@
// used to target i386.
// FIXME: This seems unlikely to be Linux-specific.
ToolChain::path_list = getProgramPaths();
- PPaths.push_back(Twine(GCCInstallation.getParentLibPath() + "/../" +
- GCCInstallation.getTriple().str() + "/bin")
- .str());
+ if (GCCInstallation.isValid()) {
+PPaths.push_back(Twine(GCCInstallation.getParentLibPath() + "/../" +
+ GCCInstallation.getTriple().str() + "/bin")
+ .str());
+ }
Distro Distro(D.getVFS());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D57930: [Driver] Verify GCCInstallation is valid
srhines accepted this revision. srhines added a comment. This revision is now accepted and ready to land. Thanks for picking this up and finishing it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57930/new/ https://reviews.llvm.org/D57930 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D57930: [Driver] Verify GCCInstallation is valid
nickdesaulniers updated this revision to Diff 200580.
nickdesaulniers added a comment.
Herald added a subscriber: ormris.
- add unit test
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57930/new/
https://reviews.llvm.org/D57930
Files:
clang/lib/Driver/ToolChains/Linux.cpp
clang/test/Driver/B-opt.c
Index: clang/test/Driver/B-opt.c
===
--- clang/test/Driver/B-opt.c
+++ clang/test/Driver/B-opt.c
@@ -20,3 +20,8 @@
// RUN: -B %S/Inputs/B_opt_tree/dir2 2>&1 -fuse-ld=ld \
// RUN: | FileCheck --check-prefix=CHECK-B-OPT-MULT %s
// CHECK-B-OPT-MULT: "{{.*}}/Inputs/B_opt_tree/dir3{{/|}}prefix-ld"
+//
+// RUN: %clang -B %S/Inputs/does_not_exist -print-search-dirs \
+// RUN: -target aarch64-linux-gnu \
+// RUN: | FileCheck --check-prefix=CHECK-B-OPT-INVALID %s
+// CHECK-B-OPT-INVALID-NOT: /..//bin
Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -234,9 +234,11 @@
// used to target i386.
// FIXME: This seems unlikely to be Linux-specific.
ToolChain::path_list = getProgramPaths();
- PPaths.push_back(Twine(GCCInstallation.getParentLibPath() + "/../" +
- GCCInstallation.getTriple().str() + "/bin")
- .str());
+ if (GCCInstallation.isValid()) {
+PPaths.push_back(Twine(GCCInstallation.getParentLibPath() + "/../" +
+ GCCInstallation.getTriple().str() + "/bin")
+ .str());
+ }
Distro Distro(D.getVFS());
Index: clang/test/Driver/B-opt.c
===
--- clang/test/Driver/B-opt.c
+++ clang/test/Driver/B-opt.c
@@ -20,3 +20,8 @@
// RUN: -B %S/Inputs/B_opt_tree/dir2 2>&1 -fuse-ld=ld \
// RUN: | FileCheck --check-prefix=CHECK-B-OPT-MULT %s
// CHECK-B-OPT-MULT: "{{.*}}/Inputs/B_opt_tree/dir3{{/|}}prefix-ld"
+//
+// RUN: %clang -B %S/Inputs/does_not_exist -print-search-dirs \
+// RUN: -target aarch64-linux-gnu \
+// RUN: | FileCheck --check-prefix=CHECK-B-OPT-INVALID %s
+// CHECK-B-OPT-INVALID-NOT: /..//bin
Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -234,9 +234,11 @@
// used to target i386.
// FIXME: This seems unlikely to be Linux-specific.
ToolChain::path_list = getProgramPaths();
- PPaths.push_back(Twine(GCCInstallation.getParentLibPath() + "/../" +
- GCCInstallation.getTriple().str() + "/bin")
- .str());
+ if (GCCInstallation.isValid()) {
+PPaths.push_back(Twine(GCCInstallation.getParentLibPath() + "/../" +
+ GCCInstallation.getTriple().str() + "/bin")
+ .str());
+ }
Distro Distro(D.getVFS());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D57930: [Driver] Verify GCCInstallation is valid
nickdesaulniers added a comment. @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=` fails in an explicit way. Would you mind adding such a test, so we can ship this useful fix? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57930/new/ https://reviews.llvm.org/D57930 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D57930: [Driver] Verify GCCInstallation is valid
danielmentz marked an inline comment as done.
danielmentz added inline comments.
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:259-260
if (GCCInstallation.getParentLibPath().find("opt/rh/devtoolset") !=
StringRef::npos)
// With devtoolset on RHEL, we want to add a bin directory that is relative
tstellar wrote:
> Do we need to add the same check here too?
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57930/new/
https://reviews.llvm.org/D57930
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D57930: [Driver] Verify GCCInstallation is valid
tstellar added inline comments.
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:259-260
if (GCCInstallation.getParentLibPath().find("opt/rh/devtoolset") !=
StringRef::npos)
// With devtoolset on RHEL, we want to add a bin directory that is relative
Do we need to add the same check here too?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57930/new/
https://reviews.llvm.org/D57930
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D57930: [Driver] Verify GCCInstallation is valid
srhines added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57930/new/ https://reviews.llvm.org/D57930 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D57930: [Driver] Verify GCCInstallation is valid
danielmentz created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D57930
Files:
clang/lib/Driver/ToolChains/Linux.cpp
Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -229,9 +229,11 @@
// used to target i386.
// FIXME: This seems unlikely to be Linux-specific.
ToolChain::path_list = getProgramPaths();
- PPaths.push_back(Twine(GCCInstallation.getParentLibPath() + "/../" +
- GCCInstallation.getTriple().str() + "/bin")
- .str());
+ if (GCCInstallation.isValid()) {
+PPaths.push_back(Twine(GCCInstallation.getParentLibPath() + "/../" +
+ GCCInstallation.getTriple().str() + "/bin")
+ .str());
+ }
Distro Distro(D.getVFS());
Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -229,9 +229,11 @@
// used to target i386.
// FIXME: This seems unlikely to be Linux-specific.
ToolChain::path_list = getProgramPaths();
- PPaths.push_back(Twine(GCCInstallation.getParentLibPath() + "/../" +
- GCCInstallation.getTriple().str() + "/bin")
- .str());
+ if (GCCInstallation.isValid()) {
+PPaths.push_back(Twine(GCCInstallation.getParentLibPath() + "/../" +
+ GCCInstallation.getTriple().str() + "/bin")
+ .str());
+ }
Distro Distro(D.getVFS());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
