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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D134454#3844627 , @zixuan-wu wrote:

> It's fine for CSKY to use config file. I only have 2 points.

Thanks. It'll be nice for CSKY to move away from changing `computeSysRoot`.

> 1. I agree sysroot should be separated from GCC because sysroot is not 
> dependent to GCC and there is even not gcc when we use llvm runtime. This 
> rule should also apply to multilib logic. Sysroot can detect multilib path 
> individually.
> 2. From user view, it's not easy to add sysroot or config file path manually, 
> so it needs good way to enable this functionality.

There is a good summary on the recent configuration file improvements: 
https://blogs.gentoo.org/mgorny/2022/10/07/clang-in-gentoo-now-sets-default-runtimes-via-config-file/
The proper way is to let the system GCC or a similar package to provide a Clang 
configuration file.


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

[Zixuan Wu via Phabricator via cfe-commits]

zixuan-wu added a comment.

It's fine for CSKY to use config file. I only have 2 points.

1. I agree sysroot should be separated from GCC because sysroot is not 
dependent to GCC and there is even not gcc when we use llvm runtime. This rule 
should also apply to multilib logic. Sysroot can detect multilib path 
individually.
2. From user view, it's not easy to add sysroot or config file path manually, 
so it needs good way to enable this functionality.


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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

- This patch should be abandoned. Inferring sysroot from gcc-toolchain is 
backward.
- The D644  (`Support Sourcery CodeBench MIPS 
toolchain`) `computeSysRoot` should not be used by new support.
- `--gcc-toolchain=` is discouraged.
- New cross compilation support should specify `--target=` with `--sysroot=` 
and possibly `--gcc-install-dir=`.
- If a distro wants to just use `--target=`, provide a configuration file.
- Gentoo folks are working on making 
https://github.com/llvm/llvm-project/issues/57570 less hacky (and eventually 
the bespoke gcc-config logic).

The following fragment in kernel `tools/scripts/Makefile.include` looks right 
to me. I don't think it is a workaround. In the future `--gcc-toolchain=` 
should be replaced with `--gcc-install-dir=`

  else ifneq ($(CROSS_COMPILE),)
  # Allow userspace to override CLANG_CROSS_FLAGS to specify their own
  # sysroots and flags or to avoid the GCC call in pure Clang builds.
  ifeq ($(CLANG_CROSS_FLAGS),)
  CLANG_CROSS_FLAGS := --target=$(notdir $(CROSS_COMPILE:%-=%))
  GCC_TOOLCHAIN_DIR := $(dir $(shell which $(CROSS_COMPILE)gcc 2>/dev/null))
  ifneq ($(GCC_TOOLCHAIN_DIR),)
  CLANG_CROSS_FLAGS += --prefix=$(GCC_TOOLCHAIN_DIR)$(notdir $(CROSS_COMPILE))
  CLANG_CROSS_FLAGS += --sysroot=$(shell $(CROSS_COMPILE)gcc -print-sysroot)
  CLANG_CROSS_FLAGS += --gcc-toolchain=$(realpath $(GCC_TOOLCHAIN_DIR)/..)
  endif # GCC_TOOLCHAIN_DIR
  endif # CLANG_CROSS_FLAGS
  CFLAGS += $(CLANG_CROSS_FLAGS)
  AFLAGS += $(CLANG_CROSS_FLAGS)
  endif # CROSS_COMPILE


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

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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

> While cross-compiling Linux kernel tools with Clang on ArchLinux, it was 
> revealed the sysroot is 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 
> during Clang invocation.

I disagree that [1] and [2] are workarounds. Figuring out the sysroot and GCC 
installation directory is the canonical and bullet-proof way to let Clang reuse 
the glibc/GCC files.
It works for system cross GCC packages as well as user-built cross GCC 
packages, regardless how sysroot is configured.
Other approaches require file hierarchy detection which is more or less 
brittle. Some is unavoidable as we want Clang to be a drop-in replacement of 
GCC.
Unfortunately traditionally we did not set a clear bar and many extended system 
packages are supported, e.g. the strange mips distribution detection, 
gentoo-config, RedHat devtools, etc.
Adding such extended support just doesn't scale. Thankfully the main branch has 
now a fullblown configuration file support (probably with some rough edges) and 
they can be easily
extended to support a new distribution's bespoke GCC customization.

For the Arch Linux example, what if another distribution claims that they use 
`/usr/aarch64-linux-gnu/sysroot` and suit them better?
What if it is another directory?

