[clang] [clang][Dependency Scanning] Adding an API to Diagnose Invalid Negative Stat Cache Entries (PR #135703)
Qiongsi Wu via cfe-commits
cfe-commits at lists.llvm.org
Tue Apr 15 14:34:30 PDT 2025
https://github.com/qiongsiwu updated https://github.com/llvm/llvm-project/pull/135703
>From d4b1210c16b4fccc6faa9445bee457a1e330a025 Mon Sep 17 00:00:00 2001
From: Qiongsi Wu <qiongsi_wu at apple.com>
Date: Mon, 14 Apr 2025 16:49:07 -0700
Subject: [PATCH 1/3] Initial commit.
---
.../DependencyScanningFilesystem.h | 8 ++++++
.../DependencyScanningFilesystem.cpp | 26 +++++++++++++++++
.../DependencyScanningFilesystemTest.cpp | 28 +++++++++++++++++++
3 files changed, 62 insertions(+)
diff --git a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
index d12814e7c9253..648158c5287c6 100644
--- a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++ b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -220,6 +220,14 @@ class DependencyScanningFilesystemSharedCache {
CacheShard &getShardForFilename(StringRef Filename) const;
CacheShard &getShardForUID(llvm::sys::fs::UniqueID UID) const;
+ /// Visits all cached entries and re-stat an entry using the UnderlyingFS if
+ /// it is negatively stat cached. If the re-stat succeeds, print diagnostics
+ /// information to OS indicating that negative stat caching has been
+ /// invalidated.
+ void
+ diagnoseNegativeStatCachedPaths(llvm::raw_ostream &OS,
+ llvm::vfs::FileSystem &UnderlyingFS) const;
+
private:
std::unique_ptr<CacheShard[]> CacheShards;
unsigned NumShards;
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
index 4d738e4bea41a..fe72a1a91909e 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -108,6 +108,32 @@ DependencyScanningFilesystemSharedCache::getShardForUID(
return CacheShards[Hash % NumShards];
}
+void DependencyScanningFilesystemSharedCache::diagnoseNegativeStatCachedPaths(
+ llvm::raw_ostream &OS, llvm::vfs::FileSystem &UnderlyingFS) const {
+ // Iterate through all shards and look for cached stat errors.
+ for (unsigned i = 0; i < NumShards; i++) {
+ const CacheShard &Shard = CacheShards[i];
+ for (const auto &Path : Shard.CacheByFilename.keys()) {
+ auto CachedPairIt = Shard.CacheByFilename.find(Path);
+ auto CachedPair = CachedPairIt->getValue();
+ const CachedFileSystemEntry *Entry = CachedPair.first;
+
+ if (Entry->getError()) {
+ // Only examine cached errors.
+ llvm::ErrorOr<llvm::vfs::Status> Stat = UnderlyingFS.status(Path);
+ if (Stat) {
+ // This is the case where we have negatively cached the non-existence
+ // of the file at Path, but the cache is later invalidated. Report
+ // diagnostics.
+ OS << Path << " did not exist when it was first searched "
+ << "but was created later. This may have led to a build failure "
+ "due to missing files.\n";
+ }
+ }
+ }
+ }
+}
+
const CachedFileSystemEntry *
DependencyScanningFilesystemSharedCache::CacheShard::findEntryByFilename(
StringRef Filename) const {
diff --git a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
index 111351fb90cee..02b192f1ba587 100644
--- a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
+++ b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
@@ -178,3 +178,31 @@ TEST(DependencyScanningFilesystem, CacheStatFailures) {
DepFS.exists("/cache/a.pcm");
EXPECT_EQ(InstrumentingFS->NumStatusCalls, 5u);
}
+
+TEST(DependencyScanningFilesystem, DiagnoseStaleStatFailures) {
+ auto InMemoryFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+ InMemoryFS->setCurrentWorkingDirectory("/");
+ InMemoryFS->addFile("/dir", 0, llvm::MemoryBuffer::getMemBuffer(""));
+
+ DependencyScanningFilesystemSharedCache SharedCache;
+ DependencyScanningWorkerFilesystem DepFS(SharedCache, InMemoryFS);
+
+ bool Path1Exists = DepFS.exists("/path1");
+ EXPECT_EQ(Path1Exists, false);
+
+ // Adding a file that has been stat-ed,
+ InMemoryFS->addFile("/path1", 0, llvm::MemoryBuffer::getMemBuffer(""));
+ Path1Exists = DepFS.exists("/path1");
+ // Due to caching in SharedCache, path1 should not exist in
+ // DepFS's eyes.
+ EXPECT_EQ(Path1Exists, false);
+
+ std::string Diags;
+ llvm::raw_string_ostream DiagsStream(Diags);
+ SharedCache.diagnoseNegativeStatCachedPaths(DiagsStream, *InMemoryFS.get());
+
+ ASSERT_STREQ(
+ "/path1 did not exist when it was first searched but was created later. "
+ "This may have led to a build failure due to missing files.\n",
+ Diags.c_str());
+}
>From b1058390940f40c9f62e9bcf745b4381a40db5e5 Mon Sep 17 00:00:00 2001
From: Qiongsi Wu <qiongsi_wu at apple.com>
Date: Tue, 15 Apr 2025 10:37:52 -0700
Subject: [PATCH 2/3] Address code review.
---
.../DependencyScanningFilesystem.cpp | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
index fe72a1a91909e..e61d8afeb51bb 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -113,18 +113,17 @@ void DependencyScanningFilesystemSharedCache::diagnoseNegativeStatCachedPaths(
// Iterate through all shards and look for cached stat errors.
for (unsigned i = 0; i < NumShards; i++) {
const CacheShard &Shard = CacheShards[i];
- for (const auto &Path : Shard.CacheByFilename.keys()) {
- auto CachedPairIt = Shard.CacheByFilename.find(Path);
- auto CachedPair = CachedPairIt->getValue();
+ std::lock_guard<std::mutex> LockGuard(Shard.CacheLock);
+ for (const auto &[Path, CachedPair] : Shard.CacheByFilename) {
const CachedFileSystemEntry *Entry = CachedPair.first;
if (Entry->getError()) {
// Only examine cached errors.
llvm::ErrorOr<llvm::vfs::Status> Stat = UnderlyingFS.status(Path);
if (Stat) {
- // This is the case where we have negatively cached the non-existence
- // of the file at Path, but the cache is later invalidated. Report
- // diagnostics.
+ // This is the case where we have cached the non-existence
+ // of the file at Path, but the file is created but the cache is
+ // not invalidated. Report diagnostics.
OS << Path << " did not exist when it was first searched "
<< "but was created later. This may have led to a build failure "
"due to missing files.\n";
>From 17a3426d04ca69c6c01b7240e9811946c00819bd Mon Sep 17 00:00:00 2001
From: Qiongsi Wu <qiongsi_wu at apple.com>
Date: Tue, 15 Apr 2025 14:34:17 -0700
Subject: [PATCH 3/3] Shorten the error message.
---
.../DependencyScanningFilesystem.cpp | 13 ++++++++++---
.../DependencyScanningFilesystemTest.cpp | 6 +++---
2 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
index e61d8afeb51bb..bdd8695991f17 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -111,6 +111,7 @@ DependencyScanningFilesystemSharedCache::getShardForUID(
void DependencyScanningFilesystemSharedCache::diagnoseNegativeStatCachedPaths(
llvm::raw_ostream &OS, llvm::vfs::FileSystem &UnderlyingFS) const {
// Iterate through all shards and look for cached stat errors.
+ SmallVector<StringRef, 4> InvalidCachedPaths;
for (unsigned i = 0; i < NumShards; i++) {
const CacheShard &Shard = CacheShards[i];
std::lock_guard<std::mutex> LockGuard(Shard.CacheLock);
@@ -124,13 +125,19 @@ void DependencyScanningFilesystemSharedCache::diagnoseNegativeStatCachedPaths(
// This is the case where we have cached the non-existence
// of the file at Path, but the file is created but the cache is
// not invalidated. Report diagnostics.
- OS << Path << " did not exist when it was first searched "
- << "but was created later. This may have led to a build failure "
- "due to missing files.\n";
+ InvalidCachedPaths.push_back(Path);
}
}
}
}
+
+ if (InvalidCachedPaths.size()) {
+ OS << "The following paths did not exist when they were first searched, "
+ "but files they point to were created later:\n";
+ for (const auto P : InvalidCachedPaths)
+ OS << "\t" << P << "\n";
+ OS << "Files missing at the paths above may have caused build errors.\n";
+ }
}
const CachedFileSystemEntry *
diff --git a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
index 02b192f1ba587..0e5c31f52cce2 100644
--- a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
+++ b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
@@ -182,7 +182,6 @@ TEST(DependencyScanningFilesystem, CacheStatFailures) {
TEST(DependencyScanningFilesystem, DiagnoseStaleStatFailures) {
auto InMemoryFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
InMemoryFS->setCurrentWorkingDirectory("/");
- InMemoryFS->addFile("/dir", 0, llvm::MemoryBuffer::getMemBuffer(""));
DependencyScanningFilesystemSharedCache SharedCache;
DependencyScanningWorkerFilesystem DepFS(SharedCache, InMemoryFS);
@@ -202,7 +201,8 @@ TEST(DependencyScanningFilesystem, DiagnoseStaleStatFailures) {
SharedCache.diagnoseNegativeStatCachedPaths(DiagsStream, *InMemoryFS.get());
ASSERT_STREQ(
- "/path1 did not exist when it was first searched but was created later. "
- "This may have led to a build failure due to missing files.\n",
+ "The following paths did not exist when they were first searched, but "
+ "files they point to were created later:\n\t/path1\nFiles missing at the "
+ "paths above may have caused build errors.\n",
Diags.c_str());
}
More information about the cfe-commits
mailing list