[PATCH] D57930: [Driver] Verify GCCInstallation is valid

[Daniel Mentz via Phabricator via cfe-commits]

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

[Daniel Mentz via Phabricator via cfe-commits]

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