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

[Adrian Ratiu via Phabricator via cfe-commits]

10ne1 created this revision.
10ne1 added reviewers: nickdesaulniers, nathanchance, manojgupta.
Herald added a subscriber: kristof.beyls.
Herald added a project: All.
10ne1 requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.

While cross-compiling Linux kernel tools with Clang on ArchLinux, it
was revealed the sysroot and triplet are not properly detected and
instead of fixing the root cause in Clang, kernel developers started
doing workarounds [1] upon workarounds [2] in the kernel build to be
able to pass a working sysroot and triplet during Clang invocation.

To give a concrete example, ArchLinux installs its cross-compliation
sysroot in /usr/aarch64-linux-gnu for triplet aarch64-linux-gnu.
This causes two problems which this commit fixes:

1. The triplet results in an Unknown OS & Unknown distro. To fix this

error, we call Triplet::normalize() which does the following transform
aarch64-linux-gnu -> aarch64-unknown-linux-gnu, then the OS & Distro
are properly detected.

2. The sysroot under /usr/ itself is not properly detected

in Linux::computeSysRoot() so we add a relative path to the ArchLinux
installed GCC toolchain.

Fixing these two bugs allows to significantly clean up the kernel
build as well as fix the cross-compilation detection on ArchLinux.

[1] 
https://github.com/torvalds/linux/commit/cebdb7374577ac6e14afb11311af8c2c44a259fa
[2] 
https://github.com/torvalds/linux/commit/7fd9fd46a459272e641be78c1cc36baab1921fa1


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D134454

Files:
  clang/include/clang/Driver/Distro.h
  clang/lib/Driver/Distro.cpp
  clang/lib/Driver/ToolChains/Linux.cpp


Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -370,6 +370,16 @@
 return std::string();
   }
 
+  const Distro Distro(getDriver().getVFS(), getTriple());
+  const StringRef InstallDir = GCCInstallation.getInstallPath();
+  const StringRef TripleStr = GCCInstallation.getTriple().str();
+
+  if (Distro.IsArchLinux()) {
+std::string Path = (InstallDir + "/../../../../"  + TripleStr).str();
+if (getVFS().exists(Path))
+  return Path;
+  }
+
   if (!GCCInstallation.isValid() || !getTriple().isMIPS())
 return std::string();
 
@@ -377,8 +387,6 @@
   // and put it into different places. Here we try to check some known
   // variants.
 
-  const StringRef InstallDir = GCCInstallation.getInstallPath();
-  const StringRef TripleStr = GCCInstallation.getTriple().str();
   const Multilib &Multilib = GCCInstallation.getMultilib();
 
   std::string Path =
Index: clang/lib/Driver/Distro.cpp
===
--- clang/lib/Driver/Distro.cpp
+++ clang/lib/Driver/Distro.cpp
@@ -205,9 +205,16 @@
 
 static Distro::DistroType GetDistro(llvm::vfs::FileSystem &VFS,
 const llvm::Triple &TargetOrHost) {
+
+  // Sometimes the OS can't be detected from the triplet due to ambiguity, for
+  // eg. ArchLinux uses aarch64-linux-gnu which results in Uknonwn OS & distro,
+  // so normalize the triplet which results in aarch64-unknown-linux-gnu, such
+  // that the Linux OS and distro are properly detected in this cases.
+  llvm::Triple NormTargetOrHost = 
llvm::Triple(Twine(TargetOrHost.normalize()));
+
   // If we don't target Linux, no need to check the distro. This saves a few
   // OS calls.
-  if (!TargetOrHost.isOSLinux())
+  if (!NormTargetOrHost.isOSLinux())
 return Distro::UnknownDistro;
 
   // True if we're backed by a real file system.
Index: clang/include/clang/Driver/Distro.h
===
--- clang/include/clang/Driver/Distro.h
+++ clang/include/clang/Driver/Distro.h
@@ -134,6 +134,8 @@
 
   bool IsGentoo() const { return DistroVal == Gentoo; }
 
+  bool IsArchLinux() const { return DistroVal == ArchLinux; }
+
   /// @}
 };
 


Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -370,6 +370,16 @@
 return std::string();
   }
 
