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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers requested changes to this revision.
nickdesaulniers added a comment.
This revision now requires changes to proceed.

It's not clear to me why Arch is special here. The path may be different, but normalizing the triple seems unnecessary. Blocking this until I'm confident that we're getting to the root cause of the issue here.



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


================
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:
> > 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.


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

https://reviews.llvm.org/D134454