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

Fri Sep 30 10:08:50 PDT 2022

nickdesaulniers added a comment.

In D134454#3824630 <https://reviews.llvm.org/D134454#3824630>, @MaskRay wrote:

> Another opinion is whether we actually need the more and more complex sysroot computation logic. 
> Inferring sysroot from GCCInstallation looks backward to me. We should get sysroot first, then compute GCCInstallation, not the order as implemented in this patch.
>
> Yes, I see mips (https://reviews.llvm.org/rG08450bd55ccdc4aee4f5f73cde97e25b3c4ce5b9 2013), Android (D45291 <https://reviews.llvm.org/D45291> in 2018) and csky (D121445 <https://reviews.llvm.org/D121445>) special cases. To be fair, if I saw them earlier, I'd raise my concerns as well.
>
> ---
>
> https://reviews.llvm.org/D134337 (default configuration file) will soon land and we can move to require distributions to provide a config file instead.

I disagree with the assessment that this patch makes sysroot detection //more// complex; it is a simplification and bug fix.  Detecting the sysroot first, then GCCInstallation is a higher risk change than this patch.

The default configuration file is interesting, but it's not clear what even arch-linux would put in their configuration file; this patch cleans up what looks like a bug to me introduced in https://reviews.llvm.org/rG08450bd55ccdc4aee4f5f73cde97e25b3c4ce5b9 that folks have been working around downstream for quite some time.

---

I think @10ne1 just needs to fix up the style nit, and a test might be nice.

clang/test/Driver/linux-cross.cpp already has a test using a "fake" Arch-Linux tree in clang/test/Driver/Inputs/archlinux_i686_tree/. A new "fake" tree could be added to emulate the current way arch lays out these sysroots.

================
Comment at: clang/lib/Driver/ToolChains/Linux.cpp:375
+  else if (getTriple().isMIPS()) {
+    const std::string &OSSuffix = GCCInstallation.getMultilib().osSuffix();

----------------
clang/lib/Driver/ToolChains/Linux.cpp `Linux::Linux` has:
```
  if (IsCSKY)
    SysRoot = SysRoot + SelectedMultilib.osSuffix();
```
lol.  So we should either do this for BOTH mips and csky here, or there.

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

https://reviews.llvm.org/D134454