subject:"\[clang\] \[llvm\] \[clang\]\[deps\] Overload `Filesystem\:\:exists` in `DependencyScanningFilesystem` to have it use cached `status` \(PR #88152\)"

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-12 Thread Artem Chikin via cfe-commits


artemcm wrote:

> Clang test now looks good to me. Might be nice to drop 
> `InstrumentingInMemoryFilesystem` in favor of the existing 
> `InstrumentingFilesystem` (that I added just moments ago, sorry!) wrapped 
> around a normal `InMemoryFileSystem`, but I'm happy to do that myself in a 
> follow-up commit.

I had to update the commit anyway. Unified on `InstrumentingFilesystem`. 

https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-12 Thread Artem Chikin via cfe-commits


https://github.com/artemcm updated 
https://github.com/llvm/llvm-project/pull/88152

>From 8164aaf2e93dde4761789e6d574088d1cb0e97fc Mon Sep 17 00:00:00 2001
From: Artem Chikin 
Date: Tue, 9 Apr 2024 09:37:09 -0700
Subject: [PATCH] [clang][deps] Overload 'Filesystem::exists' in
 'DependencyScanningFilesystem' to have it use cached `status`

As-is, calls to `exists()` fallback on the implementation in 
'ProxyFileSystem::exists' which explicitly calls out to the underlying `FS`, 
which for the 'DependencyScanningFilesystem' (overlay) is the real underlying 
filesystem.

Instead, directly overloading 'exists' allows us to have it rely on the cached 
`status` behavior used elsewhere by the 'DependencyScanningFilesystem'.
---
 .../DependencyScanningFilesystem.h|   4 +
 .../DependencyScanningFilesystem.cpp  |  11 ++
 .../DependencyScanningFilesystemTest.cpp  |  53 +--
 .../Support/VirtualFileSystemTest.cpp | 140 +-
 4 files changed, 125 insertions(+), 83 deletions(-)

diff --git 
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h 
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
index 8b6f149c7cb266..f7b4510d7f7beb 100644
--- 
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ 
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -365,6 +365,10 @@ class DependencyScanningWorkerFilesystem
   /// false if not (i.e. this entry is not a file or its scan fails).
   bool ensureDirectiveTokensArePopulated(EntryRef Entry);
 
+  /// Check whether \p Path exists. By default checks cached result of \c
+  /// status(), and falls back on FS if unable to do so.
+  bool exists(const Twine &Path) override;
+
 private:
   /// For a filename that's not yet associated with any entry in the caches,
   /// uses the underlying filesystem to either look up the entry based in the
diff --git 
a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp 
b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
index 84185c931b552c..0cab17a3424406 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -300,6 +300,17 @@ DependencyScanningWorkerFilesystem::status(const Twine 
&Path) {
   return Result->getStatus();
 }
 
+bool DependencyScanningWorkerFilesystem::exists(const Twine &Path) {
+  // While some VFS overlay filesystems may implement more-efficient
+  // mechanisms for `exists` queries, `DependencyScanningWorkerFilesystem`
+  // typically wraps `RealFileSystem` which does not specialize `exists`,
+  // so it is not likely to benefit from such optimizations. Instead,
+  // it is more-valuable to have this query go through the
+  // cached-`status` code-path of the `DependencyScanningWorkerFilesystem`.
+  llvm::ErrorOr Status = status(Path);
+  return Status && Status->exists();
+}
+
 namespace {
 
 /// The VFS that is used by clang consumes the \c CachedFileSystemEntry using
diff --git 
a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
 
b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
index 697b7d70ff035a..256dd980b2118f 100644
--- 
a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
+++ 
b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
@@ -8,6 +8,7 @@
 
 #include "clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/Twine.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "gtest/gtest.h"
 
@@ -15,31 +16,36 @@ using namespace clang::tooling::dependencies;
 
 namespace {
 struct InstrumentingFilesystem
-: llvm::RTTIExtends {
+: llvm::RTTIExtends {
   unsigned NumStatusCalls = 0;
   unsigned NumGetRealPathCalls = 0;
+  unsigned NumExistsCalls = 0;
 
   using llvm::RTTIExtends::RTTIExtends;
+  llvm::vfs::InMemoryFileSystem>::RTTIExtends;
 
   llvm::ErrorOr status(const llvm::Twine &Path) override {
 ++NumStatusCalls;
-return ProxyFileSystem::status(Path);
+return InMemoryFileSystem::status(Path);
   }
 
   std::error_code getRealPath(const llvm::Twine &Path,
   llvm::SmallVectorImpl &Output) override {
 ++NumGetRealPathCalls;
-return ProxyFileSystem::getRealPath(Path, Output);
+return InMemoryFileSystem::getRealPath(Path, Output);
+  }
+
+  bool exists(const llvm::Twine &Path) override {
+++NumExistsCalls;
+return InMemoryFileSystem::exists(Path);
   }
 };
+
 } // namespace
 
 TEST(DependencyScanningWorkerFilesystem, CacheStatusFailures) {
-  auto InMemoryFS = llvm::makeIntrusiveRefCnt();
-
   auto InstrumentingFS =
-  llvm::makeIntrusiveRefCnt(InMemoryFS);
+  llvm::makeIntrusiveRefCnt();
 
   DependencyScanningFilesystemSharedCache Sha

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-12 Thread Artem Chikin via cfe-commits


https://github.com/artemcm updated 
https://github.com/llvm/llvm-project/pull/88152

>From e010a761a34692e5ca1959fc9022d0950e669664 Mon Sep 17 00:00:00 2001
From: Artem Chikin 
Date: Tue, 9 Apr 2024 09:37:09 -0700
Subject: [PATCH] [clang][deps] Overload 'Filesystem::exists' in
 'DependencyScanningFilesystem' to have it use cached `status`

As-is, calls to `exists()` fallback on the implementation in 
'ProxyFileSystem::exists' which explicitly calls out to the underlying `FS`, 
which for the 'DependencyScanningFilesystem' (overlay) is the real underlying 
filesystem.

Instead, directly overloading 'exists' allows us to have it rely on the cached 
`status` behavior used elsewhere by the 'DependencyScanningFilesystem'.
---
 .../DependencyScanningFilesystem.h|   4 +
 .../DependencyScanningFilesystem.cpp  |  11 ++
 .../DependencyScanningFilesystemTest.cpp  |  45 ++
 .../Support/VirtualFileSystemTest.cpp | 140 +-
 4 files changed, 130 insertions(+), 70 deletions(-)

diff --git 
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h 
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
index 8b6f149c7cb266..f7b4510d7f7beb 100644
--- 
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ 
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -365,6 +365,10 @@ class DependencyScanningWorkerFilesystem
   /// false if not (i.e. this entry is not a file or its scan fails).
   bool ensureDirectiveTokensArePopulated(EntryRef Entry);
 
+  /// Check whether \p Path exists. By default checks cached result of \c
+  /// status(), and falls back on FS if unable to do so.
+  bool exists(const Twine &Path) override;
+
 private:
   /// For a filename that's not yet associated with any entry in the caches,
   /// uses the underlying filesystem to either look up the entry based in the
diff --git 
a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp 
b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
index 84185c931b552c..0cab17a3424406 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -300,6 +300,17 @@ DependencyScanningWorkerFilesystem::status(const Twine 
&Path) {
   return Result->getStatus();
 }
 
+bool DependencyScanningWorkerFilesystem::exists(const Twine &Path) {
+  // While some VFS overlay filesystems may implement more-efficient
+  // mechanisms for `exists` queries, `DependencyScanningWorkerFilesystem`
+  // typically wraps `RealFileSystem` which does not specialize `exists`,
+  // so it is not likely to benefit from such optimizations. Instead,
+  // it is more-valuable to have this query go through the
+  // cached-`status` code-path of the `DependencyScanningWorkerFilesystem`.
+  llvm::ErrorOr Status = status(Path);
+  return Status && Status->exists();
+}
+
 namespace {
 
 /// The VFS that is used by clang consumes the \c CachedFileSystemEntry using
diff --git 
a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
 
b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
index 697b7d70ff035a..ccc02b7b2f09dd 100644
--- 
a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
+++ 
b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
@@ -8,6 +8,7 @@
 
 #include "clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/Twine.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "gtest/gtest.h"
 
@@ -33,6 +34,27 @@ struct InstrumentingFilesystem
 return ProxyFileSystem::getRealPath(Path, Output);
   }
 };
+
+struct InstrumentingInMemoryFilesystem
+: llvm::RTTIExtends {
+  unsigned NumStatCalls = 0;
+  unsigned NumExistsCalls = 0;
+
+  using llvm::RTTIExtends::RTTIExtends;
+
+  llvm::ErrorOr status(const llvm::Twine &Path) override {
+++NumStatCalls;
+return InMemoryFileSystem::status(Path);
+  }
+
+  bool exists(const llvm::Twine &Path) override {
+++NumExistsCalls;
+return InMemoryFileSystem::exists(Path);
+  }
+};
+
 } // namespace
 
 TEST(DependencyScanningWorkerFilesystem, CacheStatusFailures) {
@@ -147,3 +169,26 @@ TEST(DependencyScanningFilesystem, 
RealPathAndStatusInvariants) {
 DepFS.status("/bar");
   }
 }
+
+TEST(DependencyScanningFilesystem, CacheStatOnExists) {
+  auto InMemoryInstrumentingFS =
+  llvm::makeIntrusiveRefCnt();
+  InMemoryInstrumentingFS->setCurrentWorkingDirectory("/");
+  InMemoryInstrumentingFS->addFile("/foo", 0,
+   llvm::MemoryBuffer::getMemBuffer(""));
+  InMemoryInstrumentingFS->addFile("/bar", 0,
+   llvm::MemoryBuffer::getMemBuffer(""));
+  DependencyScanningFilesystemSharedCache SharedCache;
+  DependencyS

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-12 Thread Jan Svoboda via cfe-commits


jansvoboda11 wrote:

Clang test now looks good to me. Might be nice to drop 
`InstrumentingInMemoryFilesystem` in favor of the existing 
`InstrumentingFilesystem` (that I added just moments ago, sorry!) wrapped 
around a normal `InMemoryFileSystem`, but I'm happy to do that myself in a 
follow-up commit.

https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-12 Thread Jan Svoboda via cfe-commits





jansvoboda11 wrote:

I assume clang-format was overly eager with this one, as these are just 
whitespace changes. Can we undo that?

https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-12 Thread Artem Chikin via cfe-commits


https://github.com/artemcm updated 
https://github.com/llvm/llvm-project/pull/88152

>From 24e869df273b9d75bb4fdf85f4ee8ab2ddbccc2c Mon Sep 17 00:00:00 2001
From: Artem Chikin 
Date: Tue, 9 Apr 2024 09:37:09 -0700
Subject: [PATCH] [clang][deps] Overload 'Filesystem::exists' in
 'DependencyScanningFilesystem' to have it use cached `status`

As-is, calls to `exists()` fallback on the implementation in 
'ProxyFileSystem::exists' which explicitly calls out to the underlying `FS`, 
which for the 'DependencyScanningFilesystem' (overlay) is the real underlying 
filesystem.

Instead, directly overloading 'exists' allows us to have it rely on the cached 
`status` behavior used elsewhere by the 'DependencyScanningFilesystem'.
---
 .../DependencyScanningFilesystem.h|   4 +
 .../DependencyScanningFilesystem.cpp  |  11 ++
 .../DependencyScanningFilesystemTest.cpp  |  46 ++
 .../Support/VirtualFileSystemTest.cpp | 140 +-
 4 files changed, 131 insertions(+), 70 deletions(-)

diff --git 
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h 
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
index 8b6f149c7cb266..f7b4510d7f7beb 100644
--- 
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ 
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -365,6 +365,10 @@ class DependencyScanningWorkerFilesystem
   /// false if not (i.e. this entry is not a file or its scan fails).
   bool ensureDirectiveTokensArePopulated(EntryRef Entry);
 
+  /// Check whether \p Path exists. By default checks cached result of \c
+  /// status(), and falls back on FS if unable to do so.
+  bool exists(const Twine &Path) override;
+
 private:
   /// For a filename that's not yet associated with any entry in the caches,
   /// uses the underlying filesystem to either look up the entry based in the
diff --git 
a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp 
b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
index 84185c931b552c..0cab17a3424406 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -300,6 +300,17 @@ DependencyScanningWorkerFilesystem::status(const Twine 
&Path) {
   return Result->getStatus();
 }
 
+bool DependencyScanningWorkerFilesystem::exists(const Twine &Path) {
+  // While some VFS overlay filesystems may implement more-efficient
+  // mechanisms for `exists` queries, `DependencyScanningWorkerFilesystem`
+  // typically wraps `RealFileSystem` which does not specialize `exists`,
+  // so it is not likely to benefit from such optimizations. Instead,
+  // it is more-valuable to have this query go through the
+  // cached-`status` code-path of the `DependencyScanningWorkerFilesystem`.
+  llvm::ErrorOr Status = status(Path);
+  return Status && Status->exists();
+}
+
 namespace {
 
 /// The VFS that is used by clang consumes the \c CachedFileSystemEntry using
diff --git 
a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
 
b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
index 697b7d70ff035a..67f9ee1a3f97ce 100644
--- 
a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
+++ 
b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
@@ -8,6 +8,7 @@
 
 #include "clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/Twine.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "gtest/gtest.h"
 
@@ -33,6 +34,28 @@ struct InstrumentingFilesystem
 return ProxyFileSystem::getRealPath(Path, Output);
   }
 };
+
+
+struct InstrumentingInMemoryFilesystem
+: llvm::RTTIExtends {
+  unsigned NumStatCalls = 0;
+  unsigned NumExistsCalls = 0;
+
+  using llvm::RTTIExtends::RTTIExtends;
+
+  llvm::ErrorOr status(const llvm::Twine &Path) override {
+++NumStatCalls;
+return InMemoryFileSystem::status(Path);
+  }
+
+  bool exists(const llvm::Twine &Path) override {
+++NumExistsCalls;
+return InMemoryFileSystem::exists(Path);
+  }
+};
+
 } // namespace
 
 TEST(DependencyScanningWorkerFilesystem, CacheStatusFailures) {
@@ -147,3 +170,26 @@ TEST(DependencyScanningFilesystem, 
RealPathAndStatusInvariants) {
 DepFS.status("/bar");
   }
 }