+  const Distro Distro(getDriver().getVFS(), getTriple());
+  const StringRef InstallDir = GCCInstallation.getInstallPath();
+  const StringRef TripleStr = GCCInstallation.getTriple().str();
+
+  if (Distro.IsArchLinux()) {
+std::string Path = (InstallDir + "/../../../../"  + TripleStr).str();
+if (getVFS().exists(Path))
+  return Path;
+  }
+
   if (!GCCInstallation.isValid() || !getTriple().isMIPS())
 return std::string();
 
@@ -377,8 +387,6 @@
   // and put it into different places. Here we try to check some known
   // variants.
 
-  const StringRef InstallDir = GCCInstallation.getInstallPath();
-  const StringRef TripleStr = GCCInstallation.getTriple

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

[Adrian Ratiu via Phabricator via cfe-commits]

10ne1 updated this revision to Diff 462423.
10ne1 added a comment.

Regenerated diff with git diff HEAD~1 -U99


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

https://reviews.llvm.org/D134454

Files:
  clang/include/clang/Driver/Distro.h
  clang/lib/Driver/Distro.cpp
  clang/lib/Driver/ToolChains/Linux.cpp


Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -370,6 +370,16 @@
 return std::string();
   }
 
+  const Distro Distro(getDriver().getVFS(), getTriple());
+  const StringRef InstallDir = GCCInstallation.getInstallPath();
+  const StringRef TripleStr = GCCInstallation.getTriple().str();
+
+  if (Distro.IsArchLinux()) {
+std::string Path = (InstallDir + "/../../../../"  + TripleStr).str();
+if (getVFS().exists(Path))
+  return Path;
+  }
+
   if (!GCCInstallation.isValid() || !getTriple().isMIPS())
 return std::string();
 
@@ -377,8 +387,6 @@
   // and put it into different places. Here we try to check some known
   // variants.
 
-  const StringRef InstallDir = GCCInstallation.getInstallPath();
-  const StringRef TripleStr = GCCInstallation.getTriple().str();
   const Multilib &Multilib = GCCInstallation.getMultilib();
 
   std::string Path =
Index: clang/lib/Driver/Distro.cpp
===
--- clang/lib/Driver/Distro.cpp
+++ clang/lib/Driver/Distro.cpp
@@ -205,9 +205,16 @@
 
 static Distro::DistroType GetDistro(llvm::vfs::FileSystem &VFS,
 const llvm::Triple &TargetOrHost) {
+
+  // Sometimes the OS can't be detected from the triplet due to ambiguity, for
+  // eg. ArchLinux uses aarch64-linux-gnu which results in Uknonwn OS & distro,
+  // so normalize the triplet which results in aarch64-unknown-linux-gnu, such
+  // that the Linux OS and distro are properly detected in this cases.
+  llvm::Triple NormTargetOrHost = 
llvm::Triple(Twine(TargetOrHost.normalize()));
+
   // If we don't target Linux, no need to check the distro. This saves a few
   // OS calls.
-  if (!TargetOrHost.isOSLinux())
+  if (!NormTargetOrHost.isOSLinux())
 return Distro::UnknownDistro;
 
   // True if we're backed by a real file system.
Index: clang/include/clang/Driver/Distro.h
===
--- clang/include/clang/Driver/Distro.h
+++ clang/include/clang/Driver/Distro.h
@@ -134,6 +134,8 @@
 
   bool IsGentoo() const { return DistroVal == Gentoo; }
 
+  bool IsArchLinux() const { return DistroVal == ArchLinux; }
+
   /// @}
 };
 


Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -370,6 +370,16 @@
 return std::string();
   }
 
