https://github.com/qiongsiwu created
https://github.com/llvm/llvm-project/pull/144105
`DependencyScanningFileSystemSharedCache` can currently report invalid negative
stat cached paths. This PR enhances the reporting with two modifications.
1. The reported path are now null terminated char arrays instead of
`StringRef`s. This way the API's user can avoid copying `StringRef`s to other
containers because the char arrays can be used directly.
2. The API now reports invalid cache entry due to file size changes.
Specifically, we check each file's cached size against the size of the same
file on the underlying FS. If the sizes are different, diagnostics will be
reported.
>From 2187de3d30045a02c03ca009f2497608ab6bc6a4 Mon Sep 17 00:00:00 2001
From: Qiongsi Wu <qiongsi...@apple.com>
Date: Thu, 12 Jun 2025 16:05:32 -0700
Subject: [PATCH] Enhancing DependencyScanningFilesystemSharedCache's API that
reports invalid entries.
---
.../DependencyScanningFilesystem.h | 42 +++++++++++++++----
.../DependencyScanningFilesystem.cpp | 28 ++++++++-----
.../DependencyScanningFilesystemTest.cpp | 41 +++++++++++++++---
3 files changed, 88 insertions(+), 23 deletions(-)
diff --git
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
index a20a89a4c2b76..e0656676fefff 100644
---
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
+++
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
@@ -220,13 +220,41 @@ 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 FS if
- /// it is negatively stat cached. If re-stat succeeds on a path,
- /// the path is added to InvalidPaths, indicating that the cache
- /// may have erroneously negatively cached it. The caller can then
- /// use InvalidPaths to issue diagnostics.
- std::vector<StringRef>
- getInvalidNegativeStatCachedPaths(llvm::vfs::FileSystem &UnderlyingFS) const;
+ struct InvalidEntryDiagInfo {
+ // A null terminated string that contains a path.
+ const char *Path = nullptr;
+
+ enum class Type : unsigned char { NegativeCaching = 1, SizeChanged = 2 };
+
+ Type T;
+
+ struct SizeChangedInfo {
+ uint64_t CachedSize = 0;
+ uint64_t ActualSize = 0;
+ };
+
+ std::optional<SizeChangedInfo> SizeInfo;
+
+ InvalidEntryDiagInfo(const char *Path) : Path(Path) {
+ T = Type::NegativeCaching;
+ }
+
+ InvalidEntryDiagInfo(const char *Path, uint64_t CachedSize,
+ uint64_t ActualSize)
+ : Path(Path), SizeInfo({CachedSize, ActualSize}) {
+ T = Type::SizeChanged;
+ }
+ };
+
+ /// Visits all cached entries and re-stat an entry using UnderlyingFS to
check
+ /// if the cache contains invalid entries. An entry can be invalid for two
+ /// reasons:
+ /// 1. The entry contains a stat error, indicating the file did not exist
+ /// in the cache, but the file exists on the UnderlyingFS.
+ /// 2. The entry is associated with a file whose size is different from the
+ /// actual size on the UnderlyingFS.
+ std::vector<InvalidEntryDiagInfo>
+ getInvalidEntryDiagInfo(llvm::vfs::FileSystem &UnderlyingFS) const;
private:
std::unique_ptr<CacheShard[]> CacheShards;
diff --git
a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
index 140833050f4e9..64b41a482f921 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -107,31 +107,39 @@ DependencyScanningFilesystemSharedCache::getShardForUID(
return CacheShards[Hash % NumShards];
}
-std::vector<StringRef>
-DependencyScanningFilesystemSharedCache::getInvalidNegativeStatCachedPaths(
+std::vector<DependencyScanningFilesystemSharedCache::InvalidEntryDiagInfo>
+DependencyScanningFilesystemSharedCache::getInvalidEntryDiagInfo(
llvm::vfs::FileSystem &UnderlyingFS) const {
// Iterate through all shards and look for cached stat errors.
- std::vector<StringRef> InvalidPaths;
+ std::vector<InvalidEntryDiagInfo> InvalidDiagInfo;
for (unsigned i = 0; i < NumShards; i++) {
const CacheShard &Shard = CacheShards[i];
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) {
+ llvm::ErrorOr<llvm::vfs::Status> Status = UnderlyingFS.status(Path);
+ if (Status) {
+ if (Entry->getError()) {
// This is the case where we have cached the non-existence
- // of the file at Path first, and a a file at the path is created
+ // of the file at Path first, and a file at the path is created
// later. The cache entry is not invalidated (as we have no good
// way to do it now), which may lead to missing file build errors.
- InvalidPaths.push_back(Path);
+ InvalidDiagInfo.emplace_back(Path.data());
+ } else {
+ llvm::vfs::Status CachedStatus = Entry->getStatus();
+ uint64_t CachedSize = CachedStatus.getSize();
+ uint64_t ActualSize = Status->getSize();
+ if (CachedSize != ActualSize) {
+ // This is the case where the cached file has a different size
+ // from the actual file that comes from the underlying FS.
+ InvalidDiagInfo.emplace_back(Path.data(), CachedSize, ActualSize);
+ }
}
}
}
}
- return InvalidPaths;
+ return InvalidDiagInfo;
}
const CachedFileSystemEntry *
diff --git
a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
index 7420743c97a2a..068212696943a 100644
---
a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
+++
b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
@@ -187,18 +187,47 @@ TEST(DependencyScanningFilesystem,
DiagnoseStaleStatFailures) {
DependencyScanningWorkerFilesystem DepFS(SharedCache, InMemoryFS);
bool Path1Exists = DepFS.exists("/path1.suffix");
- EXPECT_EQ(Path1Exists, false);
+ ASSERT_EQ(Path1Exists, false);
// Adding a file that has been stat-ed,
InMemoryFS->addFile("/path1.suffix", 0,
llvm::MemoryBuffer::getMemBuffer(""));
Path1Exists = DepFS.exists("/path1.suffix");
// Due to caching in SharedCache, path1 should not exist in
// DepFS's eyes.
- EXPECT_EQ(Path1Exists, false);
+ ASSERT_EQ(Path1Exists, false);
- std::vector<llvm::StringRef> InvalidPaths =
- SharedCache.getInvalidNegativeStatCachedPaths(*InMemoryFS);
+ auto InvalidEntries = SharedCache.getInvalidEntryDiagInfo(*InMemoryFS);
- EXPECT_EQ(InvalidPaths.size(), 1u);
- ASSERT_STREQ("/path1.suffix", InvalidPaths[0].str().c_str());
+ EXPECT_EQ(InvalidEntries.size(), 1u);
+ ASSERT_STREQ("/path1.suffix", InvalidEntries[0].Path);
+}
+
+TEST(DependencyScanningFilesystem, DiagnoseCachedFileSizeChange) {
+ auto InMemoryFS1 =
llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+ auto InMemoryFS2 =
llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
+ InMemoryFS1->setCurrentWorkingDirectory("/");
+ InMemoryFS2->setCurrentWorkingDirectory("/");
+
+ DependencyScanningFilesystemSharedCache SharedCache;
+ DependencyScanningWorkerFilesystem DepFS(SharedCache, InMemoryFS1);
+
+ InMemoryFS1->addFile("/path1.suffix", 0,
+ llvm::MemoryBuffer::getMemBuffer(""));
+ bool Path1Exists = DepFS.exists("/path1.suffix");
+ ASSERT_EQ(Path1Exists, true);
+
+ // Add a file to a new FS that has the same path but different content.
+ InMemoryFS2->addFile("/path1.suffix", 1,
+ llvm::MemoryBuffer::getMemBuffer(" "));
+
+ // Check against the new file system. InMemoryFS2 could be the underlying
+ // physical system in the real world.
+ auto InvalidEntries = SharedCache.getInvalidEntryDiagInfo(*InMemoryFS2);
+
+ ASSERT_EQ(InvalidEntries.size(), 1u);
+ ASSERT_STREQ("/path1.suffix", InvalidEntries[0].Path);
+ auto SizeInfo = InvalidEntries[0].SizeInfo;
+ ASSERT_TRUE(SizeInfo);
+ ASSERT_EQ(SizeInfo->CachedSize, 0u);
+ ASSERT_EQ(SizeInfo->ActualSize, 8u);
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