+
+TEST(DependencyScanningFilesystem, CacheStatOnExists) {
+  auto InMemoryInstrumentingFS =
+  llvm::makeIntrusiveRefCnt();
+  InMemoryInstrumentingFS->setCurrentWorkingDirectory("/");
+  InMemoryInstrumentingFS->addFile("/foo", 0,
+   llvm::MemoryBuffer::getMemBuffer(""));
+  InMemoryInstrumentingFS->addFile("/bar", 0,
+   llvm::MemoryBuffer::getMemBuffer(""));
+  DependencyScanningFilesystemSharedCache SharedCache;
+  Dependenc

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-12 Thread Artem Chikin via cfe-commits


https://github.com/artemcm updated 
https://github.com/llvm/llvm-project/pull/88152

>From 12621dc6344335375ac1a5c806d778f1ed15a5ce Mon Sep 17 00:00:00 2001
From: Ben Langmuir 
Date: Tue, 9 Apr 2024 11:22:44 -0700
Subject: [PATCH 1/2] [llvm][vfs] Make vfs::FileSystem::exists() virtual NFC

Allow a vfs::FileSystem to provide a more efficient implementation of
exists() if they are able to. The existing FileSystem implementations
continue to default to using status() except that overlay, proxy, and
redirecting filesystems are taught to forward calls to exists()
correctly to their wrapped/external filesystem.
---
 llvm/include/llvm/Support/VirtualFileSystem.h |   8 +-
 llvm/lib/Support/VirtualFileSystem.cpp|  56 ++
 .../Support/VirtualFileSystemTest.cpp | 165 ++
 3 files changed, 227 insertions(+), 2 deletions(-)

diff --git a/llvm/include/llvm/Support/VirtualFileSystem.h 
b/llvm/include/llvm/Support/VirtualFileSystem.h
index 4b1ca0c3d262b6..0113d6b7da25d3 100644
--- a/llvm/include/llvm/Support/VirtualFileSystem.h
+++ b/llvm/include/llvm/Support/VirtualFileSystem.h
@@ -300,8 +300,9 @@ class FileSystem : public 
llvm::ThreadSafeRefCountedBase,
   virtual std::error_code getRealPath(const Twine &Path,
   SmallVectorImpl &Output);
 
-  /// Check whether a file exists. Provided for convenience.
-  bool exists(const Twine &Path);
+  /// Check whether \p Path exists. By default this uses \c status(), but
+  /// filesystems may provide a more efficient implementation if available.
+  virtual bool exists(const Twine &Path);
 
   /// Is the file mounted on a local filesystem?
   virtual std::error_code isLocal(const Twine &Path, bool &Result);
@@ -386,6 +387,7 @@ class OverlayFileSystem : public 
RTTIExtends {
   void pushOverlay(IntrusiveRefCntPtr FS);
 
   llvm::ErrorOr status(const Twine &Path) override;
+  bool exists(const Twine &Path) override;
   llvm::ErrorOr>
   openFileForRead(const Twine &Path) override;
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
@@ -439,6 +441,7 @@ class ProxyFileSystem : public RTTIExtends {
   llvm::ErrorOr status(const Twine &Path) override {
 return FS->status(Path);
   }
+  bool exists(const Twine &Path) override { return FS->exists(Path); }
   llvm::ErrorOr>
   openFileForRead(const Twine &Path) override {
 return FS->openFileForRead(Path);
@@ -1044,6 +1047,7 @@ class RedirectingFileSystem
  bool UseExternalNames, FileSystem &ExternalFS);
 
   ErrorOr status(const Twine &Path) override;
+  bool exists(const Twine &Path) override;
   ErrorOr> openFileForRead(const Twine &Path) override;
 
   std::error_code getRealPath(const Twine &Path,
diff --git a/llvm/lib/Support/VirtualFileSystem.cpp 
b/llvm/lib/Support/VirtualFileSystem.cpp
index 32b480028e71b4..921af30bfcde9f 100644
--- a/llvm/lib/Support/VirtualFileSystem.cpp
+++ b/llvm/lib/Support/VirtualFileSystem.cpp
@@ -438,6 +438,15 @@ ErrorOr OverlayFileSystem::status(const Twine 
&Path) {
   return make_error_code(llvm::errc::no_such_file_or_directory);
 }
 
+bool OverlayFileSystem::exists(const Twine &Path) {
+  // FIXME: handle symlinks that cross file systems
+  for (iterator I = overlays_begin(), E = overlays_end(); I != E; ++I) {
+if ((*I)->exists(Path))
+  return true;
+  }
+  return false;
+}
+
 ErrorOr>
 OverlayFileSystem::openFileForRead(const llvm::Twine &Path) {
   // FIXME: handle symlinks that cross file systems
@@ -2428,6 +2437,53 @@ ErrorOr RedirectingFileSystem::status(const 
Twine &OriginalPath) {
   return S;
 }
 
+bool RedirectingFileSystem::exists(const Twine &OriginalPath) {
+  SmallString<256> Path;
+  OriginalPath.toVector(Path);
+
+  if (makeAbsolute(Path))
+return false;
+
+  if (Redirection == RedirectKind::Fallback) {
+// Attempt to find the original file first, only falling back to the
+// mapped file if that fails.
+if (ExternalFS->exists(Path))
+  return true;
+  }
+
+  ErrorOr Result = lookupPath(Path);
+  if (!Result) {
+// Was not able to map file, fallthrough to using the original path if
+// that was the specified redirection type.
+if (Redirection == RedirectKind::Fallthrough &&
+isFileNotFound(Result.getError()))
+  return ExternalFS->exists(Path);
+return false;
+  }
+
+  std::optional ExtRedirect = Result->getExternalRedirect();
+  if (!ExtRedirect) {
+assert(isa(Result->E));
+return true;
+  }
+
+  SmallString<256> RemappedPath((*ExtRedirect).str());
+  if (makeAbsolute(RemappedPath))
+return false;
+
+  if (ExternalFS->exists(RemappedPath))
+return true;
+
+  if (Redirection == RedirectKind::Fallthrough) {
+// Mapped the file but it wasn't found in the underlying filesystem,
+// fallthrough to using the original path if that was the specified
+// redirection type.
+return ExternalFS->exists(Path);
+  }
+
+  return false;
+}
+
 namespace {
 
 /// Provide a file wrapper with an overriden

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-12 Thread Artem Chikin via cfe-commits


https://github.com/artemcm updated 
https://github.com/llvm/llvm-project/pull/88152

>From c2b2cd03e1d8f92b1df814e6312158b8820b790d Mon Sep 17 00:00:00 2001
From: Ben Langmuir 
Date: Tue, 9 Apr 2024 11:22:44 -0700
Subject: [PATCH 1/2] [llvm][vfs] Make vfs::FileSystem::exists() virtual NFC

Allow a vfs::FileSystem to provide a more efficient implementation of
exists() if they are able to. The existing FileSystem implementations
continue to default to using status() except that overlay, proxy, and
redirecting filesystems are taught to forward calls to exists()
correctly to their wrapped/external filesystem.
---
 llvm/include/llvm/Support/VirtualFileSystem.h |   8 +-
 llvm/lib/Support/VirtualFileSystem.cpp|  56 ++
 .../Support/VirtualFileSystemTest.cpp | 165 ++
 3 files changed, 227 insertions(+), 2 deletions(-)

diff --git a/llvm/include/llvm/Support/VirtualFileSystem.h 
b/llvm/include/llvm/Support/VirtualFileSystem.h
index 770ca8764426a4..bdc98bfd7d2571 100644
--- a/llvm/include/llvm/Support/VirtualFileSystem.h
+++ b/llvm/include/llvm/Support/VirtualFileSystem.h
@@ -300,8 +300,9 @@ class FileSystem : public 
llvm::ThreadSafeRefCountedBase,
   virtual std::error_code getRealPath(const Twine &Path,
   SmallVectorImpl &Output) const;
 
-  /// Check whether a file exists. Provided for convenience.
-  bool exists(const Twine &Path);
+  /// Check whether \p Path exists. By default this uses \c status(), but
+  /// filesystems may provide a more efficient implementation if available.
+  virtual bool exists(const Twine &Path);
 
   /// Is the file mounted on a local filesystem?
   virtual std::error_code isLocal(const Twine &Path, bool &Result);
@@ -386,6 +387,7 @@ class OverlayFileSystem : public 
RTTIExtends {
   void pushOverlay(IntrusiveRefCntPtr FS);
 
   llvm::ErrorOr status(const Twine &Path) override;
+  bool exists(const Twine &Path) override;
   llvm::ErrorOr>
   openFileForRead(const Twine &Path) override;
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
@@ -439,6 +441,7 @@ class ProxyFileSystem : public RTTIExtends {
   llvm::ErrorOr status(const Twine &Path) override {
 return FS->status(Path);
   }
+  bool exists(const Twine &Path) override { return FS->exists(Path); }
   llvm::ErrorOr>
   openFileForRead(const Twine &Path) override {
 return FS->openFileForRead(Path);
@@ -1044,6 +1047,7 @@ class RedirectingFileSystem
  bool UseExternalNames, FileSystem &ExternalFS);
 
   ErrorOr status(const Twine &Path) override;
+  bool exists(const Twine &Path) override;
   ErrorOr> openFileForRead(const Twine &Path) override;
 
   std::error_code getRealPath(const Twine &Path,
diff --git a/llvm/lib/Support/VirtualFileSystem.cpp 
b/llvm/lib/Support/VirtualFileSystem.cpp
index 057f8eae0552c6..272880ba602b84 100644
--- a/llvm/lib/Support/VirtualFileSystem.cpp
+++ b/llvm/lib/Support/VirtualFileSystem.cpp
@@ -439,6 +439,15 @@ ErrorOr OverlayFileSystem::status(const Twine 
&Path) {
   return make_error_code(llvm::errc::no_such_file_or_directory);
 }
 
+bool OverlayFileSystem::exists(const Twine &Path) {
+  // FIXME: handle symlinks that cross file systems
+  for (iterator I = overlays_begin(), E = overlays_end(); I != E; ++I) {
+if ((*I)->exists(Path))
+  return true;
+  }
+  return false;
+}
+
 ErrorOr>
 OverlayFileSystem::openFileForRead(const llvm::Twine &Path) {
   // FIXME: handle symlinks that cross file systems
@@ -2431,6 +2440,53 @@ ErrorOr RedirectingFileSystem::status(const 
Twine &OriginalPath) {
   return S;
 }
 
+bool RedirectingFileSystem::exists(const Twine &OriginalPath) {
+  SmallString<256> Path;
+  OriginalPath.toVector(Path);
+
+  if (makeAbsolute(Path))
+return false;
+
+  if (Redirection == RedirectKind::Fallback) {
+// Attempt to find the original file first, only falling back to the
+// mapped file if that fails.
+if (ExternalFS->exists(Path))
+  return true;
+  }
+
+  ErrorOr Result = lookupPath(Path);
+  if (!Result) {
+// Was not able to map file, fallthrough to using the original path if
+// that was the specified redirection type.
+if (Redirection == RedirectKind::Fallthrough &&
+isFileNotFound(Result.getError()))
+  return ExternalFS->exists(Path);
+return false;
+  }
+
+  std::optional ExtRedirect = Result->getExternalRedirect();
+  if (!ExtRedirect) {
+assert(isa(Result->E));
+return true;
+  }
+
+  SmallString<256> RemappedPath((*ExtRedirect).str());
+  if (makeAbsolute(RemappedPath))
+return false;
+
+  if (ExternalFS->exists(RemappedPath))
+return true;
+
+  if (Redirection == RedirectKind::Fallthrough) {
+// Mapped the file but it wasn't found in the underlying filesystem,
+// fallthrough to using the original path if that was the specified
+// redirection type.
+return ExternalFS->exists(Path);
+  }
+
+  return false;
+}
+
 namespace {
 
 /// Provide a file wrapper with an over

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-11 Thread Jan Svoboda via cfe-commits


jansvoboda11 wrote:

Please apply the code formatting suggestions.

https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-11 Thread Jan Svoboda via cfe-commits



@@ -0,0 +1,51 @@
+//===- DependencyScanningFilesystemTest.cpp 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h"
+#include "llvm/ADT/Twine.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include "gtest/gtest.h"
+
+using namespace clang::tooling::dependencies;
+
+namespace {
+ struct InstrumentingFilesystem
+ : llvm::RTTIExtends {
+   unsigned NumStatCalls = 0;
+
+   using llvm::RTTIExtends::RTTIExtends;
+
+   llvm::ErrorOr status(const llvm::Twine &Path) override {
+ ++NumStatCalls;
+ return ProxyFileSystem::status(Path);
+   }
+ };
+ } // namespace
+
+
+TEST(DependencyScanningFilesystem, CacheStatOnExists) {
+  auto InMemoryFS = llvm::makeIntrusiveRefCnt();
+  InMemoryFS->setCurrentWorkingDirectory("/");
+  InMemoryFS->addFile("/foo", 0, llvm::MemoryBuffer::getMemBuffer(""));
+  InMemoryFS->addFile("/bar", 0, llvm::MemoryBuffer::getMemBuffer(""));
+
+  auto InstrumentingFS =
+llvm::makeIntrusiveRefCnt(InMemoryFS);
+  DependencyScanningFilesystemSharedCache SharedCache;
+  DependencyScanningWorkerFilesystem DepFS(SharedCache, InstrumentingFS);
+
+  DepFS.status("/foo");
+  DepFS.status("/foo");
+  DepFS.status("/bar");
+  EXPECT_EQ(InstrumentingFS->NumStatCalls, 2u);
+
+  DepFS.exists("/foo");
+  DepFS.exists("/bar");
+  EXPECT_EQ(InstrumentingFS->NumStatCalls, 2u);  

jansvoboda11 wrote:

It would be good to explicitly check that we're not calling `exists()` instead 
of relying on the fact that `InMemoryFileSystem` keeps implementing `exists()` 
in terms of `status()`.

https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-11 Thread via cfe-commits


github-actions[bot] wrote:




:warning: C/C++ code formatter, clang-format found issues in your code. 
:warning:



You can test this locally with the following command:


``bash
git-clang-format --diff 5601e35f620eccdebab988bed4b9677b29366b79 
2c9430526320b815f8722512f141e7a90f1c5eb1 -- 
clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h 
clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp 
llvm/include/llvm/Support/VirtualFileSystem.h 
llvm/lib/Support/VirtualFileSystem.cpp 
llvm/unittests/Support/VirtualFileSystemTest.cpp 
clang/unittests/Tooling/DependencyScanning/DependencyScannerTest.cpp
``





View the diff from clang-format here.


``diff
diff --git 
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h 
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
index 7b74b2b913..4d2109ef1d 100644
--- 
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ 
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -310,8 +310,8 @@ public:
   /// false if not (i.e. this entry is not a file or its scan fails).
   bool ensureDirectiveTokensArePopulated(EntryRef Entry);
 
-  /// Check whether \p Path exists. By default checks cached result of \c 
status(),
-  /// and falls back on FS if unable to do so.
+  /// Check whether \p Path exists. By default checks cached result of \c
+  /// status(), and falls back on FS if unable to do so.
   bool exists(const Twine &Path) override;
 
 private:
diff --git 
a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
 
b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
index bd2fe4fe99..0693eba3a4 100644
--- 
a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
+++ 
b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
@@ -14,28 +14,32 @@
 using namespace clang::tooling::dependencies;
 
 namespace {
- struct InstrumentingInMemoryFilesystem
- : llvm::RTTIExtends {
-   unsigned NumStatCalls = 0;
+struct InstrumentingInMemoryFilesystem
+: llvm::RTTIExtends {
+  unsigned NumStatCalls = 0;
 
-   using llvm::RTTIExtends::RTTIExtends;
-
-   llvm::ErrorOr status(const llvm::Twine &Path) override {
- ++NumStatCalls;
- return InMemoryFileSystem::status(Path);
-   }
- };
- } // namespace
+  using llvm::RTTIExtends::RTTIExtends;
 
+  llvm::ErrorOr status(const llvm::Twine &Path) override {
+++NumStatCalls;
+return InMemoryFileSystem::status(Path);
+  }
+};
+} // namespace
 
 TEST(DependencyScanningFilesystem, CacheStatOnExists) {
-  auto InMemoryInstrumentingFS = 
llvm::makeIntrusiveRefCnt();
+  auto InMemoryInstrumentingFS =
+  llvm::makeIntrusiveRefCnt();
   InMemoryInstrumentingFS->setCurrentWorkingDirectory("/");
-  InMemoryInstrumentingFS->addFile("/foo", 0, 
llvm::MemoryBuffer::getMemBuffer(""));
-  InMemoryInstrumentingFS->addFile("/bar", 0, 
llvm::MemoryBuffer::getMemBuffer(""));
+  InMemoryInstrumentingFS->addFile("/foo", 0,
+   llvm::MemoryBuffer::getMemBuffer(""));
+  InMemoryInstrumentingFS->addFile("/bar", 0,
+   llvm::MemoryBuffer::getMemBuffer(""));
   DependencyScanningFilesystemSharedCache SharedCache;
-  DependencyScanningWorkerFilesystem DepFS(SharedCache, 
InMemoryInstrumentingFS);
+  DependencyScanningWorkerFilesystem DepFS(SharedCache,
+   InMemoryInstrumentingFS);
 
   DepFS.status("/foo");
   DepFS.status("/foo");
diff --git a/llvm/unittests/Support/VirtualFileSystemTest.cpp 
b/llvm/unittests/Support/VirtualFileSystemTest.cpp
index 78fff0fcab..09a602748e 100644
--- a/llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ b/llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -3443,51 +3443,51 @@ TEST(RedirectingFileSystemTest, ExternalPaths) {
 TEST(RedirectingFileSystemTest, Exists) {
   IntrusiveRefCntPtr Dummy(new NoStatusDummyFileSystem());
   auto YAML =
-MemoryBuffer::getMemBuffer("{\n"
-   "  'version': 0,\n"
-   "  'roots': [\n"
-   "{\n"
-   "  'type': 'directory-remap',\n"
-   "  'name': '/dremap',\n"
-   "  'external-contents': '/a',\n"
-   "},"
-   "{\n"
-   "  'type': 'directory-remap',\n"
-   "  'name': '/dmissing',\n"
-   "  'external-contents': '/dmissing',\n"
-   "},"
-   "{\n"
-   "  'type': 'directory',\n"
-

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-11 Thread Alexandre Ganea via cfe-commits

aganea wrote:

> In the meantime, are you able to work around this for your non-modular 
> use-case by applying your change when building your own compiler downstream?

Yes I fixed it in our downstream. I am more worried about other users, since 
the initial version that @hyp committed a while ago in 
e1f4c4aad27992d6b8a0b8d85af42c14fa68c298 didn't suffer of these issues.

> Also, just to be sure, this is not a regression, correct?

Not per se, not from the latest commits. It seems the issue was introduced when 
module support was added in 9ab6d8236b176bf9dd43741f4d874a8afebed99c. 
Unfortunately there are users like us with big C++ codebases and stuck with 
regular #includes, no modules for now.

https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-11 Thread Artem Chikin via cfe-commits


https://github.com/artemcm updated 
https://github.com/llvm/llvm-project/pull/88152

>From c2b2cd03e1d8f92b1df814e6312158b8820b790d Mon Sep 17 00:00:00 2001
From: Ben Langmuir 
Date: Tue, 9 Apr 2024 11:22:44 -0700
Subject: [PATCH 1/2] [llvm][vfs] Make vfs::FileSystem::exists() virtual NFC

Allow a vfs::FileSystem to provide a more efficient implementation of
exists() if they are able to. The existing FileSystem implementations
continue to default to using status() except that overlay, proxy, and
redirecting filesystems are taught to forward calls to exists()
correctly to their wrapped/external filesystem.
---
 llvm/include/llvm/Support/VirtualFileSystem.h |   8 +-
 llvm/lib/Support/VirtualFileSystem.cpp|  56 ++
 .../Support/VirtualFileSystemTest.cpp | 165 ++
 3 files changed, 227 insertions(+), 2 deletions(-)

diff --git a/llvm/include/llvm/Support/VirtualFileSystem.h 
b/llvm/include/llvm/Support/VirtualFileSystem.h
index 770ca8764426a4..bdc98bfd7d2571 100644
--- a/llvm/include/llvm/Support/VirtualFileSystem.h
+++ b/llvm/include/llvm/Support/VirtualFileSystem.h
@@ -300,8 +300,9 @@ class FileSystem : public 
llvm::ThreadSafeRefCountedBase,
   virtual std::error_code getRealPath(const Twine &Path,
   SmallVectorImpl &Output) const;
 
-  /// Check whether a file exists. Provided for convenience.
-  bool exists(const Twine &Path);
+  /// Check whether \p Path exists. By default this uses \c status(), but
+  /// filesystems may provide a more efficient implementation if available.
+  virtual bool exists(const Twine &Path);
 
   /// Is the file mounted on a local filesystem?
   virtual std::error_code isLocal(const Twine &Path, bool &Result);
@@ -386,6 +387,7 @@ class OverlayFileSystem : public 
RTTIExtends {
   void pushOverlay(IntrusiveRefCntPtr FS);
 
   llvm::ErrorOr status(const Twine &Path) override;
+  bool exists(const Twine &Path) override;
   llvm::ErrorOr>
   openFileForRead(const Twine &Path) override;
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
@@ -439,6 +441,7 @@ class ProxyFileSystem : public RTTIExtends {
   llvm::ErrorOr status(const Twine &Path) override {
 return FS->status(Path);
   }
+  bool exists(const Twine &Path) override { return FS->exists(Path); }
   llvm::ErrorOr>
   openFileForRead(const Twine &Path) override {
 return FS->openFileForRead(Path);
@@ -1044,6 +1047,7 @@ class RedirectingFileSystem
  bool UseExternalNames, FileSystem &ExternalFS);
 
   ErrorOr status(const Twine &Path) override;
+  bool exists(const Twine &Path) override;
   ErrorOr> openFileForRead(const Twine &Path) override;
 
   std::error_code getRealPath(const Twine &Path,
diff --git a/llvm/lib/Support/VirtualFileSystem.cpp 
b/llvm/lib/Support/VirtualFileSystem.cpp
index 057f8eae0552c6..272880ba602b84 100644
--- a/llvm/lib/Support/VirtualFileSystem.cpp
+++ b/llvm/lib/Support/VirtualFileSystem.cpp
@@ -439,6 +439,15 @@ ErrorOr OverlayFileSystem::status(const Twine 
&Path) {
   return make_error_code(llvm::errc::no_such_file_or_directory);
 }
 
+bool OverlayFileSystem::exists(const Twine &Path) {
+  // FIXME: handle symlinks that cross file systems
+  for (iterator I = overlays_begin(), E = overlays_end(); I != E; ++I) {
+if ((*I)->exists(Path))
+  return true;
+  }
+  return false;
+}
+
 ErrorOr>
 OverlayFileSystem::openFileForRead(const llvm::Twine &Path) {
   // FIXME: handle symlinks that cross file systems
@@ -2431,6 +2440,53 @@ ErrorOr RedirectingFileSystem::status(const 
Twine &OriginalPath) {
   return S;
 }
 
+bool RedirectingFileSystem::exists(const Twine &OriginalPath) {
+  SmallString<256> Path;
+  OriginalPath.toVector(Path);
+
+  if (makeAbsolute(Path))
+return false;
+
+  if (Redirection == RedirectKind::Fallback) {
+// Attempt to find the original file first, only falling back to the
+// mapped file if that fails.
+if (ExternalFS->exists(Path))
+  return true;
+  }
+
+  ErrorOr Result = lookupPath(Path);
+  if (!Result) {
+// Was not able to map file, fallthrough to using the original path if
+// that was the specified redirection type.
+if (Redirection == RedirectKind::Fallthrough &&
+isFileNotFound(Result.getError()))
+  return ExternalFS->exists(Path);
+return false;
+  }
+
+  std::optional ExtRedirect = Result->getExternalRedirect();
+  if (!ExtRedirect) {
+assert(isa(Result->E));
+return true;
+  }
+
+  SmallString<256> RemappedPath((*ExtRedirect).str());
+  if (makeAbsolute(RemappedPath))
+return false;
+
+  if (ExternalFS->exists(RemappedPath))
+return true;
+
+  if (Redirection == RedirectKind::Fallthrough) {
+// Mapped the file but it wasn't found in the underlying filesystem,
+// fallthrough to using the original path if that was the specified
+// redirection type.
+return ExternalFS->exists(Path);
+  }
+
+  return false;
+}
+
 namespace {
 
 /// Provide a file wrapper with an over

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-11 Thread Jan Svoboda via cfe-commits


https://github.com/jansvoboda11 edited 
https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-11 Thread Jan Svoboda via cfe-commits



@@ -270,6 +270,12 @@ DependencyScanningWorkerFilesystem::status(const Twine 
&Path) {
   return Result->getStatus();
 }
 
+bool
+DependencyScanningWorkerFilesystem::exists(const Twine &Path) {
+  llvm::ErrorOr Status = status(Path);

jansvoboda11 wrote:

The one from Ben's commit: using `exists()` instead of `status()` can be 
implemented more efficiently on some VFSs. It can be surprising that 
`DependencyScanningWorkerFilesystem` just throws that out of the window. But 
it's typically wrapped around `RealFileSystem` that doesn't specialize 
`exists()`, and we could not cache the result of just `exists()` here anyway.

https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-11 Thread Artem Chikin via cfe-commits



@@ -270,6 +270,12 @@ DependencyScanningWorkerFilesystem::status(const Twine 
&Path) {
   return Result->getStatus();
 }
 
+bool
+DependencyScanningWorkerFilesystem::exists(const Twine &Path) {
+  llvm::ErrorOr Status = status(Path);

artemcm wrote:

Which optimization are you referring to? 

https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-11 Thread Artem Chikin via cfe-commits



@@ -0,0 +1,51 @@
+//===- DependencyScanningFilesystemTest.cpp 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h"
+#include "llvm/ADT/Twine.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include "gtest/gtest.h"
+
+using namespace clang::tooling::dependencies;
+
+namespace {
+ struct InstrumentingFilesystem
+ : llvm::RTTIExtends {
+   unsigned NumStatCalls = 0;
+
+   using llvm::RTTIExtends::RTTIExtends;
+
+   llvm::ErrorOr status(const llvm::Twine &Path) override {
+ ++NumStatCalls;
+ return ProxyFileSystem::status(Path);
+   }
+ };
+ } // namespace
+
+
+TEST(DependencyScanningFilesystem, CacheStatOnExists) {
+  auto InMemoryFS = llvm::makeIntrusiveRefCnt();
+  InMemoryFS->setCurrentWorkingDirectory("/");
+  InMemoryFS->addFile("/foo", 0, llvm::MemoryBuffer::getMemBuffer(""));
+  InMemoryFS->addFile("/bar", 0, llvm::MemoryBuffer::getMemBuffer(""));
+
+  auto InstrumentingFS =
+llvm::makeIntrusiveRefCnt(InMemoryFS);
+  DependencyScanningFilesystemSharedCache SharedCache;
+  DependencyScanningWorkerFilesystem DepFS(SharedCache, InstrumentingFS);
+
+  DepFS.status("/foo");
+  DepFS.status("/foo");
+  DepFS.status("/bar");
+  EXPECT_EQ(InstrumentingFS->NumStatCalls, 2u);
+
+  DepFS.exists("/foo");
+  DepFS.exists("/bar");
+  EXPECT_EQ(InstrumentingFS->NumStatCalls, 2u);  

artemcm wrote:

Gah, was silly to use a ProxyFilesystem as the underlying FS. Fixed, thank you 
very much for catching. 

https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-11 Thread Artem Chikin via cfe-commits


https://github.com/artemcm updated 
https://github.com/llvm/llvm-project/pull/88152

>From c2b2cd03e1d8f92b1df814e6312158b8820b790d Mon Sep 17 00:00:00 2001
From: Ben Langmuir 
Date: Tue, 9 Apr 2024 11:22:44 -0700
Subject: [PATCH 1/2] [llvm][vfs] Make vfs::FileSystem::exists() virtual NFC

Allow a vfs::FileSystem to provide a more efficient implementation of
exists() if they are able to. The existing FileSystem implementations
continue to default to using status() except that overlay, proxy, and
redirecting filesystems are taught to forward calls to exists()
correctly to their wrapped/external filesystem.
---
 llvm/include/llvm/Support/VirtualFileSystem.h |   8 +-
 llvm/lib/Support/VirtualFileSystem.cpp|  56 ++
 .../Support/VirtualFileSystemTest.cpp | 165 ++
 3 files changed, 227 insertions(+), 2 deletions(-)

diff --git a/llvm/include/llvm/Support/VirtualFileSystem.h 
b/llvm/include/llvm/Support/VirtualFileSystem.h
index 770ca8764426a4..bdc98bfd7d2571 100644
--- a/llvm/include/llvm/Support/VirtualFileSystem.h
+++ b/llvm/include/llvm/Support/VirtualFileSystem.h
@@ -300,8 +300,9 @@ class FileSystem : public 
llvm::ThreadSafeRefCountedBase,
   virtual std::error_code getRealPath(const Twine &Path,
   SmallVectorImpl &Output) const;
 
-  /// Check whether a file exists. Provided for convenience.
-  bool exists(const Twine &Path);
+  /// Check whether \p Path exists. By default this uses \c status(), but
+  /// filesystems may provide a more efficient implementation if available.
+  virtual bool exists(const Twine &Path);
 
   /// Is the file mounted on a local filesystem?
   virtual std::error_code isLocal(const Twine &Path, bool &Result);
@@ -386,6 +387,7 @@ class OverlayFileSystem : public 
RTTIExtends {
   void pushOverlay(IntrusiveRefCntPtr FS);
 
   llvm::ErrorOr status(const Twine &Path) override;
+  bool exists(const Twine &Path) override;
   llvm::ErrorOr>
   openFileForRead(const Twine &Path) override;
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
@@ -439,6 +441,7 @@ class ProxyFileSystem : public RTTIExtends {
   llvm::ErrorOr status(const Twine &Path) override {
 return FS->status(Path);
   }
+  bool exists(const Twine &Path) override { return FS->exists(Path); }
   llvm::ErrorOr>
   openFileForRead(const Twine &Path) override {
 return FS->openFileForRead(Path);
@@ -1044,6 +1047,7 @@ class RedirectingFileSystem
  bool UseExternalNames, FileSystem &ExternalFS);
 
   ErrorOr status(const Twine &Path) override;
+  bool exists(const Twine &Path) override;
   ErrorOr> openFileForRead(const Twine &Path) override;
 
   std::error_code getRealPath(const Twine &Path,
diff --git a/llvm/lib/Support/VirtualFileSystem.cpp 
b/llvm/lib/Support/VirtualFileSystem.cpp
index 057f8eae0552c6..272880ba602b84 100644
--- a/llvm/lib/Support/VirtualFileSystem.cpp
+++ b/llvm/lib/Support/VirtualFileSystem.cpp
@@ -439,6 +439,15 @@ ErrorOr OverlayFileSystem::status(const Twine 
&Path) {
   return make_error_code(llvm::errc::no_such_file_or_directory);
 }
 
+bool OverlayFileSystem::exists(const Twine &Path) {
+  // FIXME: handle symlinks that cross file systems
+  for (iterator I = overlays_begin(), E = overlays_end(); I != E; ++I) {
+if ((*I)->exists(Path))
+  return true;
+  }
+  return false;
+}
+
 ErrorOr>
 OverlayFileSystem::openFileForRead(const llvm::Twine &Path) {
   // FIXME: handle symlinks that cross file systems
@@ -2431,6 +2440,53 @@ ErrorOr RedirectingFileSystem::status(const 
Twine &OriginalPath) {
   return S;
 }
 
+bool RedirectingFileSystem::exists(const Twine &OriginalPath) {
+  SmallString<256> Path;
+  OriginalPath.toVector(Path);
+
+  if (makeAbsolute(Path))
+return false;
+
+  if (Redirection == RedirectKind::Fallback) {
+// Attempt to find the original file first, only falling back to the
+// mapped file if that fails.
+if (ExternalFS->exists(Path))
+  return true;
+  }
+
+  ErrorOr Result = lookupPath(Path);
+  if (!Result) {
+// Was not able to map file, fallthrough to using the original path if
+// that was the specified redirection type.
+if (Redirection == RedirectKind::Fallthrough &&
+isFileNotFound(Result.getError()))
+  return ExternalFS->exists(Path);
+return false;
+  }
+
+  std::optional ExtRedirect = Result->getExternalRedirect();
+  if (!ExtRedirect) {
+assert(isa(Result->E));
+return true;
+  }
+
+  SmallString<256> RemappedPath((*ExtRedirect).str());
+  if (makeAbsolute(RemappedPath))
+return false;
+
+  if (ExternalFS->exists(RemappedPath))
+return true;
+
+  if (Redirection == RedirectKind::Fallthrough) {
+// Mapped the file but it wasn't found in the underlying filesystem,
+// fallthrough to using the original path if that was the specified
+// redirection type.
+return ExternalFS->exists(Path);
+  }
+
+  return false;
+}
+
 namespace {
 
 /// Provide a file wrapper with an over

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-11 Thread Artem Chikin via cfe-commits


https://github.com/artemcm updated 
https://github.com/llvm/llvm-project/pull/88152

>From c2b2cd03e1d8f92b1df814e6312158b8820b790d Mon Sep 17 00:00:00 2001
From: Ben Langmuir 
Date: Tue, 9 Apr 2024 11:22:44 -0700
Subject: [PATCH 1/2] [llvm][vfs] Make vfs::FileSystem::exists() virtual NFC

Allow a vfs::FileSystem to provide a more efficient implementation of
exists() if they are able to. The existing FileSystem implementations
continue to default to using status() except that overlay, proxy, and
redirecting filesystems are taught to forward calls to exists()
correctly to their wrapped/external filesystem.
---
 llvm/include/llvm/Support/VirtualFileSystem.h |   8 +-
 llvm/lib/Support/VirtualFileSystem.cpp|  56 ++
 .../Support/VirtualFileSystemTest.cpp | 165 ++
 3 files changed, 227 insertions(+), 2 deletions(-)

diff --git a/llvm/include/llvm/Support/VirtualFileSystem.h 
b/llvm/include/llvm/Support/VirtualFileSystem.h
index 770ca8764426a4..bdc98bfd7d2571 100644
--- a/llvm/include/llvm/Support/VirtualFileSystem.h
+++ b/llvm/include/llvm/Support/VirtualFileSystem.h
@@ -300,8 +300,9 @@ class FileSystem : public 
llvm::ThreadSafeRefCountedBase,
   virtual std::error_code getRealPath(const Twine &Path,
   SmallVectorImpl &Output) const;
 
-  /// Check whether a file exists. Provided for convenience.
-  bool exists(const Twine &Path);
+  /// Check whether \p Path exists. By default this uses \c status(), but
+  /// filesystems may provide a more efficient implementation if available.
+  virtual bool exists(const Twine &Path);
 
   /// Is the file mounted on a local filesystem?
   virtual std::error_code isLocal(const Twine &Path, bool &Result);
@@ -386,6 +387,7 @@ class OverlayFileSystem : public 
RTTIExtends {
   void pushOverlay(IntrusiveRefCntPtr FS);
 
   llvm::ErrorOr status(const Twine &Path) override;
+  bool exists(const Twine &Path) override;
   llvm::ErrorOr>
   openFileForRead(const Twine &Path) override;
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
@@ -439,6 +441,7 @@ class ProxyFileSystem : public RTTIExtends {
   llvm::ErrorOr status(const Twine &Path) override {
 return FS->status(Path);
   }
+  bool exists(const Twine &Path) override { return FS->exists(Path); }
   llvm::ErrorOr>
   openFileForRead(const Twine &Path) override {
 return FS->openFileForRead(Path);
@@ -1044,6 +1047,7 @@ class RedirectingFileSystem
  bool UseExternalNames, FileSystem &ExternalFS);
 
   ErrorOr status(const Twine &Path) override;
+  bool exists(const Twine &Path) override;
   ErrorOr> openFileForRead(const Twine &Path) override;
 
   std::error_code getRealPath(const Twine &Path,
diff --git a/llvm/lib/Support/VirtualFileSystem.cpp 
b/llvm/lib/Support/VirtualFileSystem.cpp
index 057f8eae0552c6..272880ba602b84 100644
--- a/llvm/lib/Support/VirtualFileSystem.cpp
+++ b/llvm/lib/Support/VirtualFileSystem.cpp
@@ -439,6 +439,15 @@ ErrorOr OverlayFileSystem::status(const Twine 
&Path) {
   return make_error_code(llvm::errc::no_such_file_or_directory);
 }
 
+bool OverlayFileSystem::exists(const Twine &Path) {
+  // FIXME: handle symlinks that cross file systems
+  for (iterator I = overlays_begin(), E = overlays_end(); I != E; ++I) {
+if ((*I)->exists(Path))
+  return true;
+  }
+  return false;
+}
+
 ErrorOr>
 OverlayFileSystem::openFileForRead(const llvm::Twine &Path) {
   // FIXME: handle symlinks that cross file systems
@@ -2431,6 +2440,53 @@ ErrorOr RedirectingFileSystem::status(const 
Twine &OriginalPath) {
   return S;
 }
 
+bool RedirectingFileSystem::exists(const Twine &OriginalPath) {
+  SmallString<256> Path;
+  OriginalPath.toVector(Path);
+
+  if (makeAbsolute(Path))
+return false;
+
+  if (Redirection == RedirectKind::Fallback) {
+// Attempt to find the original file first, only falling back to the
+// mapped file if that fails.
+if (ExternalFS->exists(Path))
+  return true;
+  }
+
+  ErrorOr Result = lookupPath(Path);
+  if (!Result) {
+// Was not able to map file, fallthrough to using the original path if
+// that was the specified redirection type.
+if (Redirection == RedirectKind::Fallthrough &&
+isFileNotFound(Result.getError()))
+  return ExternalFS->exists(Path);
+return false;
+  }
+
+  std::optional ExtRedirect = Result->getExternalRedirect();
+  if (!ExtRedirect) {
+assert(isa(Result->E));
+return true;
+  }
+
+  SmallString<256> RemappedPath((*ExtRedirect).str());
+  if (makeAbsolute(RemappedPath))
+return false;
+
+  if (ExternalFS->exists(RemappedPath))
+return true;
+
+  if (Redirection == RedirectKind::Fallthrough) {
+// Mapped the file but it wasn't found in the underlying filesystem,
+// fallthrough to using the original path if that was the specified
+// redirection type.
+return ExternalFS->exists(Path);
+  }
+
+  return false;
+}
+
 namespace {
 
 /// Provide a file wrapper with an over

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-11 Thread Jan Svoboda via cfe-commits


jansvoboda11 wrote:

@aganea Ah, got it. Unfortunately, caching stat failures for all directories 
doesn't work for modules. Clang is supposed to create the modules cache 
directory if one doesn't exist. But if we first cache its non-existence, Clang 
will never see it again, even after Clang itself created it.

I think we could turn off caching of stat failures only for the modules cache 
directory by giving the build system an API to configure 
`DependencyScanningService` with the modules cache directory, wire up all 
`DependencyScanningWorkerFilesystem` to only avoid caching stat failures for 
that path, and have all `DependencyScanningWorker` overwrite the 
`-fmodules-cache-path=` arguments with the configured path.

In the meantime, are you able to work around this for your non-modular use-case 
by applying your change when building your own compiler downstream?

Also, just to be sure, this is not a regression, correct?

https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-11 Thread Alexandre Ganea via cfe-commits

aganea wrote:

> > Not sure @jansvoboda11 perhaps if we want to cherry pick 
> > [b768a8c](https://github.com/llvm/llvm-project/commit/b768a8c1db85b9e84fd8b356570a3a8fbe37acf6)
> >  on `release/18.x`? Or should we land just a simple PR with just the 
> > function change above?
> 
> I can try pulling 
> [b768a8c](https://github.com/llvm/llvm-project/commit/b768a8c1db85b9e84fd8b356570a3a8fbe37acf6)
>  into the release branch.

Sorry I've been misleading, pushing b768a8c on 18.x doesn't fix the issue. The 
problem is around the directory queries and the function in b768a8c returns 
`false` for that case (thus no caching of errors).

Running with:
```
static bool shouldCacheStatFailures(StringRef Filename) {
  StringRef Ext = llvm::sys::path::extension(Filename);
  if (Ext.empty())
return false; // This may be the module cache directory.
  return true;
}
```
Takes 4 min 8 sec.

Then running with:
```
static bool shouldCacheStatFailures(StringRef Filename) {
  return true;
}
```
Takes 1 min 47 sec.

I think something more involved would be needed here. @jansvoboda11 in which 
case we don't want to consider the file system immutable during the execution 
of clang-scan-deps?

https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-10 Thread Jan Svoboda via cfe-commits



@@ -270,6 +270,12 @@ DependencyScanningWorkerFilesystem::status(const Twine 
&Path) {
   return Result->getStatus();
 }
 
+bool
+DependencyScanningWorkerFilesystem::exists(const Twine &Path) {
+  llvm::ErrorOr Status = status(Path);

jansvoboda11 wrote:

Would be good to explain why it's necessary & okay to kill the `exists()` 
optimization here.

https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-10 Thread Jan Svoboda via cfe-commits



@@ -0,0 +1,51 @@
+//===- DependencyScanningFilesystemTest.cpp 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h"
+#include "llvm/ADT/Twine.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include "gtest/gtest.h"
+
+using namespace clang::tooling::dependencies;
+
+namespace {
+ struct InstrumentingFilesystem
+ : llvm::RTTIExtends {
+   unsigned NumStatCalls = 0;
+
+   using llvm::RTTIExtends::RTTIExtends;
+
+   llvm::ErrorOr status(const llvm::Twine &Path) override {
+ ++NumStatCalls;
+ return ProxyFileSystem::status(Path);
+   }
+ };
+ } // namespace
+
+
+TEST(DependencyScanningFilesystem, CacheStatOnExists) {
+  auto InMemoryFS = llvm::makeIntrusiveRefCnt();
+  InMemoryFS->setCurrentWorkingDirectory("/");
+  InMemoryFS->addFile("/foo", 0, llvm::MemoryBuffer::getMemBuffer(""));
+  InMemoryFS->addFile("/bar", 0, llvm::MemoryBuffer::getMemBuffer(""));
+
+  auto InstrumentingFS =
+llvm::makeIntrusiveRefCnt(InMemoryFS);
+  DependencyScanningFilesystemSharedCache SharedCache;
+  DependencyScanningWorkerFilesystem DepFS(SharedCache, InstrumentingFS);
+
+  DepFS.status("/foo");
+  DepFS.status("/foo");
+  DepFS.status("/bar");
+  EXPECT_EQ(InstrumentingFS->NumStatCalls, 2u);
+
+  DepFS.exists("/foo");
+  DepFS.exists("/bar");
+  EXPECT_EQ(InstrumentingFS->NumStatCalls, 2u);  

jansvoboda11 wrote:

Wait, doesn't this test pass even prior to this patch?

https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-10 Thread Jan Svoboda via cfe-commits


https://github.com/jansvoboda11 requested changes to this pull request.


https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-10 Thread Jan Svoboda via cfe-commits


https://github.com/jansvoboda11 edited 
https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-10 Thread Jan Svoboda via cfe-commits


jansvoboda11 wrote:

Seems like some tests failed on Linux.

https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-10 Thread Jan Svoboda via cfe-commits


https://github.com/jansvoboda11 approved this pull request.

LGTM. Would be nice to land Ben's change separately.

https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-10 Thread Artem Chikin via cfe-commits


artemcm wrote:

> I'd like to see a unit test specific to `DependencyScanningFilesystem`, 
> similar to what I have here: #68645.

Done! Added one in `DependencyScanningFilesystemTest.cpp`. Thank you.

https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-10 Thread Artem Chikin via cfe-commits


https://github.com/artemcm updated 
https://github.com/llvm/llvm-project/pull/88152

>From c2b2cd03e1d8f92b1df814e6312158b8820b790d Mon Sep 17 00:00:00 2001
From: Ben Langmuir 
Date: Tue, 9 Apr 2024 11:22:44 -0700
Subject: [PATCH 1/2] [llvm][vfs] Make vfs::FileSystem::exists() virtual NFC

Allow a vfs::FileSystem to provide a more efficient implementation of
exists() if they are able to. The existing FileSystem implementations
continue to default to using status() except that overlay, proxy, and
redirecting filesystems are taught to forward calls to exists()
correctly to their wrapped/external filesystem.
---
 llvm/include/llvm/Support/VirtualFileSystem.h |   8 +-
 llvm/lib/Support/VirtualFileSystem.cpp|  56 ++
 .../Support/VirtualFileSystemTest.cpp | 165 ++
 3 files changed, 227 insertions(+), 2 deletions(-)

diff --git a/llvm/include/llvm/Support/VirtualFileSystem.h 
b/llvm/include/llvm/Support/VirtualFileSystem.h
index 770ca8764426a4..bdc98bfd7d2571 100644
--- a/llvm/include/llvm/Support/VirtualFileSystem.h
+++ b/llvm/include/llvm/Support/VirtualFileSystem.h
@@ -300,8 +300,9 @@ class FileSystem : public 
llvm::ThreadSafeRefCountedBase,
   virtual std::error_code getRealPath(const Twine &Path,
   SmallVectorImpl &Output) const;
 
-  /// Check whether a file exists. Provided for convenience.
-  bool exists(const Twine &Path);
+  /// Check whether \p Path exists. By default this uses \c status(), but
+  /// filesystems may provide a more efficient implementation if available.
+  virtual bool exists(const Twine &Path);
 
   /// Is the file mounted on a local filesystem?
   virtual std::error_code isLocal(const Twine &Path, bool &Result);
@@ -386,6 +387,7 @@ class OverlayFileSystem : public 
RTTIExtends {
   void pushOverlay(IntrusiveRefCntPtr FS);
 
   llvm::ErrorOr status(const Twine &Path) override;
+  bool exists(const Twine &Path) override;
   llvm::ErrorOr>
   openFileForRead(const Twine &Path) override;
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
@@ -439,6 +441,7 @@ class ProxyFileSystem : public RTTIExtends {
   llvm::ErrorOr status(const Twine &Path) override {
 return FS->status(Path);
   }
+  bool exists(const Twine &Path) override { return FS->exists(Path); }
   llvm::ErrorOr>
   openFileForRead(const Twine &Path) override {
 return FS->openFileForRead(Path);
@@ -1044,6 +1047,7 @@ class RedirectingFileSystem
  bool UseExternalNames, FileSystem &ExternalFS);
 
   ErrorOr status(const Twine &Path) override;
+  bool exists(const Twine &Path) override;
   ErrorOr> openFileForRead(const Twine &Path) override;
 
   std::error_code getRealPath(const Twine &Path,
diff --git a/llvm/lib/Support/VirtualFileSystem.cpp 
b/llvm/lib/Support/VirtualFileSystem.cpp
index 057f8eae0552c6..272880ba602b84 100644
--- a/llvm/lib/Support/VirtualFileSystem.cpp
+++ b/llvm/lib/Support/VirtualFileSystem.cpp
@@ -439,6 +439,15 @@ ErrorOr OverlayFileSystem::status(const Twine 
&Path) {
   return make_error_code(llvm::errc::no_such_file_or_directory);
 }
 
+bool OverlayFileSystem::exists(const Twine &Path) {
+  // FIXME: handle symlinks that cross file systems
+  for (iterator I = overlays_begin(), E = overlays_end(); I != E; ++I) {
+if ((*I)->exists(Path))
+  return true;
+  }
+  return false;
+}
+
 ErrorOr>
 OverlayFileSystem::openFileForRead(const llvm::Twine &Path) {
   // FIXME: handle symlinks that cross file systems
@@ -2431,6 +2440,53 @@ ErrorOr RedirectingFileSystem::status(const 
Twine &OriginalPath) {
   return S;
 }
 
+bool RedirectingFileSystem::exists(const Twine &OriginalPath) {
+  SmallString<256> Path;
+  OriginalPath.toVector(Path);
+
+  if (makeAbsolute(Path))
+return false;
+
+  if (Redirection == RedirectKind::Fallback) {
+// Attempt to find the original file first, only falling back to the
+// mapped file if that fails.
+if (ExternalFS->exists(Path))
+  return true;
+  }
+
+  ErrorOr Result = lookupPath(Path);
+  if (!Result) {
+// Was not able to map file, fallthrough to using the original path if
+// that was the specified redirection type.
+if (Redirection == RedirectKind::Fallthrough &&
+isFileNotFound(Result.getError()))
+  return ExternalFS->exists(Path);
+return false;
+  }
+
+  std::optional ExtRedirect = Result->getExternalRedirect();
+  if (!ExtRedirect) {
+assert(isa(Result->E));
+return true;
+  }
+
+  SmallString<256> RemappedPath((*ExtRedirect).str());
+  if (makeAbsolute(RemappedPath))
+return false;
+
+  if (ExternalFS->exists(RemappedPath))
+return true;
+
+  if (Redirection == RedirectKind::Fallthrough) {
+// Mapped the file but it wasn't found in the underlying filesystem,
+// fallthrough to using the original path if that was the specified
+// redirection type.
+return ExternalFS->exists(Path);
+  }
+
+  return false;
+}
+
 namespace {
 
 /// Provide a file wrapper with an over

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-10 Thread Jan Svoboda via cfe-commits


jansvoboda11 wrote:

I'd like to see a unit test specific to `DependencyScanningFilesystem`, similar 
to what I have here: https://github.com/llvm/llvm-project/pull/68645.

https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-10 Thread Jan Svoboda via cfe-commits


jansvoboda11 wrote:

> Not sure @jansvoboda11 perhaps if we want to cherry pick 
> [b768a8c](https://github.com/llvm/llvm-project/commit/b768a8c1db85b9e84fd8b356570a3a8fbe37acf6)
>  on `release/18.x`? Or should we land just a simple PR with just the function 
> change above?

I can try pulling b768a8c1db85b9e84fd8b356570a3a8fbe37acf6 into the release 
branch.

https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-10 Thread Alexandre Ganea via cfe-commits



@@ -201,6 +201,21 @@ class ErrorDummyFileSystem : public DummyFileSystem {
   }
 };
 
+/// A version of \c DummyFileSystem that aborts on \c status() to test that
+/// \c exists() is being used.
+class NoStatusDummyFileSystem : public DummyFileSystem {
+public:
+  ErrorOr status(const Twine &Path) override {
+llvm::report_fatal_error(
+"unexpected call to NoStatusDummyFileSystem::status");
+  }
+
+  bool exists(const Twine &Path) override {
+auto Status = DummyFileSystem::status(Path);

aganea wrote:

Explicit return type here too.

https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-10 Thread Alexandre Ganea via cfe-commits


https://github.com/aganea approved this pull request.

Otherwise this PR LGTM.

https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-10 Thread Alexandre Ganea via cfe-commits


https://github.com/aganea edited https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-10 Thread Alexandre Ganea via cfe-commits

aganea wrote:

> This patch doesn`t improve my usage, it seems I'm hitting a different 
> codepath than you do. I'll investigate.

Ah I see, the commit 
https://github.com/llvm/llvm-project/commit/b768a8c1db85b9e84fd8b356570a3a8fbe37acf6
 didn't make it in time for the 18.x branch. The issue I'm seeing here is with 
regular C++ code, no PCH, no modules. It is related to 
`shouldCacheStatFailures()` which currently in `release/18.x` happens to return 
`false` for directories. The searching behavior for `#include` files with 
`clang-cl` (not sure about plain clang) is essentially to query every include 
path passed on the command-line. That creates lots of OS `status()` calls, like 
shown in my profile above. Going back to the previous behavior as on `main` 
solves my issue, ie:
```
static bool shouldCacheStatFailures(StringRef Filename) {
  StringRef Ext = llvm::sys::path::extension(Filename);
  if (Ext.empty())
return false; // This may be the module cache directory.
  return true;
}
```
As an order of magnitude, the Unreal Engine target I'm testing consists of 
17,801 TUs, generating 1,2 billion status() calls in total (by counting the 
calls to `FileManager::getStatValue`) out of which  8,3 millions are real OS 
calls after the change above (by counting the calls to 
`llvm::sys::windows::status()`.

Not sure @jansvoboda11 perhaps if we want to cherry pick 
b768a8c1db85b9e84fd8b356570a3a8fbe37acf6 on `release/18.x`? Or should we land 
just a simple PR with just the function change above?

https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-09 Thread Alexandre Ganea via cfe-commits


aganea wrote:

This patch doesn`t improve my usage, it seems I'm hitting a different codepath 
than you do. I'll investigate.

https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-09 Thread Artem Chikin via cfe-commits


https://github.com/artemcm updated 
https://github.com/llvm/llvm-project/pull/88152

>From c2b2cd03e1d8f92b1df814e6312158b8820b790d Mon Sep 17 00:00:00 2001
From: Ben Langmuir 
Date: Tue, 9 Apr 2024 11:22:44 -0700
Subject: [PATCH 1/2] [llvm][vfs] Make vfs::FileSystem::exists() virtual NFC

Allow a vfs::FileSystem to provide a more efficient implementation of
exists() if they are able to. The existing FileSystem implementations
continue to default to using status() except that overlay, proxy, and
redirecting filesystems are taught to forward calls to exists()
correctly to their wrapped/external filesystem.
---
 llvm/include/llvm/Support/VirtualFileSystem.h |   8 +-
 llvm/lib/Support/VirtualFileSystem.cpp|  56 ++
 .../Support/VirtualFileSystemTest.cpp | 165 ++
 3 files changed, 227 insertions(+), 2 deletions(-)

diff --git a/llvm/include/llvm/Support/VirtualFileSystem.h 
b/llvm/include/llvm/Support/VirtualFileSystem.h
index 770ca8764426a4..bdc98bfd7d2571 100644
--- a/llvm/include/llvm/Support/VirtualFileSystem.h
+++ b/llvm/include/llvm/Support/VirtualFileSystem.h
@@ -300,8 +300,9 @@ class FileSystem : public 
llvm::ThreadSafeRefCountedBase,
   virtual std::error_code getRealPath(const Twine &Path,
   SmallVectorImpl &Output) const;
 
-  /// Check whether a file exists. Provided for convenience.
-  bool exists(const Twine &Path);
+  /// Check whether \p Path exists. By default this uses \c status(), but
+  /// filesystems may provide a more efficient implementation if available.
+  virtual bool exists(const Twine &Path);
 
   /// Is the file mounted on a local filesystem?
   virtual std::error_code isLocal(const Twine &Path, bool &Result);
@@ -386,6 +387,7 @@ class OverlayFileSystem : public 
RTTIExtends {
   void pushOverlay(IntrusiveRefCntPtr FS);
 
   llvm::ErrorOr status(const Twine &Path) override;
+  bool exists(const Twine &Path) override;
   llvm::ErrorOr>
   openFileForRead(const Twine &Path) override;
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
@@ -439,6 +441,7 @@ class ProxyFileSystem : public RTTIExtends {
   llvm::ErrorOr status(const Twine &Path) override {
 return FS->status(Path);
   }
+  bool exists(const Twine &Path) override { return FS->exists(Path); }
   llvm::ErrorOr>
   openFileForRead(const Twine &Path) override {
 return FS->openFileForRead(Path);
@@ -1044,6 +1047,7 @@ class RedirectingFileSystem
  bool UseExternalNames, FileSystem &ExternalFS);
 
   ErrorOr status(const Twine &Path) override;
+  bool exists(const Twine &Path) override;
   ErrorOr> openFileForRead(const Twine &Path) override;
 
   std::error_code getRealPath(const Twine &Path,
diff --git a/llvm/lib/Support/VirtualFileSystem.cpp 
b/llvm/lib/Support/VirtualFileSystem.cpp
index 057f8eae0552c6..272880ba602b84 100644
--- a/llvm/lib/Support/VirtualFileSystem.cpp
+++ b/llvm/lib/Support/VirtualFileSystem.cpp
@@ -439,6 +439,15 @@ ErrorOr OverlayFileSystem::status(const Twine 
&Path) {
   return make_error_code(llvm::errc::no_such_file_or_directory);
 }
 
+bool OverlayFileSystem::exists(const Twine &Path) {
+  // FIXME: handle symlinks that cross file systems
+  for (iterator I = overlays_begin(), E = overlays_end(); I != E; ++I) {
+if ((*I)->exists(Path))
+  return true;
+  }
+  return false;
+}
+
 ErrorOr>
 OverlayFileSystem::openFileForRead(const llvm::Twine &Path) {
   // FIXME: handle symlinks that cross file systems
@@ -2431,6 +2440,53 @@ ErrorOr RedirectingFileSystem::status(const 
Twine &OriginalPath) {
   return S;
 }
 
+bool RedirectingFileSystem::exists(const Twine &OriginalPath) {
+  SmallString<256> Path;
+  OriginalPath.toVector(Path);
+
+  if (makeAbsolute(Path))
+return false;
+
+  if (Redirection == RedirectKind::Fallback) {
+// Attempt to find the original file first, only falling back to the
+// mapped file if that fails.
+if (ExternalFS->exists(Path))
+  return true;
+  }
+
+  ErrorOr Result = lookupPath(Path);
+  if (!Result) {
+// Was not able to map file, fallthrough to using the original path if
+// that was the specified redirection type.
+if (Redirection == RedirectKind::Fallthrough &&
+isFileNotFound(Result.getError()))
+  return ExternalFS->exists(Path);
+return false;
+  }
+
+  std::optional ExtRedirect = Result->getExternalRedirect();
+  if (!ExtRedirect) {
+assert(isa(Result->E));
+return true;
+  }
+
+  SmallString<256> RemappedPath((*ExtRedirect).str());
+  if (makeAbsolute(RemappedPath))
+return false;
+
+  if (ExternalFS->exists(RemappedPath))
+return true;
+
+  if (Redirection == RedirectKind::Fallthrough) {
+// Mapped the file but it wasn't found in the underlying filesystem,
+// fallthrough to using the original path if that was the specified
+// redirection type.
+return ExternalFS->exists(Path);
+  }
+
+  return false;
+}
+
 namespace {
 
 /// Provide a file wrapper with an over

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-09 Thread Alexandre Ganea via cfe-commits



@@ -270,6 +270,12 @@ DependencyScanningWorkerFilesystem::status(const Twine 
&Path) {
   return Result->getStatus();
 }
 
+bool
+DependencyScanningWorkerFilesystem::exists(const Twine &Path) {
+  auto Status = status(Path);

aganea wrote:

The return type is unclear at first sight, I would suggest replacing the `auto` 
with the concrete type. Please see 
https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-09 Thread Alexandre Ganea via cfe-commits


https://github.com/aganea commented:

Thanks for the change! Quite interestingly I was just looking precisely at this 
issue today :smile: Things are worst on Windows, where mini-filter drivers 
kick-in and bring performance to its knees. I'll try your patch, see how that 
improves my usage!

https://github.com/llvm/llvm-project/assets/37383324/f6fc28e2-8039-4cfc-aaa8-bfbbfe234394";>


https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-09 Thread Alexandre Ganea via cfe-commits


https://github.com/aganea edited https://github.com/llvm/llvm-project/pull/88152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-09 Thread Artem Chikin via cfe-commits


https://github.com/artemcm updated 
https://github.com/llvm/llvm-project/pull/88152

>From c2b2cd03e1d8f92b1df814e6312158b8820b790d Mon Sep 17 00:00:00 2001
From: Ben Langmuir 
Date: Tue, 9 Apr 2024 11:22:44 -0700
Subject: [PATCH 1/2] [llvm][vfs] Make vfs::FileSystem::exists() virtual NFC

Allow a vfs::FileSystem to provide a more efficient implementation of
exists() if they are able to. The existing FileSystem implementations
continue to default to using status() except that overlay, proxy, and
redirecting filesystems are taught to forward calls to exists()
correctly to their wrapped/external filesystem.
---
 llvm/include/llvm/Support/VirtualFileSystem.h |   8 +-
 llvm/lib/Support/VirtualFileSystem.cpp|  56 ++
 .../Support/VirtualFileSystemTest.cpp | 165 ++
 3 files changed, 227 insertions(+), 2 deletions(-)

diff --git a/llvm/include/llvm/Support/VirtualFileSystem.h 
b/llvm/include/llvm/Support/VirtualFileSystem.h
index 770ca8764426a4..bdc98bfd7d2571 100644
--- a/llvm/include/llvm/Support/VirtualFileSystem.h
+++ b/llvm/include/llvm/Support/VirtualFileSystem.h
@@ -300,8 +300,9 @@ class FileSystem : public 
llvm::ThreadSafeRefCountedBase,
   virtual std::error_code getRealPath(const Twine &Path,
   SmallVectorImpl &Output) const;
 
-  /// Check whether a file exists. Provided for convenience.
-  bool exists(const Twine &Path);
+  /// Check whether \p Path exists. By default this uses \c status(), but
+  /// filesystems may provide a more efficient implementation if available.
+  virtual bool exists(const Twine &Path);
 
   /// Is the file mounted on a local filesystem?
   virtual std::error_code isLocal(const Twine &Path, bool &Result);
@@ -386,6 +387,7 @@ class OverlayFileSystem : public 
RTTIExtends {
   void pushOverlay(IntrusiveRefCntPtr FS);
 
   llvm::ErrorOr status(const Twine &Path) override;
+  bool exists(const Twine &Path) override;
   llvm::ErrorOr>
   openFileForRead(const Twine &Path) override;
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
@@ -439,6 +441,7 @@ class ProxyFileSystem : public RTTIExtends {
   llvm::ErrorOr status(const Twine &Path) override {
 return FS->status(Path);
   }
+  bool exists(const Twine &Path) override { return FS->exists(Path); }
   llvm::ErrorOr>
   openFileForRead(const Twine &Path) override {
 return FS->openFileForRead(Path);
@@ -1044,6 +1047,7 @@ class RedirectingFileSystem
  bool UseExternalNames, FileSystem &ExternalFS);
 
   ErrorOr status(const Twine &Path) override;
+  bool exists(const Twine &Path) override;
   ErrorOr> openFileForRead(const Twine &Path) override;
 
   std::error_code getRealPath(const Twine &Path,
diff --git a/llvm/lib/Support/VirtualFileSystem.cpp 
b/llvm/lib/Support/VirtualFileSystem.cpp
index 057f8eae0552c6..272880ba602b84 100644
--- a/llvm/lib/Support/VirtualFileSystem.cpp
+++ b/llvm/lib/Support/VirtualFileSystem.cpp
@@ -439,6 +439,15 @@ ErrorOr OverlayFileSystem::status(const Twine 
&Path) {
   return make_error_code(llvm::errc::no_such_file_or_directory);
 }
 
+bool OverlayFileSystem::exists(const Twine &Path) {
+  // FIXME: handle symlinks that cross file systems
+  for (iterator I = overlays_begin(), E = overlays_end(); I != E; ++I) {
+if ((*I)->exists(Path))
+  return true;
+  }
+  return false;
+}
+
 ErrorOr>
 OverlayFileSystem::openFileForRead(const llvm::Twine &Path) {
   // FIXME: handle symlinks that cross file systems
@@ -2431,6 +2440,53 @@ ErrorOr RedirectingFileSystem::status(const 
Twine &OriginalPath) {
   return S;
 }
 
+bool RedirectingFileSystem::exists(const Twine &OriginalPath) {
+  SmallString<256> Path;
+  OriginalPath.toVector(Path);
+
+  if (makeAbsolute(Path))
+return false;
+
+  if (Redirection == RedirectKind::Fallback) {
+// Attempt to find the original file first, only falling back to the
+// mapped file if that fails.
+if (ExternalFS->exists(Path))
+  return true;
+  }
+
+  ErrorOr Result = lookupPath(Path);
+  if (!Result) {
+// Was not able to map file, fallthrough to using the original path if
+// that was the specified redirection type.
+if (Redirection == RedirectKind::Fallthrough &&
+isFileNotFound(Result.getError()))
+  return ExternalFS->exists(Path);
+return false;
+  }
+
+  std::optional ExtRedirect = Result->getExternalRedirect();
+  if (!ExtRedirect) {
+assert(isa(Result->E));
+return true;
+  }
+
+  SmallString<256> RemappedPath((*ExtRedirect).str());
+  if (makeAbsolute(RemappedPath))
+return false;
+
+  if (ExternalFS->exists(RemappedPath))
+return true;
+
+  if (Redirection == RedirectKind::Fallthrough) {
+// Mapped the file but it wasn't found in the underlying filesystem,
+// fallthrough to using the original path if that was the specified
+// redirection type.
+return ExternalFS->exists(Path);
+  }
+
+  return false;
+}
+
 namespace {
 
 /// Provide a file wrapper with an over

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-09 Thread Artem Chikin via cfe-commits


https://github.com/artemcm updated 
https://github.com/llvm/llvm-project/pull/88152

>From c2b2cd03e1d8f92b1df814e6312158b8820b790d Mon Sep 17 00:00:00 2001
From: Ben Langmuir 
Date: Tue, 9 Apr 2024 11:22:44 -0700
Subject: [PATCH 1/2] [llvm][vfs] Make vfs::FileSystem::exists() virtual NFC

Allow a vfs::FileSystem to provide a more efficient implementation of
exists() if they are able to. The existing FileSystem implementations
continue to default to using status() except that overlay, proxy, and
redirecting filesystems are taught to forward calls to exists()
correctly to their wrapped/external filesystem.
---
 llvm/include/llvm/Support/VirtualFileSystem.h |   8 +-
 llvm/lib/Support/VirtualFileSystem.cpp|  56 ++
 .../Support/VirtualFileSystemTest.cpp | 165 ++
 3 files changed, 227 insertions(+), 2 deletions(-)

diff --git a/llvm/include/llvm/Support/VirtualFileSystem.h 
b/llvm/include/llvm/Support/VirtualFileSystem.h
index 770ca8764426a4..bdc98bfd7d2571 100644
--- a/llvm/include/llvm/Support/VirtualFileSystem.h
+++ b/llvm/include/llvm/Support/VirtualFileSystem.h
@@ -300,8 +300,9 @@ class FileSystem : public 
llvm::ThreadSafeRefCountedBase,
   virtual std::error_code getRealPath(const Twine &Path,
   SmallVectorImpl &Output) const;
 
-  /// Check whether a file exists. Provided for convenience.
-  bool exists(const Twine &Path);
+  /// Check whether \p Path exists. By default this uses \c status(), but
+  /// filesystems may provide a more efficient implementation if available.
+  virtual bool exists(const Twine &Path);
 
   /// Is the file mounted on a local filesystem?
   virtual std::error_code isLocal(const Twine &Path, bool &Result);
@@ -386,6 +387,7 @@ class OverlayFileSystem : public 
RTTIExtends {
   void pushOverlay(IntrusiveRefCntPtr FS);
 
   llvm::ErrorOr status(const Twine &Path) override;
+  bool exists(const Twine &Path) override;
   llvm::ErrorOr>
   openFileForRead(const Twine &Path) override;
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
@@ -439,6 +441,7 @@ class ProxyFileSystem : public RTTIExtends {
   llvm::ErrorOr status(const Twine &Path) override {
 return FS->status(Path);
   }
+  bool exists(const Twine &Path) override { return FS->exists(Path); }
   llvm::ErrorOr>
   openFileForRead(const Twine &Path) override {
 return FS->openFileForRead(Path);
@@ -1044,6 +1047,7 @@ class RedirectingFileSystem
  bool UseExternalNames, FileSystem &ExternalFS);
 
   ErrorOr status(const Twine &Path) override;
+  bool exists(const Twine &Path) override;
   ErrorOr> openFileForRead(const Twine &Path) override;
 
   std::error_code getRealPath(const Twine &Path,
diff --git a/llvm/lib/Support/VirtualFileSystem.cpp 
b/llvm/lib/Support/VirtualFileSystem.cpp
index 057f8eae0552c6..272880ba602b84 100644
--- a/llvm/lib/Support/VirtualFileSystem.cpp
+++ b/llvm/lib/Support/VirtualFileSystem.cpp
@@ -439,6 +439,15 @@ ErrorOr OverlayFileSystem::status(const Twine 
&Path) {
   return make_error_code(llvm::errc::no_such_file_or_directory);
 }
 
+bool OverlayFileSystem::exists(const Twine &Path) {
+  // FIXME: handle symlinks that cross file systems
+  for (iterator I = overlays_begin(), E = overlays_end(); I != E; ++I) {
+if ((*I)->exists(Path))
+  return true;
+  }
+  return false;
+}
+
 ErrorOr>
 OverlayFileSystem::openFileForRead(const llvm::Twine &Path) {
   // FIXME: handle symlinks that cross file systems
@@ -2431,6 +2440,53 @@ ErrorOr RedirectingFileSystem::status(const 
Twine &OriginalPath) {
   return S;
 }
 
+bool RedirectingFileSystem::exists(const Twine &OriginalPath) {
+  SmallString<256> Path;
+  OriginalPath.toVector(Path);
+
+  if (makeAbsolute(Path))
+return false;
+
+  if (Redirection == RedirectKind::Fallback) {
+// Attempt to find the original file first, only falling back to the
+// mapped file if that fails.
+if (ExternalFS->exists(Path))
+  return true;
+  }
+
+  ErrorOr Result = lookupPath(Path);
+  if (!Result) {
+// Was not able to map file, fallthrough to using the original path if
+// that was the specified redirection type.
+if (Redirection == RedirectKind::Fallthrough &&
+isFileNotFound(Result.getError()))
+  return ExternalFS->exists(Path);
+return false;
+  }
+
+  std::optional ExtRedirect = Result->getExternalRedirect();
+  if (!ExtRedirect) {
+assert(isa(Result->E));
+return true;
+  }
+
+  SmallString<256> RemappedPath((*ExtRedirect).str());
+  if (makeAbsolute(RemappedPath))
+return false;
+
+  if (ExternalFS->exists(RemappedPath))
+return true;
+
+  if (Redirection == RedirectKind::Fallthrough) {
+// Mapped the file but it wasn't found in the underlying filesystem,
+// fallthrough to using the original path if that was the specified
+// redirection type.
+return ExternalFS->exists(Path);
+  }
+
+  return false;
+}
+
 namespace {
 
 /// Provide a file wrapper with an over

[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)

2024-04-09 Thread Artem Chikin via cfe-commits


https://github.com/artemcm updated 
https://github.com/llvm/llvm-project/pull/88152

>From fdf95d6c9186b91cf39a988e3baaa21a9cabe6a1 Mon Sep 17 00:00:00 2001
From: Ben Langmuir 
Date: Thu, 6 Oct 2022 15:58:23 -0700
Subject: [PATCH 1/2] [llvm][vfs] Make vfs::FileSystem::exists() virtual NFC

Allow a vfs::FileSystem to provide a more efficient implementation of
exists() if they are able to. The existing FileSystem implementations
continue to default to using status() except that overlay, proxy, and
redirecting filesystems are taught to forward calls to exists()
correctly to their wrapped/external filesystem.
---
 llvm/include/llvm/Support/VirtualFileSystem.h |   8 +-
 llvm/lib/Support/VirtualFileSystem.cpp|  57 +++
 .../Support/VirtualFileSystemTest.cpp | 160 ++
 3 files changed, 223 insertions(+), 2 deletions(-)

diff --git a/llvm/include/llvm/Support/VirtualFileSystem.h 
b/llvm/include/llvm/Support/VirtualFileSystem.h
index 770ca8764426a4..bdc98bfd7d2571 100644
--- a/llvm/include/llvm/Support/VirtualFileSystem.h
+++ b/llvm/include/llvm/Support/VirtualFileSystem.h
@@ -300,8 +300,9 @@ class FileSystem : public 
llvm::ThreadSafeRefCountedBase,
   virtual std::error_code getRealPath(const Twine &Path,
   SmallVectorImpl &Output) const;
 
-  /// Check whether a file exists. Provided for convenience.
-  bool exists(const Twine &Path);
+  /// Check whether \p Path exists. By default this uses \c status(), but
+  /// filesystems may provide a more efficient implementation if available.
+  virtual bool exists(const Twine &Path);
 
   /// Is the file mounted on a local filesystem?
   virtual std::error_code isLocal(const Twine &Path, bool &Result);
@@ -386,6 +387,7 @@ class OverlayFileSystem : public 
RTTIExtends {
   void pushOverlay(IntrusiveRefCntPtr FS);
 
   llvm::ErrorOr status(const Twine &Path) override;
+  bool exists(const Twine &Path) override;
   llvm::ErrorOr>
   openFileForRead(const Twine &Path) override;
   directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
@@ -439,6 +441,7 @@ class ProxyFileSystem : public RTTIExtends {
   llvm::ErrorOr status(const Twine &Path) override {
 return FS->status(Path);
   }
+  bool exists(const Twine &Path) override { return FS->exists(Path); }
   llvm::ErrorOr>
   openFileForRead(const Twine &Path) override {
 return FS->openFileForRead(Path);
@@ -1044,6 +1047,7 @@ class RedirectingFileSystem
  bool UseExternalNames, FileSystem &ExternalFS);
 
   ErrorOr status(const Twine &Path) override;
+  bool exists(const Twine &Path) override;
   ErrorOr> openFileForRead(const Twine &Path) override;
 
   std::error_code getRealPath(const Twine &Path,
diff --git a/llvm/lib/Support/VirtualFileSystem.cpp 
b/llvm/lib/Support/VirtualFileSystem.cpp
index 057f8eae0552c6..ee679f14bf2a37 100644
--- a/llvm/lib/Support/VirtualFileSystem.cpp
+++ b/llvm/lib/Support/VirtualFileSystem.cpp
@@ -439,6 +439,15 @@ ErrorOr OverlayFileSystem::status(const Twine 
&Path) {
   return make_error_code(llvm::errc::no_such_file_or_directory);
 }
 
+bool OverlayFileSystem::exists(const Twine &Path) {
+  // FIXME: handle symlinks that cross file systems
+  for (iterator I = overlays_begin(), E = overlays_end(); I != E; ++I) {
+if ((*I)->exists(Path))
+  return true;
+  }
+  return false;
+}
+
 ErrorOr>
 OverlayFileSystem::openFileForRead(const llvm::Twine &Path) {
   // FIXME: handle symlinks that cross file systems
@@ -2431,6 +2440,54 @@ ErrorOr RedirectingFileSystem::status(const 
Twine &OriginalPath) {
   return S;
 }
 
+bool RedirectingFileSystem::exists(const Twine &OriginalPath) {
+  SmallString<256> CanonicalPath;
+  OriginalPath.toVector(CanonicalPath);
+
+  if (makeCanonical(CanonicalPath))
+return false;
+
+  if (Redirection == RedirectKind::Fallback) {
+// Attempt to find the original file first, only falling back to the
+// mapped file if that fails.
+if (ExternalFS->exists(CanonicalPath))
+  return true;
+  }
+
+  ErrorOr Result =
+  lookupPath(CanonicalPath);
+  if (!Result) {
+// Was not able to map file, fallthrough to using the original path if
+// that was the specified redirection type.
+if (Redirection == RedirectKind::Fallthrough &&
+isFileNotFound(Result.getError()))
+  return ExternalFS->exists(CanonicalPath);
+return false;
+  }
+
+  Optional ExtRedirect = Result->getExternalRedirect();
+  if (!ExtRedirect) {
+assert(isa(Result->E));
+return true;
+  }
+
+  SmallString<256> CanonicalRemappedPath((*ExtRedirect).str());
+  if (makeCanonical(CanonicalRemappedPath))
+return false;
+
+  if (ExternalFS->exists(CanonicalRemappedPath))
+return true;
+
+  if (Redirection == RedirectKind::Fallthrough) {
+// Mapped the file but it wasn't found in the underlying filesystem,
+// fallthrough to using the original path if that was the specified
+// redirection type.
+return ExternalFS->exists(Canon

43 matches

Mail list logo