Clang Driver detects a GCC installation in the sysroot, then it is backward to 
change the sysroot after a GCC installation is found.
Worse, `computeSysroot` result does not completely replace reading the stored 
`Sysroot` variable.
Some places (e.g. -isysroot, --print-search-dirs) use the `Sysroot` variable.
I am inclined to think that they are correct and what we actually need to fix 
are mips/Android/CSKY in `Linux::computeSysRoot`.


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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

In 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  in 2018) and csky (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  = 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

___
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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

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


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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

Another opinion is whether we actually need the more and more complex sysroot 
computation logic. 
https://reviews.llvm.org/D134337 will soon land and we can move to require 
distributions to provide a config file instead.

Yes, I see mips and csky special cases. To be fair, if I saw CSKY (D121445 
) earlier, I'd raise my concern 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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/lib/Driver/ToolChains/Linux.cpp:369
+  // Path = $GCCToolchainPath/csky-linux-gnuabiv2/libc
+  if (getTriple().isCSKY())
+Path += "/libc";

If the `else` branch uses braces, the `then` branch needs brances as well.
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

I can fix this.


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

[Fangrui Song via Phabricator via cfe-commits]

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

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.

Request changes for now.


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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

Since this decreases complexity, it's probably fine. But we really need some 
tests in clang/test/Driver see linux-cross.cpp


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#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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers accepted this revision.
nickdesaulniers added a comment.
This revision is now accepted and ready to land.

Looks great! Nice job @10ne1 . Need me to commit this for 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 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  = 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  = 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  = 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  = GCCInstallation.getMultilib().osSuffix();
 
-  Path = (InstallDir + "/../../../../sysroot" + Multilib.osSuffix()).str();
+Path += "/libc" + OSSuffix;
+if (getVFS().exists(Path))
+  return Path;
+
+Path = (InstallDir + "/../../../../sysroot" + 

[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  = 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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/Driver/ToolChains/Linux.cpp:376
+  if (getTriple().isMIPS()) {
+const Multilib  = GCCInstallation.getMultilib();
+Path = Path + "/libc" + Multilib.osSuffix();

nickdesaulniers wrote:
> It might be worthwhile to have the variable be
> ```
> std::string OSSuffix = GCCInstallation.getMultilib().osSuffix();
> ```
> rather than the `Multilib` object.
err... I guess a `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

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/Driver/ToolChains/Linux.cpp:363
   const StringRef TripleStr = GCCInstallation.getTriple().str();
-  const Multilib  = GCCInstallation.getMultilib();
+  std::string Path = (InstallDir + "/../../../../" + TripleStr).str();
 

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



Comment at: clang/lib/Driver/ToolChains/Linux.cpp:369-375
+  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()) {

CSKY and MIPS should be mutually exclusive. Please make this second `if` an 
`else if`.

```
if csky:
  ...
elif mips:
  ...
```



Comment at: clang/lib/Driver/ToolChains/Linux.cpp:370
+  if (getTriple().isCSKY())
+Path = Path + "/libc";
 

+=



Comment at: clang/lib/Driver/ToolChains/Linux.cpp:373
+  // Standalone MIPS toolchains use different names for sysroot folder
+  // and put it into different places. Here we try to check some known
+  // variants.

Specifically there's 2 known variants for mips, it looks like. Please update 
this comment.



Comment at: clang/lib/Driver/ToolChains/Linux.cpp:376
+  if (getTriple().isMIPS()) {
+const Multilib  = GCCInstallation.getMultilib();
+Path = Path + "/libc" + Multilib.osSuffix();

It might be worthwhile to have the variable be
```
std::string OSSuffix = GCCInstallation.getMultilib().osSuffix();
```
rather than the `Multilib` object.



Comment at: clang/lib/Driver/ToolChains/Linux.cpp:377
+const Multilib  = GCCInstallation.getMultilib();
+Path = Path + "/libc" + Multilib.osSuffix();
+if (getVFS().exists(Path))

+=



Comment at: clang/lib/Driver/ToolChains/Linux.cpp:377-389
   const StringRef InstallDir = GCCInstallation.getInstallPath();
   const StringRef TripleStr = GCCInstallation.getTriple().str();
   const 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;

10ne1 wrote:
> 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();
> ```
Yes, this is much nicer in that we don't have to special case arch-linux.  
Sometimes refactorings beget more refactorings; it's nice when that happens.


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  = 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  = 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  = 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  = GCCInstallation.getMultilib();
+Path = Path + "/libc" + Multilib.osSuffix();
+if (getVFS().exists(Path))
+  return Path;
 
-  Path = (InstallDir + "/../../../../sysroot" + 

[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  = 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

[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  = 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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[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  = 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  = 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 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  = 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  = 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