[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:360-361
     // CSKY toolchains use different names for sysroot folder.
     if (!GCCInstallation.isValid())
       return std::string();
     // GCCInstallation.getInstallPath() =
----------------
hoist


================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:365-366
     // Path = $GCCToolchainPath/csky-linux-gnuabiv2/libc
     std::string Path = (GCCInstallation.getInstallPath() + "/../../../../" +
                         GCCInstallation.getTriple().str() + "/libc")
                            .str();
----------------
duplication with below...


================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:373-374
 
-  if (!GCCInstallation.isValid() || !getTriple().isMIPS())
+  if (!GCCInstallation.isValid())
     return std::string();
 
----------------
Cool, now this can be hoisted above `if (getTriple().isCSKY()) {` and not checked again within that condition.


================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:376
 
-  // Standalone MIPS toolchains use different names for sysroot folder
-  // and put it into different places. Here we try to check some known
-  // variants.
-
+  const Distro Distro(getDriver().getVFS(), getTriple());
   const StringRef InstallDir = GCCInstallation.getInstallPath();
----------------
I see that `getVFS()` is called already without going through `getDriver()` in this method. Can we do so as well here?


================
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;
----------------
...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 ""
```


================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:377-394
+  if (Distro.IsArchLinux()) {
+    std::string Path = (InstallDir + "/../../../../"  + TripleStr).str();
+    if (getVFS().exists(Path))
+      return Path;
+  }
+
   if (!GCCInstallation.isValid() || !getTriple().isMIPS())
----------------
10ne1 wrote:
> nickdesaulniers wrote:
> > 10ne1 wrote:
> > > nickdesaulniers wrote:
> > > > Do we need the Arch-linux test before the call to `GCCInstallation.isValid()`? Otherwise it seems like this duplicates a fair amount of code computing the `Path`.  The initial parts of the Path seem to match; there's only more to it if the Distro is not arch-linux.
> > > In my testing on ArchLinux, `GCCInstallation.isValid()` is always `true`.
> > > 
> > > The problem is that if test condition doesn't just test for a valid GCC install, it also tests `getTriple().isMIPS()` which is always false on Arch Linux which does not support mips, so the sysroot will always be an empty string.
> > > 
> > > If you wish I can create a separate commit / review to untangle `if (!GCCInstallation.isValid() || !getTriple().isMIPS())` and try to reduce the code duplication, because really having a valid GCC install is independent from running on mips. What do you think?
> > That doesn't make sense.
> > 
> > If `GCCInstallation.isValid()` is always `true` then we should not be returning the empty string.
> It does makes sense if you read the condition carefully:
> 
> ```
> if (!GCCInstallation.isValid() || !getTriple().isMIPS())
> ```
> 
> results in:
> 
> ```
> if ( ! true || ! false )
> ```
> 
> which means an empty string is always returned because Arch does not support mips even though a valid GCC install is present.
> 
> I think I'll just go ahead and clean up this code and update the diff to drop the triple normalization. 
> 
> 
Ah, ok. Yeah your refactoring is making this clearer.  I think we can refactor this method a bit more (in addition to fixing this Arch-Linux specific issue).


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

https://reviews.llvm.org/D134454