+  const Distro Distro(getDriver().getVFS(), getTriple());
+  const StringRef InstallDir = GCCInstallation.getInstallPath();
+  const StringRef TripleStr = GCCInstallation.getTriple().str();
+
+  if (Distro.IsArchLinux()) {
+std::string Path = (InstallDir + "/../../../../"  + TripleStr).str();
+if (getVFS().exists(Path))
+  return Path;
+  }
+
   if (!GCCInstallation.isValid() || !getTriple().isMIPS())
 return std::string();
 
@@ -377,8 +387,6 @@
   // and put it into different places. Here we try to check some known
   // variants.
 
-  const StringRef InstallDir = GCCInstallation.getInstallPath();
-  const StringRef TripleStr = GCCInstallation.getTriple().str();
   const Multilib &Multilib = GCCInstallation.getMultilib();
 
   std::string Path =
Index: clang/lib/Driver/Distro.cpp
===
--- clang/lib/Driver/Distro.cpp
+++ clang/lib/Driver/Distro.cpp
@@ -205,9 +205,16 @@
 
 static Distro::DistroType GetDistro(llvm::vfs::FileSystem &VFS,
 const llvm::Triple &TargetOrHost) {
+
+  // Sometimes the OS can't be detected from the triplet due to ambiguity, for
+  // eg. ArchLinux uses aarch64-linux-gnu which results in Uknonwn OS & distro,
+  // so normalize the triplet which results in aarch64-unknown-linux-gnu, such
+  // that the Linux OS and distro are properly detected in this cases.
+  llvm::Triple NormTargetOrHost = llvm::Triple(Twine(TargetOrHost.normalize()));
+
   // If we don't target Linux, no need to check the distro. This saves a few
   // OS calls.
-  if (!TargetOrHost.isOSLinux())
+  if (!NormTargetOrHost.isOSLinux())
 return Distro::UnknownDistro;
 
   // True if we're backed by a real file system.
Index: clang/include/clang/Driver/Distro.h
===
--- clang/include/clang/Driver/Distro.h
+++ clang/include/clang/Driver/Distro.h
@@ -134,6 +134,8 @@
 
   bool IsGentoo() const { return DistroVal == Gentoo; }
 
+  bool IsArchLinux() co

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

[Adrian Ratiu via Phabricator via cfe-commits]

10ne1 added a comment.

In D134454#3812153 , @nickdesaulniers 
wrote:

> Is it worth contacting the package maintainer for LLVM+clang for Arch-Linux 
> in regards to this patch?

Yes, thanks for the suggestion, I will open an issue on the Arch bug tracker 
for clang so the maintainer is aware and he can also be part of the discussion 
here.

I also plan to back-port this fix once it gets into a suitable form in LLVM so 
having the Arch PKGBUILD fixed (Arch is a rolling release, no stable backports 
required) would allow us to also clean up the Linux kernel tools tree.


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

https://reviews.llvm.org/D134454

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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


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

https://reviews.llvm.org/D134454

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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/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:
> 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?


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

https://reviews.llvm.org/D134454

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 (!GCC

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

[Adrian Ratiu via Phabricator via cfe-commits]

10ne1 added a comment.

In D134454#3816460 , @MaskRay wrote:

> I am nervous as well as Arch Linux has many derivatives. They likely don't 
> use `ID=arch` in `/etc/os-release`. The patch won't work for them.
> In general, I don't think our current approach adding new `Distro::*` flavors 
> scale or really meet the needs of numerous less-popular distributions.
>
> The last few comments of 
> https://discourse.llvm.org/t/rfc-adding-a-default-file-location-to-config-file-support/63606
>  discuss a generic mechanism solving the distribution difference problem with 
> configuration files.
> Also, the new driver option `--gcc-install-dir` 
> (https://discourse.llvm.org/t/add-gcc-install-dir-deprecate-gcc-toolchain-and-remove-gcc-install-prefix/65091
>  ; milestone: 16.0.0) can be useful for some tasks.

@MaskRay I agree the current /etc/os-release and Distro::* based OS detection 
is not scalable and will require adding more and more per-distro exceptions, 
which is ugly.

However what other option do we currently have other than waiting for the 
generic distro mechanism to be implemented and continue doing workarounds in 
projects depending on LLVM/Clang like the kernel?

My suggestion is to add a Distro::* exception to fix Arch Linux now which will 
also allow to clean up the kernel code and **afterwards**, when the generic 
mechanism is finally implemented, this along with all the other Distro:* 
ugliness can be dropped. Basically I try to avoid blocking for an indefinite 
amount of time for a proper solution, which when it will finally come, will 
drop this code anyway.

What do you think? If you say we should wait, that is fine with me as well.


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

https://reviews.llvm.org/D134454

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian Ratiu via Phabricator via cfe-commits]

10ne1 updated this revision to Diff 463177.
10ne1 retitled this revision from "[Driver][Distro] Fix ArchLinux triplet and 
sysroot detection" to "[Driver][Distro] Fix ArchLinux sysroot detection".
10ne1 edited the summary of this revision.

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

https://reviews.llvm.org/D134454

Files:
  clang/include/clang/Driver/Distro.h
  clang/lib/Driver/ToolChains/Linux.cpp


Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -370,28 +370,30 @@
 return std::string();
   }
 
