[Distro] Bypass distro detection on non-Linux hosts

[aganea]

Summary

This skips distro detection when we're not running on Linux, or when the target triple is not Linux. This saves a few OS calls for each invocation of clang.exe.

[rnk]

I'd rather avoid the ifdef if possible. I looked to see where these come from:

$ git grep -l Distro ../clang/lib/Driver/
../clang/lib/Driver/CMakeLists.txt
../clang/lib/Driver/Distro.cpp
../clang/lib/Driver/ToolChains/Clang.cpp
../clang/lib/Driver/ToolChains/Cuda.cpp
../clang/lib/Driver/ToolChains/Linux.cpp

I think the only one that matters is in Clang.cpp, but it looks like it should never run on Windows:

if (Args.hasFlag(options::OPT_faddrsig, options::OPT_fno_addrsig,
                 (TC.getTriple().isOSBinFormatELF() ||
                  TC.getTriple().isOSBinFormatCOFF()) &&
                    !TC.getTriple().isPS4() &&
                    !TC.getTriple().isOSNetBSD() &&
                    !Distro(D.getVFS()).IsGentoo() &&
                    !TC.getTriple().isAndroid() &&
                     TC.useIntegratedAs()))
  CmdArgs.push_back("-faddrsig");

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.

[aganea]

In 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?

[aganea]

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?

[aganea]

Removed #ifdef _WIN32
Use the target triple where possible to detect the distro. The Cuda installation detector uses the host triple, in order to use the host tooling.
Skip distro detection when the target or host system is not Linux.

[rnk]

In 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, @aganea wrote:

In 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

clang/lib/Driver/Distro.cpp

28

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.

clang/lib/Driver/ToolChains/Linux.cpp

514 ↗

(On Diff #230525)

So many duplicate detections. =/

clang/lib/Driver/Distro.cpp

 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/MemoryBuffer.h"
 
 using namespace clang::driver;
 using namespace clang;
 
 static Distro::DistroType DetectDistro(llvm::vfs::FileSystem &VFS) {
+#ifdef _WIN32
+  IntrusiveRefCntPtr<llvm::vfs::FileSystem> RealFS =
+      llvm::vfs::getRealFileSystem();
+  if (&VFS == RealFS.get())
+    return Distro::UnknownDistro;
+#endif
   llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> File =
       VFS.getBufferForFile("/etc/lsb-release");
   if (File) {
     StringRef Data = File.get()->getBuffer();
     SmallVector<StringRef, 16> Lines;
     Data.split(Lines, "\n");
     Distro::DistroType Version = Distro::UnknownDistro;
     for (StringRef Line : Lines)
       if (Version == Distro::UnknownDistro && Line.startswith("DISTRIB_CODENAME="))
         Version = llvm::StringSwitch<Distro::DistroType>(Line.substr(17))

clang/unittests/Driver/DistroTest.cpp

TEST(DistroTest, DetectGentoo) {
   ASSERT_EQ(Distro(Distro::Gentoo), Gentoo);
   ASSERT_FALSE(Gentoo.IsUbuntu());
   ASSERT_FALSE(Gentoo.IsRedhat());
   ASSERT_FALSE(Gentoo.IsOpenSUSE());
   ASSERT_FALSE(Gentoo.IsDebian());
   ASSERT_TRUE(Gentoo.IsGentoo());
 }
 
+#ifdef _WIN32
+TEST(DistroTest, DetectOnWindows) {
+
+  class CountingFileSystem : public llvm::vfs::ProxyFileSystem {
+  public:
+    CountingFileSystem() : ProxyFileSystem(llvm::vfs::getRealFileSystem()) {}
+
+    llvm::ErrorOr<llvm::vfs::Status> status(const llvm::Twine &Path) override {
+      ++Count;
+      return llvm::vfs::ProxyFileSystem::status(Path);
+    }
+
+    llvm::ErrorOr<std::unique_ptr<llvm::vfs::File>>
+    openFileForRead(const llvm::Twine &Path) override {
+      ++Count;
+      return llvm::vfs::ProxyFileSystem::openFileForRead(Path);
+    }
+
+    unsigned Count{};
+  };
+
+  CountingFileSystem CFileSystem;
+  Distro WinVFS{ CFileSystem };
+  ASSERT_EQ(Distro(Distro::UnknownDistro), WinVFS);
+  ASSERT_GT(CFileSystem.Count, 0U);
+
+  llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> RFS =
+      llvm::vfs::getRealFileSystem();
+  Distro WinRFS{*RFS};
+  ASSERT_EQ(Distro(Distro::UnknownDistro), WinRFS);
+}
+#endif
+
 } // end anonymous namespace