[clang][Dependency Scanning] Report only Regular File Size Changes When Computing Out-of-Date File System Cache Entries

[qiongsiwu]

We noticed that when a directory's content changes, its size reported by status can change as well. We do not want to include such "false positive" cases. This PR revises the computation so that only regular file size changes are considered out-of-date.

rdar://152247357

[llvmbot]

@llvm/pr-subscribers-clang

Author: Qiongsi Wu (qiongsiwu)

Changes

We noticed that when a directory's content changes, its size reported by status can change as well. We do not want to include such "false positive" cases. This PR revises the computation so that only regular file size changes are considered out-of-date.

rdar://152247357

---

Full diff: https://github.com/llvm/llvm-project/pull/148082.diff

2 Files Affected:

• (modified) clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp (+16-7)
• (modified) clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp (+31)

diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
index 2437b2d3595e5..b641e4a0f0abb 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -117,7 +117,6 @@ DependencyScanningFilesystemSharedCache::getOutOfDateEntries(
     std::lock_guard<std::mutex> LockGuard(Shard.CacheLock);
     for (const auto &[Path, CachedPair] : Shard.CacheByFilename) {
       const CachedFileSystemEntry *Entry = CachedPair.first;
-
       llvm::ErrorOr<llvm::vfs::Status> Status = UnderlyingFS.status(Path);
       if (Status) {
         if (Entry->getError()) {
@@ -128,12 +127,22 @@ DependencyScanningFilesystemSharedCache::getOutOfDateEntries(
           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);
+          if (Status->getType() == llvm::sys::fs::file_type::regular_file &&
+              Status->getType() == CachedStatus.getType()) {
+            // We only check regular files. Directory files sizes could change
+            // due to content changes, and reporting directory size changes can
+            // lead to false positives.
+            // TODO: At the moment, we do not detect symlinks to files whose
+            // size may change. We need to decide if we want to detect cached
+            // symlink size changes. We can also expand this to detect file
+            // type changes.
+            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);
+            }
           }
         }
       }
diff --git a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
index b461d9109271c..023c02ddaa3e4 100644
--- a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
+++ b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp
@@ -233,3 +233,34 @@ TEST(DependencyScanningFilesystem, DiagnoseCachedFileSizeChange) {
   ASSERT_EQ(SizeInfo->CachedSize, 0u);
   ASSERT_EQ(SizeInfo->ActualSize, 8u);
 }
+
+TEST(DependencyScanningFilesystem, DoNotDiagnoseDirSizeChange) {
+  llvm::SmallString<128> Dir;
+  ASSERT_FALSE(llvm::sys::fs::createUniqueDirectory("tmp", Dir));
+
+  llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS =
+      llvm::vfs::createPhysicalFileSystem();
+
+  DependencyScanningFilesystemSharedCache SharedCache;
+  DependencyScanningWorkerFilesystem DepFS(SharedCache, FS);
+
+  // Trigger the file system cache.
+  ASSERT_EQ(DepFS.exists(Dir), true);
+
+  // Add a file to the FS to change its size.
+  // It seems that directory sizes reported are not meaningful,
+  // and should not be used to check for size changes.
+  // This test is setup only to trigger a size change so that we
+  // know we are excluding directories from reporting.
+  llvm::SmallString<128> FilePath = Dir;
+  llvm::sys::path::append(FilePath, "file.h");
+  {
+    std::error_code EC;
+    llvm::raw_fd_ostream TempFile(FilePath, EC);
+    ASSERT_FALSE(EC);
+  }
+
+  // We do not report directory size changes.
+  auto InvalidEntries = SharedCache.getOutOfDateEntries(*FS);
+  EXPECT_EQ(InvalidEntries.size(), 0u);
+}

[jansvoboda11]

LGTM, thanks!