[clang] [llvm] [clang][deps] Overload `Filesystem::exists` in `DependencyScanningFilesystem` to have it use cached `status` (PR #88152)
Artem Chikin via cfe-commits
cfe-commits at lists.llvm.org
Thu Apr 11 10:20:06 PDT 2024
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 <blangmuir at apple.com>
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<FileSystem>,
virtual std::error_code getRealPath(const Twine &Path,
SmallVectorImpl<char> &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<OverlayFileSystem, FileSystem> {
void pushOverlay(IntrusiveRefCntPtr<FileSystem> FS);
llvm::ErrorOr<Status> status(const Twine &Path) override;
+ bool exists(const Twine &Path) override;
llvm::ErrorOr<std::unique_ptr<File>>
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<ProxyFileSystem, FileSystem> {
llvm::ErrorOr<Status> status(const Twine &Path) override {
return FS->status(Path);
}
+ bool exists(const Twine &Path) override { return FS->exists(Path); }
llvm::ErrorOr<std::unique_ptr<File>>
openFileForRead(const Twine &Path) override {
return FS->openFileForRead(Path);
@@ -1044,6 +1047,7 @@ class RedirectingFileSystem
bool UseExternalNames, FileSystem &ExternalFS);
ErrorOr<Status> status(const Twine &Path) override;
+ bool exists(const Twine &Path) override;
ErrorOr<std::unique_ptr<File>> 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<Status> 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<std::unique_ptr<File>>
OverlayFileSystem::openFileForRead(const llvm::Twine &Path) {
// FIXME: handle symlinks that cross file systems
@@ -2431,6 +2440,53 @@ ErrorOr<Status> 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<RedirectingFileSystem::LookupResult> 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<StringRef> ExtRedirect = Result->getExternalRedirect();
+ if (!ExtRedirect) {
+ assert(isa<RedirectingFileSystem::DirectoryEntry>(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 status.
diff --git a/llvm/unittests/Support/VirtualFileSystemTest.cpp b/llvm/unittests/Support/VirtualFileSystemTest.cpp
index 695b09343257f1..f78e7d81499825 100644
--- a/llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ b/llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -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<vfs::Status> 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);
+ return Status && Status->exists();
+ }
+};
+
/// Replace back-slashes by front-slashes.
std::string getPosixPath(const Twine &S) {
SmallString<128> Result;
@@ -968,6 +983,30 @@ TEST(OverlayFileSystemTest, PrintOutput) {
Output);
}
+TEST(OverlayFileSystemTest, Exists) {
+ IntrusiveRefCntPtr<DummyFileSystem> Lower(new NoStatusDummyFileSystem());
+ IntrusiveRefCntPtr<DummyFileSystem> Upper(new NoStatusDummyFileSystem());
+ IntrusiveRefCntPtr<vfs::OverlayFileSystem> O(
+ new vfs::OverlayFileSystem(Lower));
+ O->pushOverlay(Upper);
+
+ Lower->addDirectory("/both");
+ Upper->addDirectory("/both");
+ Lower->addRegularFile("/both/lower_file");
+ Upper->addRegularFile("/both/upper_file");
+ Lower->addDirectory("/lower");
+ Upper->addDirectory("/upper");
+
+ EXPECT_TRUE(O->exists("/both"));
+ EXPECT_TRUE(O->exists("/both"));
+ EXPECT_TRUE(O->exists("/both/lower_file"));
+ EXPECT_TRUE(O->exists("/both/upper_file"));
+ EXPECT_TRUE(O->exists("/lower"));
+ EXPECT_TRUE(O->exists("/upper"));
+ EXPECT_FALSE(O->exists("/both/nope"));
+ EXPECT_FALSE(O->exists("/nope"));
+}
+
TEST(ProxyFileSystemTest, Basic) {
IntrusiveRefCntPtr<vfs::InMemoryFileSystem> Base(
new vfs::InMemoryFileSystem());
@@ -3364,6 +3403,11 @@ TEST(RedirectingFileSystemTest, ExternalPaths) {
SeenPaths.push_back(Dir.str());
return ProxyFileSystem::dir_begin(Dir, EC);
}
+
+ bool exists(const Twine &Path) override {
+ SeenPaths.push_back(Path.str());
+ return ProxyFileSystem::exists(Path);
+ }
};
std::error_code EC;
@@ -3395,3 +3439,124 @@ TEST(RedirectingFileSystemTest, ExternalPaths) {
EXPECT_EQ(CheckFS->SeenPaths, Expected);
}
+
+TEST(RedirectingFileSystemTest, Exists) {
+ IntrusiveRefCntPtr<DummyFileSystem> 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"
+ " 'name': '/both',\n"
+ " 'contents': [\n"
+ " {\n"
+ " 'type': 'file',\n"
+ " 'name': 'vfile',\n"
+ " 'external-contents': '/c'\n"
+ " }\n"
+ " ]\n"
+ " },\n"
+ " {\n"
+ " 'type': 'directory',\n"
+ " 'name': '/vdir',\n"
+ " 'contents': ["
+ " {\n"
+ " 'type': 'directory-remap',\n"
+ " 'name': 'dremap',\n"
+ " 'external-contents': '/b'\n"
+ " },\n"
+ " {\n"
+ " 'type': 'file',\n"
+ " 'name': 'missing',\n"
+ " 'external-contents': '/missing'\n"
+ " },\n"
+ " {\n"
+ " 'type': 'file',\n"
+ " 'name': 'vfile',\n"
+ " 'external-contents': '/c'\n"
+ " }]\n"
+ " }]\n"
+ "}");
+
+ Dummy->addDirectory("/a");
+ Dummy->addRegularFile("/a/foo");
+ Dummy->addDirectory("/b");
+ Dummy->addRegularFile("/c");
+ Dummy->addRegularFile("/both/foo");
+
+ auto Redirecting = vfs::RedirectingFileSystem::create(
+ std::move(YAML), nullptr, "", nullptr, Dummy);
+
+ EXPECT_TRUE(Redirecting->exists("/dremap"));
+ EXPECT_FALSE(Redirecting->exists("/dmissing"));
+ EXPECT_FALSE(Redirecting->exists("/unknown"));
+ EXPECT_TRUE(Redirecting->exists("/both"));
+ EXPECT_TRUE(Redirecting->exists("/both/foo"));
+ EXPECT_TRUE(Redirecting->exists("/both/vfile"));
+ EXPECT_TRUE(Redirecting->exists("/vdir"));
+ EXPECT_TRUE(Redirecting->exists("/vdir/dremap"));
+ EXPECT_FALSE(Redirecting->exists("/vdir/missing"));
+ EXPECT_TRUE(Redirecting->exists("/vdir/vfile"));
+ EXPECT_FALSE(Redirecting->exists("/vdir/unknown"));
+}
+
+TEST(RedirectingFileSystemTest, ExistsFallback) {
+ IntrusiveRefCntPtr<DummyFileSystem> Dummy(new NoStatusDummyFileSystem());
+ auto YAML =
+ MemoryBuffer::getMemBuffer("{\n"
+ " 'version': 0,\n"
+ " 'redirecting-with': 'fallback',\n"
+ " 'roots': [\n"
+ " {\n"
+ " 'type': 'file',\n"
+ " 'name': '/fallback',\n"
+ " 'external-contents': '/missing',\n"
+ " },"
+ " ]\n"
+ "}");
+
+ Dummy->addRegularFile("/fallback");
+
+ auto Redirecting = vfs::RedirectingFileSystem::create(
+ std::move(YAML), nullptr, "", nullptr, Dummy);
+
+ EXPECT_TRUE(Redirecting->exists("/fallback"));
+ EXPECT_FALSE(Redirecting->exists("/missing"));
+}
+
+TEST(RedirectingFileSystemTest, ExistsRedirectOnly) {
+ IntrusiveRefCntPtr<DummyFileSystem> Dummy(new NoStatusDummyFileSystem());
+ auto YAML =
+ MemoryBuffer::getMemBuffer("{\n"
+ " 'version': 0,\n"
+ " 'redirecting-with': 'redirect-only',\n"
+ " 'roots': [\n"
+ " {\n"
+ " 'type': 'file',\n"
+ " 'name': '/vfile',\n"
+ " 'external-contents': '/a',\n"
+ " },"
+ " ]\n"
+ "}");
+
+ Dummy->addRegularFile("/a");
+ Dummy->addRegularFile("/b");
+
+ auto Redirecting = vfs::RedirectingFileSystem::create(
+ std::move(YAML), nullptr, "", nullptr, Dummy);
+
+ EXPECT_FALSE(Redirecting->exists("/a"));
+ EXPECT_FALSE(Redirecting->exists("/b"));
+ EXPECT_TRUE(Redirecting->exists("/vfile"));
+}
>From 2c9430526320b815f8722512f141e7a90f1c5eb1 Mon Sep 17 00:00:00 2001
From: Artem Chikin <achikin at apple.com>
Date: Tue, 9 Apr 2024 09:37:09 -0700
Subject: [PATCH 2/2] [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 +++++
clang/unittests/Tooling/CMakeLists.txt | 3 +-
.../DependencyScannerTest.cpp | 2 +-
.../DependencyScanningFilesystemTest.cpp | 48 +++++++++++++++++++
.../Support/VirtualFileSystemTest.cpp | 2 +-
6 files changed, 67 insertions(+), 3 deletions(-)
rename clang/unittests/Tooling/{ => DependencyScanning}/DependencyScannerTest.cpp (99%)
create mode 100644 clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
diff --git a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
index 9a522a3e2fe252..7b74b2b9134e6a 100644
--- a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -310,6 +310,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 9b7812a1adb9e3..090fe8042211ea 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -270,6 +270,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<llvm::vfs::Status> 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/CMakeLists.txt b/clang/unittests/Tooling/CMakeLists.txt
index 5a10a6b285390e..0eb612f8d94987 100644
--- a/clang/unittests/Tooling/CMakeLists.txt
+++ b/clang/unittests/Tooling/CMakeLists.txt
@@ -13,7 +13,6 @@ add_clang_unittest(ToolingTests
CastExprTest.cpp
CommentHandlerTest.cpp
CompilationDatabaseTest.cpp
- DependencyScannerTest.cpp
DiagnosticsYamlTest.cpp
ExecutionTest.cpp
FixItTest.cpp
@@ -24,6 +23,8 @@ add_clang_unittest(ToolingTests
LookupTest.cpp
QualTypeNamesTest.cpp
RangeSelectorTest.cpp
+ DependencyScanning/DependencyScannerTest.cpp
+ DependencyScanning/DependencyScanningFilesystemTest.cpp
RecursiveASTVisitorTests/Attr.cpp
RecursiveASTVisitorTests/BitfieldInitializer.cpp
RecursiveASTVisitorTests/CallbacksLeaf.cpp
diff --git a/clang/unittests/Tooling/DependencyScannerTest.cpp b/clang/unittests/Tooling/DependencyScanning/DependencyScannerTest.cpp
similarity index 99%
rename from clang/unittests/Tooling/DependencyScannerTest.cpp
rename to clang/unittests/Tooling/DependencyScanning/DependencyScannerTest.cpp
index 8735fcad200461..ae0a237c3f91cc 100644
--- a/clang/unittests/Tooling/DependencyScannerTest.cpp
+++ b/clang/unittests/Tooling/DependencyScanning/DependencyScannerTest.cpp
@@ -1,4 +1,4 @@
-//===- unittest/Tooling/ToolingTest.cpp - Tooling unit tests --------------===//
+//===- DependencyScannerTest.cpp - Tooling unit tests ---------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
diff --git a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
new file mode 100644
index 00000000000000..bd2fe4fe992e8f
--- /dev/null
+++ b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
@@ -0,0 +1,48 @@
+//===- 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 InstrumentingInMemoryFilesystem
+ : llvm::RTTIExtends<InstrumentingInMemoryFilesystem, llvm::vfs::InMemoryFileSystem> {
+ unsigned NumStatCalls = 0;
+
+ using llvm::RTTIExtends<InstrumentingInMemoryFilesystem,
+ llvm::vfs::InMemoryFileSystem>::RTTIExtends;
+
+ llvm::ErrorOr<llvm::vfs::Status> status(const llvm::Twine &Path) override {
+ ++NumStatCalls;
+ return InMemoryFileSystem::status(Path);
+ }
+ };
+ } // namespace
+
+
+TEST(DependencyScanningFilesystem, CacheStatOnExists) {
+ auto InMemoryInstrumentingFS = llvm::makeIntrusiveRefCnt<InstrumentingInMemoryFilesystem>();
+ InMemoryInstrumentingFS->setCurrentWorkingDirectory("/");
+ InMemoryInstrumentingFS->addFile("/foo", 0, llvm::MemoryBuffer::getMemBuffer(""));
+ InMemoryInstrumentingFS->addFile("/bar", 0, llvm::MemoryBuffer::getMemBuffer(""));
+ DependencyScanningFilesystemSharedCache SharedCache;
+ DependencyScanningWorkerFilesystem DepFS(SharedCache, InMemoryInstrumentingFS);
+
+ DepFS.status("/foo");
+ DepFS.status("/foo");
+ DepFS.status("/bar");
+ EXPECT_EQ(InMemoryInstrumentingFS->NumStatCalls, 2u);
+
+ DepFS.exists("/foo");
+ DepFS.exists("/bar");
+ EXPECT_EQ(InMemoryInstrumentingFS->NumStatCalls, 2u);
+}
diff --git a/llvm/unittests/Support/VirtualFileSystemTest.cpp b/llvm/unittests/Support/VirtualFileSystemTest.cpp
index f78e7d81499825..78fff0fcab7910 100644
--- a/llvm/unittests/Support/VirtualFileSystemTest.cpp
+++ b/llvm/unittests/Support/VirtualFileSystemTest.cpp
@@ -211,7 +211,7 @@ class NoStatusDummyFileSystem : public DummyFileSystem {
}
bool exists(const Twine &Path) override {
- auto Status = DummyFileSystem::status(Path);
+ llvm::ErrorOr<llvm::vfs::Status> Status = status(Path);
return Status && Status->exists();
}
};
More information about the cfe-commits
mailing list