rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land.
In D70467#1755364 <https://reviews.llvm.org/D70467#1755364>, @aganea wrote: > Actually, I'm not sure the `DetectDistro()` does what it intends to do when > cross-compiling: if I compile on Ubuntu and I forcibly specify `-target > x86_64-linux` on the cmd-line, should it detect which Linux distro I'm on? In > the case of `CudaInstallationDetector()` it seems it should use the host > triple, whereas in the case `clang/lib/Driver/ToolChains/Linux.cpp:Linux()` > it adds flags for building the target, so it should take the target triple > maybe. Should we pass a triple to `DetectDistro`? It does seem wrong to use DetectDistro when the user passes a target triple. DetectDistro inherently detects something about that host, and it seems we don't have a way to pass that information explicitly on the command line. So, cross-compiling from one distro to another might somehow do the wrong thing depending on what distro detection does. In D70467#1755337 <https://reviews.llvm.org/D70467#1755337>, @aganea wrote: > In D70467#1752611 <https://reviews.llvm.org/D70467#1752611>, @rnk wrote: > > > Hm, I guess it does happen. I think that condition should be restructured > > to only do all that BSD, PS4, Android, Gentoo etc logic if the format is > > ELF, if COFF, then always default to -faddrsig. > > > We're cross-compiling all our target platforms on Windows. It seems this > patch would be still valid in that case: > > > clang -target x86_64-linux a.cpp > > > Would still call `DetectDistro()`, and unless I'm missing something, we > wouldn't want it to lookup `C:\etc\lsb-release` even if it's there? Or people > are really doing that to target a specific distro when compiling from > Windows? It doesn't make sense from my POV. > The same argument applies to CUDA: the code in > `clang/lib/Driver/ToolChains/Cuda.cpp:CudaInstallationDetector()` says > "HostTriple" but it's actually the Target triple -- you can try my cmd-line > above, it hits the Distro detect. > Unless you specify a non-real FS, I wouldn't want `DetectDistro` to return > anything else than `Distro::UnknownDistro` when running on a non-Linux OS. > Would you have a different opinion or something I don't see? True, that makes sense to me. lgtm ================ Comment at: clang/lib/Driver/Distro.cpp:29 + // If the host is not running Linux, and we're backed by a real file system, + // no need to check the distro. + IntrusiveRefCntPtr<llvm::vfs::FileSystem> RealFS = ---------------- I think it's worth adding more to this comment. This is the case where someone is cross-compiling from BSD or Windows to Linux, and it would be meaningless to try to figure out the "distro" of the non-Linux host. ================ Comment at: clang/lib/Driver/ToolChains/Linux.cpp:514 - const Distro Distro(getDriver().getVFS()); + const Distro Distro(getDriver().getVFS(), Triple); ---------------- So many duplicate detections. =/ CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70467/new/ https://reviews.llvm.org/D70467 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits