[PATCH] D70467: [Distro] Bypass distro detection on Windows

[Alexandre Ganea via Phabricator via cfe-commits]

aganea updated this revision to Diff 230525.
aganea added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70467/new/

https://reviews.llvm.org/D70467

Files:
  clang/include/clang/Driver/Distro.h
  clang/lib/Driver/Distro.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/Cuda.cpp
  clang/lib/Driver/ToolChains/Linux.cpp
  clang/unittests/Driver/DistroTest.cpp

Index: clang/unittests/Driver/DistroTest.cpp
===
--- clang/unittests/Driver/DistroTest.cpp
+++ clang/unittests/Driver/DistroTest.cpp
@@ -44,7 +44,7 @@
"SUPPORT_URL=\"http://help.ubuntu.com/\"\n";
"BUG_REPORT_URL=\"http://bugs.launchpad.net/ubuntu/\"\n";));
 
-  Distro UbuntuTrusty{UbuntuTrustyFileSystem};
+  Distro UbuntuTrusty{UbuntuTrustyFileSystem, llvm::Triple("unknown-pc-linux")};
   ASSERT_EQ(Distro(Distro::UbuntuTrusty), UbuntuTrusty);
   ASSERT_TRUE(UbuntuTrusty.IsUbuntu());
   ASSERT_FALSE(UbuntuTrusty.IsRedhat());
@@ -52,6 +52,9 @@
   ASSERT_FALSE(UbuntuTrusty.IsDebian());
   ASSERT_FALSE(UbuntuTrusty.IsGentoo());
 
+  Distro UbuntuTrusty2{UbuntuTrustyFileSystem, llvm::Triple("unknown-pc-windows")};
+  ASSERT_EQ(Distro(Distro::UnknownDistro), UbuntuTrusty2);
+
   llvm::vfs::InMemoryFileSystem UbuntuYakketyFileSystem;
   UbuntuYakketyFileSystem.addFile("/etc/debian_version", 0,
   llvm::MemoryBuffer::getMemBuffer("stretch/sid\n"));
@@ -74,7 +77,7 @@
"VERSION_CODENAME=yakkety\n"
"UBUNTU_CODENAME=yakkety\n"));
 
-  Distro UbuntuYakkety{UbuntuYakketyFileSystem};
+  Distro UbuntuYakkety{UbuntuYakketyFileSystem, llvm::Triple("unknown-pc-linux")};
   ASSERT_EQ(Distro(Distro::UbuntuYakkety), UbuntuYakkety);
   ASSERT_TRUE(UbuntuYakkety.IsUbuntu());
   ASSERT_FALSE(UbuntuYakkety.IsRedhat());
@@ -109,7 +112,7 @@
"REDHAT_SUPPORT_PRODUCT=\"Fedora\"\n"
"REDHAT_SUPPORT_PRODUCT_VERSION=25\n"
"PRIVACY_POLICY_URL=https://fedoraproject.org/wiki/Legal:PrivacyPolicy\n";));
-  Distro Fedora25{Fedora25FileSystem};
+  Distro Fedora25{Fedora25FileSystem, llvm::Triple("unknown-pc-linux")};
   ASSERT_EQ(Distro(Distro::Fedora), Fedora25);
   ASSERT_FALSE(Fedora25.IsUbuntu());
   ASSERT_TRUE(Fedora25.IsRedhat());
@@ -146,7 +149,7 @@
"REDHAT_SUPPORT_PRODUCT=\"centos\"\n"
"REDHAT_SUPPORT_PRODUCT_VERSION=\"7\"\n"));
 
-  Distro CentOS7{CentOS7FileSystem};
+  Distro CentOS7{CentOS7FileSystem, llvm::Triple("unknown-pc-linux")};
   ASSERT_EQ(Distro(Distro::RHEL7), CentOS7);
   ASSERT_FALSE(CentOS7.IsUbuntu());
   ASSERT_TRUE(CentOS7.IsRedhat());
@@ -174,7 +177,7 @@
"HOME_URL=\"https://opensuse.org/\"\n";
"ID_LIKE=\"suse\"\n"));
 
-  Distro OpenSUSELeap421{OpenSUSELeap421FileSystem};
+  Distro OpenSUSELeap421{OpenSUSELeap421FileSystem, llvm::Triple("unknown-pc-linux")};
   ASSERT_EQ(Distro(Distro::OpenSUSE), OpenSUSELeap421);
   ASSERT_FALSE(OpenSUSELeap421.IsUbuntu());
   ASSERT_FALSE(OpenSUSELeap421.IsRedhat());
@@ -200,7 +203,7 @@
"HOME_URL=\"https://opensuse.org/\"\n";
"ID_LIKE=\"suse\"\n"));
 
-  Distro OpenSUSE132{OpenSUSE132FileSystem};
+  Distro OpenSUSE132{OpenSUSE132FileSystem, llvm::Triple("unknown-pc-linux")};
   ASSERT_EQ(Distro(Distro::OpenSUSE), OpenSUSE132);
   ASSERT_FALSE(OpenSUSE132.IsUbuntu());
   ASSERT_FALSE(OpenSUSE132.IsRedhat());
@@ -217,7 +220,7 @@
   llvm::MemoryBuffer::getMemBuffer("LSB_VERSION=\"core-2.0-noarch:core-3.0-noarch:core-2.0-x86_64:core-3.0-x86_64\"\n"));
 
   // SLES10 is unsupported and therefore evaluates to unknown
-  Distro SLES10{SLES10FileSystem};
+  Distro SLES10{SLES10FileSystem, llvm::Triple("unknown-pc-linux")};
   ASSERT_EQ(Distro(Distro::UnknownDistro), SLES10);
   ASSERT_FALSE(SLES10.IsUbuntu());
   ASSERT_FALSE(SLES10.IsRedhat());
@@ -240,7 +243,7 @@
"SUPPORT_URL=\"http://www.debian.org/support\"\n";
"BUG_REPORT_URL=\"https://bugs.debian.org/\"\n";));
 
-  Distro DebianJessie{DebianJessieFileSystem};
+  Distro DebianJessie{DebianJessieFileSystem, llvm::Triple("unknown-pc-linux")};
   ASSERT_EQ(Distro(Distro::DebianJessie), DebianJessie);
   ASSERT_FALSE(DebianJessie.IsUbuntu());
   ASSERT_FALSE(DebianJessie.IsRedhat());
@@ -259,7 +262,7 @@
"SUPPOR

[PATCH] D70467: [Distro] Bypass distro detection on Windows

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

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? 
Shouldn't it use the target triple, not the host triple? 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`?


Repository:
  rG LLVM Github Monorepo

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

[PATCH] D70467: [Distro] Bypass distro detection on Windows

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

[PATCH] D70467: [Distro] Bypass distro detection on Windows

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

[PATCH] D70467: [Distro] Bypass distro detection on Windows

[Alexandre Ganea via Phabricator via cfe-commits]

aganea created this revision.
aganea added reviewers: sylvestre.ledru, rnk.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This skips distro detection on Windows, thus saving a few `stat` calls for each 
invocation of `clang.exe`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70467

Files:
  clang/lib/Driver/Distro.cpp
  clang/unittests/Driver/DistroTest.cpp


Index: clang/unittests/Driver/DistroTest.cpp
===
--- clang/unittests/Driver/DistroTest.cpp
+++ clang/unittests/Driver/DistroTest.cpp
@@ -337,4 +337,37 @@
   ASSERT_TRUE(Gentoo.IsGentoo());
 }
 
+#ifdef _WIN32
+TEST(DistroTest, DetectOnWindows) {
+
+  class CountingFileSystem : public llvm::vfs::ProxyFileSystem {
+  public:
+CountingFileSystem() : ProxyFileSystem(llvm::vfs::getRealFileSystem()) {}
+
+llvm::ErrorOr status(const llvm::Twine &Path) override {
+  ++Count;
+  return llvm::vfs::ProxyFileSystem::status(Path);
+}
+
+llvm::ErrorOr>
+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 RFS =
+  llvm::vfs::getRealFileSystem();
+  Distro WinRFS{*RFS};
+  ASSERT_EQ(Distro(Distro::UnknownDistro), WinRFS);
+}
+#endif
+
 } // end anonymous namespace
Index: clang/lib/Driver/Distro.cpp
===
--- clang/lib/Driver/Distro.cpp
+++ clang/lib/Driver/Distro.cpp
@@ -18,6 +18,12 @@
 using namespace clang;
 
 static Distro::DistroType DetectDistro(llvm::vfs::FileSystem &VFS) {
+#ifdef _WIN32
+  IntrusiveRefCntPtr RealFS =
+  llvm::vfs::getRealFileSystem();
+  if (&VFS == RealFS.get())
+return Distro::UnknownDistro;
+#endif
   llvm::ErrorOr> File =
   VFS.getBufferForFile("/etc/lsb-release");
   if (File) {


Index: clang/unittests/Driver/DistroTest.cpp
===
--- clang/unittests/Driver/DistroTest.cpp
+++ clang/unittests/Driver/DistroTest.cpp
@@ -337,4 +337,37 @@
   ASSERT_TRUE(Gentoo.IsGentoo());
 }
 
+#ifdef _WIN32
+TEST(DistroTest, DetectOnWindows) {
+
+  class CountingFileSystem : public llvm::vfs::ProxyFileSystem {
+  public:
+CountingFileSystem() : ProxyFileSystem(llvm::vfs::getRealFileSystem()) {}
+
+llvm::ErrorOr status(const llvm::Twine &Path) override {
+  ++Count;
+  return llvm::vfs::ProxyFileSystem::status(Path);
+}
+
+llvm::ErrorOr>
+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 RFS =
+  llvm::vfs::getRealFileSystem();
+  Distro WinRFS{*RFS};
+  ASSERT_EQ(Distro(Distro::UnknownDistro), WinRFS);
+}
+#endif
+
 } // end anonymous namespace
Index: clang/lib/Driver/Distro.cpp
===
--- clang/lib/Driver/Distro.cpp
+++ clang/lib/Driver/Distro.cpp
@@ -18,6 +18,12 @@
 using namespace clang;
 
 static Distro::DistroType DetectDistro(llvm::vfs::FileSystem &VFS) {
+#ifdef _WIN32
+  IntrusiveRefCntPtr RealFS =
+  llvm::vfs::getRealFileSystem();
+  if (&VFS == RealFS.get())
+return Distro::UnknownDistro;
+#endif
   llvm::ErrorOr> File =
   VFS.getBufferForFile("/etc/lsb-release");
   if (File) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits