[PATCH] D70467: [Distro] Bypass distro detection on non-Linux hosts

[Reid Kleckner via Phabricator via cfe-commits]

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