-  if (!GCCInstallation.isValid() || !getTriple().isMIPS())
+  if (!GCCInstallation.isValid())
 return std::string();
 
-  // 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();
   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;
 
-  Path = (InstallDir + "/../../../../sysroot" + Multilib.osSuffix()).str();
+  // 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()) {
+std::string Path = BasePath + "/libc" + Multilib.osSuffix();
+if (getVFS().exists(Path))
+  return Path;
 
-  if (getVFS().exists(Path))
-return Path;
+Path = (InstallDir + "/../../../../sysroot" + Multilib.osSuffix()).str();
+if (getVFS().exists(Path))
+  return Path;
+  }
 
   return std::string();
 }
Index: clang/include/clang/Driver/Distro.h
===
--- clang/include/clang/Driver/Distro.h
+++ clang/include/clang/Driver/Distro.h
@@ -134,6 +134,8 @@
 
   bool IsGentoo() const { return DistroVal == Gentoo; }
 
+  bool IsArchLinux() const { return DistroVal == ArchLinux; }
+
   /// @}
 };
 


Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -370,28 +370,30 @@
 return std::string();
   }
 
-  if (!GCCInstallation.isValid() || !getTriple().isMIPS())
+  if (!GCCInstallation.isValid())
 return std::string();
 
-  // 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();
   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;
 
-  Path = (InstallDir + "/../../../../sysroot" + Multilib.osSuffix()).str();
+  // 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()) {
+std::string Path = BasePath + "/libc" + Multilib.osSuffix();
+if (getVFS().exists(Path))
+  return Path;
 
-  if (getVFS().exists(Path))
-return Path;
+Path = (InstallDir + "/../../../../sysroot" + Multilib.osSuffix()).str();
+if (getVFS().exists(Path))
+  return Path;
+  }
 
   return std::string();
 }
Index: clang/include/clang/Driver/Distro.h
===
--- clang/include/clang/Driver/Distro.h
+++ clang/include/clang/Driver/Distro.h
@@ -134,6 +134,8 @@
 
   bool IsGentoo() const { return DistroVal == Gentoo; }
 
+  bool IsArchLinux() const { return DistroVal == ArchLinux; }
+
   /// @}
 };
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian Ratiu via Phabricator via cfe-commits]

10ne1 marked 2 inline comments as done.
10ne1 added a comment.

@nickdesaulniers @MaskRay I've updated the diff & summary based on the review 
comments which I've marked as done. Please tell me what you think. Thank you!


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

