[clang][modules] Dependency Scanning: Temporarily Turn Off Negative Directory Caching

[qiongsiwu]

55323ca implemented negative caching of directories. The implementation may be too aggressive in certain cases, and may lead to file not found errors even if the files exist on disk.

This PR temporarily turns off negative directory caching to fix the build failures.

rdar://148027982

[llvmbot]

@llvm/pr-subscribers-clang

Author: Qiongsi Wu (qiongsiwu)

Changes

55323ca implemented negative caching of directories. The implementation may be too aggressive in certain cases, and may lead to file not found errors even if the files exist on disk.

This PR temporarily turns off negative directory caching to fix the build failures.

rdar://148027982

---

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

1 Files Affected:

• (modified) clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp (+7)

diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
index 4d738e4bea41a..805409046cf54 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -241,6 +241,13 @@ DependencyScanningWorkerFilesystem::computeAndStoreResult(
   llvm::ErrorOr<llvm::vfs::Status> Stat =
       getUnderlyingFS().status(OriginalFilename);
   if (!Stat) {
+    // rdar://148027982
+    // Negative caching directories can cause build failures.
+    // FIXME: we should remove the check below once we know
+    // the build failures' root causes.
+    if (llvm::sys::path::extension(OriginalFilename).empty())
+      return Stat.getError();
+
     const auto &Entry =
         getOrEmplaceSharedEntryForFilename(FilenameForLookup, Stat.getError());
     return insertLocalEntryForFilename(FilenameForLookup, Entry);

[cyndyishida]

This PR temporarily turns off negative directory caching to fix the build failures.

What does temporarily mean here? Is it until the project identified with errors can resolve themselves? Is it until error detection in the build system can catch cases where negative stating can cause incorrect build errors? Is it a pre-determined time frame?

I'm concerned about disabling this optimization entirely without an understanding of how it will be turned back on. What is the added performance cost of doing this in environments where I/O accesses can be expensive?

[qiongsiwu]

To be clear, this PR is basically cherry-picking swiftlang#9076, but we are only taking what is necessary to fix our problem at the moment. I updated the description to reflect this.

What does temporarily mean here? Is it until the project identified with errors can resolve themselves? Is it until error detection in the build system can catch cases where negative stating can cause incorrect build errors? Is it a pre-determined time frame?

My intention is that this should be turned back on when we know the root cause of the build failures, and have some ways to fix the failures. While I don't have an exact date to turn this back on, I expect some updates this week. Ideally, I hope we can turn the optimization back on here in a week or two.

I'm concerned about disabling this optimization entirely without an understanding of how it will be turned back on. What is the added performance cost of doing this in environments where I/O accesses can be expensive?

@jansvoboda11 may I get your comments on the possible performance impact? I suspect this is tolerable for a few weeks because we have released compilers with swiftlang#9076 in them.

[cyndyishida]

To be clear, this PR is basically cherry-picking swiftlang#9076

AFAICT that patch special-cased for framework directories while this one disables much more. Is that not the case?

[qiongsiwu]

AFAICT that patch special-cased for framework directories while this one disables much more. Is that not the case?

Oh sorry I misspoke - it is not the case that skipping framework negative caching that fixed the problem. It is the other case in shouldCacheStatFailures that fixed the build issue, namely, the case where the extension is empty. When I applied swiftlang#9076 for testing I added shouldCacheStatFailures back, which included the directory caching part. So it was not correct to say that swiftlang#9076 fixed the problem, but rather, an older compiler never had the problem because it did not have 55323ca. In other words, it seems that 55323ca is the commit that caused the build failure.

[jansvoboda11]

The change itself makes sense to me. But AFAIK this is a workaround for misconfigured Xcode projects, so I suggest we carry this patch downstream (and possibly only in the current release branch while working to fix the build system or projects in the next release).

[cyndyishida]

Talked a bit offline, but the patch LGTM, assuming it is purely restoring the previous behavior before 55323ca, which would result in performance parity with the release compiler.

I suggest we carry this patch downstream (and possibly only in the current release branch while working to fix the build system or projects in the next release).

I don't have a strong opinion on whether it's merged here or in the fork. But I do believe all changes should go to the development branch too, to avoid the human cost of investigating the same issue as if it's an independently new one. When there's a resolution to the underlying cause, reverts should go to both places as well.

[qiongsiwu]

Thanks both for comments! I am closing this PR in favor of implementing a downstream only change.