[PATCH] D134454: [Driver][Distro] Fix ArchLinux sysroot detection

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:363
   const StringRef TripleStr = GCCInstallation.getTriple().str();
-  const Multilib &Multilib = GCCInstallation.getMultilib();
+  std::string Path = (InstallDir + "/../../../../" + TripleStr).str();
 
----------------
With all of the string concatenation going on, I wonder if `Path` should be a `Twine`?  `llvm::vfs::Filesystem::exists` accepts a `const Twine&`.  That avoids multiple reallocations and copies, and does one lazily.

(Every time I've tried to use `Twine`, I wind up with either `-Wdangling-gsl` or segfaults though! Still, please give it a shot.)


================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:369-375
+  if (getTriple().isCSKY())
+    Path = Path + "/libc";
 
-  if (getVFS().exists(Path))
-    return Path;
+  // Standalone MIPS toolchains use different names for sysroot folder
+  // and put it into different places. Here we try to check some known
+  // variants.
+  if (getTriple().isMIPS()) {
----------------
CSKY and MIPS should be mutually exclusive. Please make this second `if` an `else if`.

```
if csky:
  ...
elif mips:
  ...
```


================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:370
+  if (getTriple().isCSKY())
+    Path = Path + "/libc";
 
----------------
+=


================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:373
+  // Standalone MIPS toolchains use different names for sysroot folder
+  // and put it into different places. Here we try to check some known
+  // variants.
----------------
Specifically there's 2 known variants for mips, it looks like. Please update this comment.


================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:376
+  if (getTriple().isMIPS()) {
+    const Multilib &Multilib = GCCInstallation.getMultilib();
+    Path = Path + "/libc" + Multilib.osSuffix();
----------------
It might be worthwhile to have the variable be
```
std::string OSSuffix = GCCInstallation.getMultilib().osSuffix();
```
rather than the `Multilib` object.


================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:377
+    const Multilib &Multilib = GCCInstallation.getMultilib();
+    Path = Path + "/libc" + Multilib.osSuffix();
+    if (getVFS().exists(Path))
----------------
+=


================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:377-389
   const StringRef InstallDir = GCCInstallation.getInstallPath();
   const StringRef TripleStr = GCCInstallation.getTriple().str();
   const Multilib &Multilib = GCCInstallation.getMultilib();
+  std::string BasePath = (InstallDir + "/../../../../" + TripleStr).str();
 
-  std::string Path =
-      (InstallDir + "/../../../../" + TripleStr + "/libc" + Multilib.osSuffix())
-          .str();
-
-  if (getVFS().exists(Path))
-    return Path;
+  if (Distro.IsArchLinux() && getVFS().exists(BasePath))
+    return BasePath;
----------------
10ne1 wrote:
> nickdesaulniers wrote:
> > ...seems like some of this is duplicated with CSKY above. Can you please refactor the MIPS and CSKY checks to reuse more code?
> > 
> > I doubt arch-linux has a CSKY port; I suspect you might be able to check for arch-linux first, then do the CSKY and MIPS special casing after.
> > 
> > All this code is doing is figuring out a Path, then if it exists then return it.  Feel like is should be:
> > 
> > ```
> > if android:
> >   path = ...
> > elif arch:
> >   path = ...
> > elif csky:
> >   path = ...
> > elif mips:
> >   path = ...
> > else:
> >   return ""
> > 
> > if path.exists():
> >   return path
> > return ""
> > ```
> Thanks for this suggestion. Indeed after doing all the simplifications I have some great news: we do not need to test if Distro == ArchLinux because the sysroot path it uses is the implicit base path one common to all architectures!
> 
> So just doing the following is enough to also fix ArchLinux:
> 
> ```
>   std::string Path = (InstallDir + "/../../../../" + TripleStr).str();
>   
>   if (getTriple().isCSKY())
>     Path = Path + "/libc";
>   
>   if (getTriple().isMIPS()) {
>     ...
>    }
> 
>   if (getVFS().exists(Path))
>     return Path;
> 
>   return std::string();
> ```
Yes, this is much nicer in that we don't have to special case arch-linux.  Sometimes refactorings beget more refactorings; it's nice when that happens.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134454/new/

https://reviews.llvm.org/D134454