https://reviews.llvm.org/D134454

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian Ratiu via Phabricator via cfe-commits]

10ne1 updated this revision to Diff 463189.
10ne1 added a comment.

Fixed some clang-format problems.


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

https://reviews.llvm.org/D134454

Files:
  clang/include/clang/Driver/Distro.h
  clang/lib/Driver/ToolChains/Linux.cpp


Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -370,28 +370,30 @@
 return std::string();
   }
 
-  if (!GCCInstallation.isValid() || !getTriple().isMIPS())
+  if (!GCCInstallation.isValid())
 return std::string();
 
-  // 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();
   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;
 
-  Path = (InstallDir + "/../../../../sysroot" + Multilib.osSuffix()).str();
+  // 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()) {
+std::string Path = BasePath + "/libc" + Multilib.osSuffix();
+if (getVFS().exists(Path))
+  return Path;
 
-  if (getVFS().exists(Path))
-return Path;
+Path = (InstallDir + "/../../../../sysroot" + Multilib.osSuffix()).str();
+if (getVFS().exists(Path))
+  return Path;
+  }
 
   return std::string();
 }
Index: clang/include/clang/Driver/Distro.h
===
--- clang/include/clang/Driver/Distro.h
+++ clang/include/clang/Driver/Distro.h
@@ -134,6 +134,8 @@
 
   bool IsGentoo() const { return DistroVal == Gentoo; }
 
+  bool IsArchLinux() const { return DistroVal == ArchLinux; }
+
   /// @}
 };
 


Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -370,28 +370,30 @@
 return std::string();
   }
 
-  if (!GCCInstallation.isValid() || !getTriple().isMIPS())
+  if (!GCCInstallation.isValid())
 return std::string();
 
-  // 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();
   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;
 
-  Path = (InstallDir + "/../../../../sysroot" + Multilib.osSuffix()).str();
+  // 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()) {
+std::string Path = BasePath + "/libc" + Multilib.osSuffix();
+if (getVFS().exists(Path))
+  return Path;
 
-  if (getVFS().exists(Path))
-return Path;
+Path = (InstallDir + "/../../../../sysroot" + Multilib.osSuffix()).str();
+if (getVFS().exists(Path))
+  return Path;
+  }
 
   return std::string();
 }
Index: clang/include/clang/Driver/Distro.h
===
--- clang/include/clang/Driver/Distro.h
+++ clang/include/clang/Driver/Distro.h
@@ -134,6 +134,8 @@
 
   bool IsGentoo() const { return DistroVal == Gentoo; }
 
+  bool IsArchLinux() const { return DistroVal == ArchLinux; }
+
   /// @}
 };
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian Ratiu via Phabricator via cfe-commits]

10ne1 marked 5 inline comments as done.
10ne1 added a comment.

