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

[Adrian Ratiu via Phabricator via cfe-commits]

10ne1 added inline comments.


================
Comment at: clang/lib/Driver/Distro.cpp:213
+  // that the Linux OS and distro are properly detected in this cases.
+  llvm::Triple NormTargetOrHost = llvm::Triple(Twine(TargetOrHost.normalize()));
+
----------------
nickdesaulniers wrote:
> 10ne1 wrote:
> > nickdesaulniers wrote:
> > > Twine has an intentionally non-explicit constructor that accepts a StringRef, which also has an intentionally non-explicit constructor that accepts a std::string, which is what Triple::normalize() returns.
> > > 
> > > Let's be consistent in the explicit construction of these temporary objects by removing the explicit call to the Twine constructor.
> > > 
> > > Also, why is it necessary to normalize the Triple? Can you give an example of the "bad" input and how this "fixes" it?
> > I do not claim to fully understand the LLVM toolchain & triplet auto-detection code, so maybe this normalization is more of a workaround than a real fix. Maybe we need to do the normalization earlier? I do not know, any suggestions are welcome.
> > 
> > The behavior I'm seeing is:
> > 
> > If TargetOrHost triplet is "aarch64-linux-gnu" then TargetOrHost.isOSLinux() == false and GetDistro returns Distro::UnknownDistro which causes failures like the following when building the kernel tools:
> > 
> > ```
> > make ARCH=arm64 CC=clang CROSS_COMPILE=aarch64-linux-gnu- bpf
> >   DESCEND bpf
> > 
> > Auto-detecting system features:
> > ...                        libbfd: [ OFF ]
> > ...        disassembler-four-args: [ OFF ]
> > 
> > 
> >   DESCEND bpftool
> > 
> > Auto-detecting system features:
> > ...                        libbfd: [ on  ]
> > ...        disassembler-four-args: [ on  ]
> > ...                          zlib: [ OFF ]
> > ...                        libcap: [ OFF ]
> > ...               clang-bpf-co-re: [ on  ]
> > 
> > 
> > make[2]: *** No rule to make target '/home/adi/workspace/cola/GOO0021/chromiumos/src/third_party/kernel/v5.15/tools/include/linux/math.h', needed by 'btf.o'.  Stop.
> > make[1]: *** [Makefile:110: bpftool] Error 2
> > make: *** [Makefile:69: bpf] Error 2
> > ```
> > 
> > If we do the triple normalization step before detecting the distro, the triplet becomes `aarch64-unknown-linux-gnu` which results in TargetOrHost.isOSLinux() == true, the distro is properly detected, then the system features are ON and the build works.
> > If TargetOrHost triplet is "aarch64-linux-gnu" then TargetOrHost.isOSLinux() == false 
> 
> What?! That sounds like a bug. What does Triple::getOS() return in that case?
> 
> Otherwise it sounds like `GetDistro` is getting called to early, before whatever sets the OS has been initialized correctly.
I've done some further debugging and turns out there was an error in my test environment due to the Linux kernel tools/ cleanup patch not being correctly applied.

After fixing that I can confirm the values for the triple are correct and this normalization is not required anymore:

```
llvm::dbgs() << "TargetOrHost.getOS() = " << TargetOrHost.getOS() << "\n";
llvm::dbgs() << "TargetOrHost.isOSLinux() = " << TargetOrHost.isOSLinux() << "\n";
```

produces:

```
TargetOrHost.isOSLinux() = 1
TargetOrHost.getOS() = 9
```

which is what we expect.

I will drop this specific triplet code and update the diff, the only remaining open issue is the sysroot detection change below. Thanks for your patience!


================
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())
----------------
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. 




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

https://reviews.llvm.org/D134454