nickdesaulniers added a comment. In D134454#3824630 <https://reviews.llvm.org/D134454#3824630>, @MaskRay wrote:
> Another opinion is whether we actually need the more and more complex sysroot > computation logic. > Inferring sysroot from GCCInstallation looks backward to me. We should get > sysroot first, then compute GCCInstallation, not the order as implemented in > this patch. > > Yes, I see mips > (https://reviews.llvm.org/rG08450bd55ccdc4aee4f5f73cde97e25b3c4ce5b9 2013), > Android (D45291 <https://reviews.llvm.org/D45291> in 2018) and csky (D121445 > <https://reviews.llvm.org/D121445>) special cases. To be fair, if I saw them > earlier, I'd raise my concerns as well. > > --- > > https://reviews.llvm.org/D134337 (default configuration file) will soon land > and we can move to require distributions to provide a config file instead. I disagree with the assessment that this patch makes sysroot detection //more// complex; it is a simplification and bug fix. Detecting the sysroot first, then GCCInstallation is a higher risk change than this patch. The default configuration file is interesting, but it's not clear what even arch-linux would put in their configuration file; this patch cleans up what looks like a bug to me introduced in https://reviews.llvm.org/rG08450bd55ccdc4aee4f5f73cde97e25b3c4ce5b9 that folks have been working around downstream for quite some time. --- I think @10ne1 just needs to fix up the style nit, and a test might be nice. clang/test/Driver/linux-cross.cpp already has a test using a "fake" Arch-Linux tree in clang/test/Driver/Inputs/archlinux_i686_tree/. A new "fake" tree could be added to emulate the current way arch lays out these sysroots. ================ Comment at: clang/lib/Driver/ToolChains/Linux.cpp:375 + else if (getTriple().isMIPS()) { + const std::string &OSSuffix = GCCInstallation.getMultilib().osSuffix(); ---------------- clang/lib/Driver/ToolChains/Linux.cpp `Linux::Linux` has: ``` if (IsCSKY) SysRoot = SysRoot + SelectedMultilib.osSuffix(); ``` lol. So we should either do this for BOTH mips and csky here, or there. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134454/new/ https://reviews.llvm.org/D134454 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
