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

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

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

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

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

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

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

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/

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

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

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

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 >

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

10ne1 added a comment. In D134454#3826179 , @MaskRay wrote: > In D134454#3826143 , @10ne1 wrote: > >> In D134454#3824571 , @MaskRay >> wrote: >> >>> I'll grab an Arch

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

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

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

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 >

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

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

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

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

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

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

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

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

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

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/

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

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

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

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

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

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 +

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

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

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

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 + "/../../../../" +

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

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:

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

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

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

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

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

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:

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

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/

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

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