FYI: @MaskRay I think you will be very happy that after the simplifications 
Nick suggested the Distro::* additions are not necessary anymore.




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;

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();
```


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

https://reviews.llvm.org/D134454

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian Ratiu via Phabricator via cfe-commits]

10ne1 updated this revision to Diff 463465.
10ne1 marked an inline comment as done.
10ne1 edited the summary of this revision.
Herald added subscribers: atanasyan, arichardson, sdardis.

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

https://reviews.llvm.org/D134454

Files:
  clang/lib/Driver/ToolChains/Linux.cpp


Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -355,40 +355,31 @@
   return AndroidSysRootPath;
   }
 
-  if (getTriple().isCSKY()) {
-// CSKY toolchains use different names for sysroot folder.
-if (!GCCInstallation.isValid())
-  return std::string();
-// GCCInstallation.getInstallPath() =
-//   $GCCToolchainPath/lib/gcc/csky-linux-gnuabiv2/6.3.0
-// Path = $GCCToolchainPath/csky-linux-gnuabiv2/libc
-std::string Path = (GCCInstallation.getInstallPath() + "/../../../../" +
-GCCInstallation.getTriple().str() + "/libc")
-   .str();
-if (getVFS().exists(Path))
-  return Path;
-return std::string();
-  }
-
-  if (!GCCInstallation.isValid() || !getTriple().isMIPS())
+  if (!GCCInstallation.isValid())
 return std::string();
 
-  // Standalone MIPS toolchains use different names for sysroot folder
-  // and put it into different places. Here we try to check some known
-  // variants.
-
   const StringRef InstallDir = GCCInstallation.getInstallPath();
   const StringRef TripleStr = GCCInstallation.getTriple().str();
-  const Multilib &Multilib = GCCInstallation.getMultilib();
+  std::string Path = (InstallDir + "/../../../../" + TripleStr).str();
 
-  std::string Path =
-  (InstallDir + "/../../../../" + TripleStr + "/libc" + 
Multilib.osSuffix())
-  .str();
+  // CSKY toolchains use different names for sysroot folder.
+  // GCCInstallation.getInstallPath() =
+  //   $GCCToolchainPath/lib/gcc/csky-linux-gnuabiv2/6.3.0
+  // Path = $GCCToolchainPath/csky-linux-gnuabiv2/libc
+  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()) {
+const Multilib &Multilib = GCCInstallation.getMultilib();
+Path = Path + "/libc" + Multilib.osSuffix();
+if (getVFS().exists(Path))
+  return Path;
 
-  Path = (InstallDir + "/../../../../sysroot" + Multilib.osSuffix()).str();
+Path = (InstallDir + "/../../../../sysroot" + Multilib.osSuffix()).str();
+  }
 
   if (getVFS().exists(Path))
 return Path;


Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -355,40 +355,31 @@
   return AndroidSysRootPath;
   }
 
-  if (getTriple().isCSKY()) {
-// CSKY toolchains use different names for sysroot folder.
-if (!GCCInstallation.isValid())
-  return std::string();
-// GCCInstallation.getInstallPath() =
-//   $GCCToolchainPath/lib/gcc/csky-linux-gnuabiv2/6.3.0
-// Path = $GCCToolchainPath/csky-linux-gnuabiv2/libc
-std::string Path = (GCCInstallation.getInstallPath() + "/../../../../" +
-GCCInstallation.getTriple().str() + "/libc")
-   .str();
-if (getVFS().exists(Path))
-  return Path;
-return std::string();
-  }
-
-  if (!GCCInstallation.isValid() || !getTriple().isMIPS())
+  if (!GCCInstallation.isValid())
 return std::string();
 
-  // Standalone MIPS toolchains use different names for sysroot folder
-  // and put it into different places. Here we try to check some known
-  // variants.
-
   const StringRef InstallDir = GCCInstallation.getInstallPath();
   const StringRef TripleStr = GCCInstallation.getTriple().str();
-  const Multilib &Multilib = GCCInstallation.getMultilib();
+  std::string Path = (InstallDir + "/../../../../" + TripleStr).str();
 
-  std::string Path =
-  (InstallDir + "/../../../../" + TripleStr + "/libc" + Multilib.osSuffix())
-  .str();
+  // CSKY toolchains use different names for sysroot folder.
+  // GCCInstallation.getInstallPath() =
+  //   $GCCToolchainPath/lib/gcc/csky-linux-gnuabiv2/6.3.0
+  // Path = $GCCToolchainPath/csky-linux-gnuabiv2/libc
+  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()) {
+const Multilib &Multilib = GCCInstallation.getMultilib();
+Path = Path + "/libc" + Multilib.osSuffix();
+if (getVFS().exists(Path))
+  return Path;
 
-  Path = (InstallDi

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

[Adrian Ratiu via Phabricator via cfe-commits]

10ne1 marked 7 inline comments as done.
10ne1 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();
 

nickdesaulniers wrote:
> 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.)
I tried making Twine work but I got many errors, couldn't even make it build. 
Looks like twine doesn't have `+=` operator for strings, and I also got many 
`error: use of deleted function ‘llvm::Twine& llvm::Twine::operator=(const 
llvm::Twine&)’`. C++ is such a mystery language.

Please let's just leave it a string for now, it's enough cleanups. :) Thanks!


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

https://reviews.llvm.org/D134454

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian Ratiu via Phabricator via cfe-commits]

10ne1 updated this revision to Diff 463864.
10ne1 marked an inline comment as done.
10ne1 added a comment.

Updated based on feedback from Nick.


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

https://reviews.llvm.org/D134454

Files:
  clang/lib/Driver/ToolChains/Linux.cpp


Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -355,40 +355,31 @@
   return AndroidSysRootPath;
   }
 
-  if (getTriple().isCSKY()) {
-// CSKY toolchains use different names for sysroot folder.
-if (!GCCInstallation.isValid())
-  return std::string();
-// GCCInstallation.getInstallPath() =
-//   $GCCToolchainPath/lib/gcc/csky-linux-gnuabiv2/6.3.0
-// Path = $GCCToolchainPath/csky-linux-gnuabiv2/libc
-std::string Path = (GCCInstallation.getInstallPath() + "/../../../../" +
-GCCInstallation.getTriple().str() + "/libc")
-   .str();
-if (getVFS().exists(Path))
-  return Path;
-return std::string();
-  }
-
-  if (!GCCInstallation.isValid() || !getTriple().isMIPS())
+  if (!GCCInstallation.isValid())
 return std::string();
 
-  // Standalone MIPS toolchains use different names for sysroot folder
-  // and put it into different places. Here we try to check some known
-  // variants.
-
   const StringRef InstallDir = GCCInstallation.getInstallPath();
   const StringRef TripleStr = GCCInstallation.getTriple().str();
-  const Multilib &Multilib = GCCInstallation.getMultilib();
+  std::string Path = (InstallDir + "/../../../../" + TripleStr).str();
 
-  std::string Path =
-  (InstallDir + "/../../../../" + TripleStr + "/libc" + 
Multilib.osSuffix())
-  .str();
+  // CSKY toolchains use different names for sysroot folder.
+  // GCCInstallation.getInstallPath() =
+  //   $GCCToolchainPath/lib/gcc/csky-linux-gnuabiv2/6.3.0
+  // Path = $GCCToolchainPath/csky-linux-gnuabiv2/libc
+  if (getTriple().isCSKY())
+Path += "/libc";
 
-  if (getVFS().exists(Path))
-return Path;
+  // Standalone MIPS toolchains use different names for sysroot folder
+  // and put it into different places. Here check the two known variants.
+  else if (getTriple().isMIPS()) {
+const std::string &OSSuffix = GCCInstallation.getMultilib().osSuffix();
 
-  Path = (InstallDir + "/../../../../sysroot" + Multilib.osSuffix()).str();
+Path += "/libc" + OSSuffix;
+if (getVFS().exists(Path))
+  return Path;
+
+Path = (InstallDir + "/../../../../sysroot" + OSSuffix).str();
+  }
 
   if (getVFS().exists(Path))
 return Path;


Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -355,40 +355,31 @@
   return AndroidSysRootPath;
   }
 
-  if (getTriple().isCSKY()) {
-// CSKY toolchains use different names for sysroot folder.
-if (!GCCInstallation.isValid())
-  return std::string();
-// GCCInstallation.getInstallPath() =
-//   $GCCToolchainPath/lib/gcc/csky-linux-gnuabiv2/6.3.0
-// Path = $GCCToolchainPath/csky-linux-gnuabiv2/libc
-std::string Path = (GCCInstallation.getInstallPath() + "/../../../../" +
-GCCInstallation.getTriple().str() + "/libc")
-   .str();
-if (getVFS().exists(Path))
-  return Path;
-return std::string();
-  }
-
-  if (!GCCInstallation.isValid() || !getTriple().isMIPS())
+  if (!GCCInstallation.isValid())
 return std::string();
 
-  // Standalone MIPS toolchains use different names for sysroot folder
-  // and put it into different places. Here we try to check some known
-  // variants.
-
   const StringRef InstallDir = GCCInstallation.getInstallPath();
   const StringRef TripleStr = GCCInstallation.getTriple().str();
-  const Multilib &Multilib = GCCInstallation.getMultilib();
+  std::string Path = (InstallDir + "/../../../../" + TripleStr).str();
 
-  std::string Path =
-  (InstallDir + "/../../../../" + TripleStr + "/libc" + Multilib.osSuffix())
-  .str();
+  // CSKY toolchains use different names for sysroot folder.
+  // GCCInstallation.getInstallPath() =
+  //   $GCCToolchainPath/lib/gcc/csky-linux-gnuabiv2/6.3.0
+  // Path = $GCCToolchainPath/csky-linux-gnuabiv2/libc
+  if (getTriple().isCSKY())
+Path += "/libc";
 
-  if (getVFS().exists(Path))
-return Path;
+  // Standalone MIPS toolchains use different names for sysroot folder
+  // and put it into different places. Here check the two known variants.
+  else if (getTriple().isMIPS()) {
+const std::string &OSSuffix = GCCInstallation.getMultilib().osSuffix();
 
-  Path = (InstallDir + "/../../../../sysroot" + Multilib.osSuffix()).str();
+Path += "/libc" + OSSuffix;
+if (getVFS().exists(Path))
+  return Path;
+
+Path = (InstallDir 

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

[Adrian Ratiu via Phabricator via cfe-commits]

10ne1 added a comment.

In D134454#3824391 , @nickdesaulniers 
wrote:

> Looks great! Nice job @10ne1 . Need me to commit this for you?

Yes please, thank you!


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

https://reviews.llvm.org/D134454

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian Ratiu via Phabricator via cfe-commits]

10ne1 added a comment.

In D134454#3824571 , @MaskRay wrote:

> I'll grab an Arch Linux machine for testing, but I don't think this is 
> currently in a form for submitting.
> This adds new functionality for non-MIPS and we need some fake file 
> hierarchies (like those used in `linux-cross.cpp`).
> I'll add the test, though, and submit this for you.
>
> Request changes for now.

Ok. Thanks, please ping if you need any action on my side.


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

https://reviews.llvm.org/D134454

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian Ratiu via Phabricator via cfe-commits]

10ne1 added a comment.

In D134454#3826179 , @MaskRay wrote:

> In D134454#3826143 , @10ne1 wrote:
>
>> In D134454#3824571 , @MaskRay 
>> wrote:
>>
>>> I'll grab an Arch Linux machine for testing, but I don't think this is 
>>> currently in a form for submitting.
>>> This adds new functionality for non-MIPS and we need some fake file 
>>> hierarchies (like those used in `linux-cross.cpp`).
>>> I'll add the test, though, and submit this for you.
>>>
>>> Request changes for now.
>>
>> Ok. Thanks, please ping if you need any action on my side.
>
> My latest thought is that this patch is going toward a wrong direction: 
> https://reviews.llvm.org/D134454#3824630

To verify I understand correctly what you are suggesting: should we close this 
review and wait for distros like Arch to specify the sysroots via the new 
config files mechanism? (Arch is currently at LLVM 14.0.6, not sure when they 
will upgrade).

This will also block the kernel cleanups, but if @nickdesaulniers also agrees 
to postpone that kernel work, then ok, let's wait.


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

https://reviews.llvm.org/D134454

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian Ratiu via Phabricator via cfe-commits]

10ne1 added a comment.

@MaskRay and @nickdesaulniers Can you please work together to reach a consensus 
on what is the best path forward? I am ok either way, just need to know what 
the next steps are. :) Thank you.


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

https://reviews.llvm.org/D134454

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits