diff --git a/clang/include/clang-c/Dependencies.h b/clang/include/clang-c/Dependencies.h index 3f7e4495d1873..fd63d6209a97a 100644 --- a/clang/include/clang-c/Dependencies.h +++ b/clang/include/clang-c/Dependencies.h @@ -735,6 +735,11 @@ const char *clang_experimental_DepGraph_getTUContextHash(CXDepGraph); CINDEX_LINKAGE CXDiagnosticSet clang_experimental_DepGraph_getDiagnostics(CXDepGraph); +CINDEX_LINKAGE +CXCStringArray + clang_experimental_DependencyScannerService_getInvalidNegStatCachedPaths( + CXDependencyScannerService); + /** * @} */ diff --git a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h index 9cc4776916d2f..21d4f01739e61 100644 --- a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h +++ b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h @@ -233,6 +233,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 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 + getInvalidNegativeStatCachedPaths(llvm::vfs::FileSystem &UnderlyingFS) const; + private: std::unique_ptr CacheShards; unsigned NumShards; diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp index 9f3ad9c1b2b7e..9916550a28aff 100644 --- a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp +++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp @@ -113,6 +113,33 @@ DependencyScanningFilesystemSharedCache::getShardForUID( return CacheShards[Hash % NumShards]; } +std::vector +DependencyScanningFilesystemSharedCache::getInvalidNegativeStatCachedPaths( + llvm::vfs::FileSystem &UnderlyingFS) const { + // Iterate through all shards and look for cached stat errors. + std::vector InvalidPaths; + for (unsigned i = 0; i < NumShards; i++) { + const CacheShard &Shard = CacheShards[i]; + std::lock_guard LockGuard(Shard.CacheLock); + for (const auto &[Path, CachedPair] : Shard.CacheByFilename) { + const CachedFileSystemEntry *Entry = CachedPair.first; + + if (Entry->getError()) { + // Only examine cached errors. + llvm::ErrorOr Stat = UnderlyingFS.status(Path); + if (Stat) { + // 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 + // 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); + } + } + } + } + return InvalidPaths; +} + const CachedFileSystemEntry * DependencyScanningFilesystemSharedCache::CacheShard::findEntryByFilename( StringRef Filename) const { diff --git a/clang/test/ClangScanDeps/error-c-api.cpp b/clang/test/ClangScanDeps/error-c-api.cpp index b40319fcb6313..cfd1bcbd3a75c 100644 --- a/clang/test/ClangScanDeps/error-c-api.cpp +++ b/clang/test/ClangScanDeps/error-c-api.cpp @@ -4,3 +4,4 @@ // CHECK: error: failed to get dependencies // CHECK-NEXT: 'missing.h' file not found +// CHECK-NEXT: number of invalid negatively stat cached paths: 0 diff --git a/clang/tools/c-index-test/core_main.cpp b/clang/tools/c-index-test/core_main.cpp index 713e8566b96e7..cb2e42af3335d 100644 --- a/clang/tools/c-index-test/core_main.cpp +++ b/clang/tools/c-index-test/core_main.cpp @@ -894,6 +894,14 @@ static int scanDeps(ArrayRef Args, std::string WorkingDirectory, clang_disposeString(Spelling); clang_disposeDiagnostic(Diag); } + + CXCStringArray InvalidNegativeStatCachedPaths = + clang_experimental_DependencyScannerService_getInvalidNegStatCachedPaths( + Service); + + llvm::errs() << "note: number of invalid negatively stat cached paths: " + << InvalidNegativeStatCachedPaths.Count << "\n"; + return 1; } diff --git a/clang/tools/libclang/CDependencies.cpp b/clang/tools/libclang/CDependencies.cpp index 4195b6c609bf7..e559933c4dc35 100644 --- a/clang/tools/libclang/CDependencies.cpp +++ b/clang/tools/libclang/CDependencies.cpp @@ -41,12 +41,57 @@ struct DependencyScannerServiceOptions { ScanningOutputFormat getFormat() const; }; + +struct CStringsManager { + SmallVector>> OwnedCStr; + SmallVector>> OwnedStdStr; + + /// Doesn't own the string contents. + CXCStringArray createCStringsRef(ArrayRef Strings) { + OwnedCStr.push_back(std::make_unique>()); + std::vector &CStrings = *OwnedCStr.back(); + CStrings.reserve(Strings.size()); + for (const auto &String : Strings) + CStrings.push_back(String.c_str()); + return {CStrings.data(), CStrings.size()}; + } + + /// Doesn't own the string contents. + CXCStringArray createCStringsRef(const llvm::StringSet<> &StringsUnordered) { + std::vector Strings; + + for (auto SI = StringsUnordered.begin(), SE = StringsUnordered.end(); + SI != SE; ++SI) + Strings.push_back(SI->getKey()); + + llvm::sort(Strings); + + OwnedCStr.push_back(std::make_unique>()); + std::vector &CStrings = *OwnedCStr.back(); + CStrings.reserve(Strings.size()); + for (const auto &String : Strings) + CStrings.push_back(String.data()); + return {CStrings.data(), CStrings.size()}; + } + + /// Gets ownership of string contents. + CXCStringArray createCStringsOwned(std::vector &&Strings) { + OwnedStdStr.push_back( + std::make_unique>(std::move(Strings))); + return createCStringsRef(*OwnedStdStr.back()); + } +}; + +struct DependencyScannerService { + DependencyScanningService Service; + CStringsManager StrMgr{}; +}; } // end anonymous namespace DEFINE_SIMPLE_CONVERSION_FUNCTIONS(DependencyScannerServiceOptions, CXDependencyScannerServiceOptions) -DEFINE_SIMPLE_CONVERSION_FUNCTIONS(DependencyScanningService, +DEFINE_SIMPLE_CONVERSION_FUNCTIONS(DependencyScannerService, CXDependencyScannerService) DEFINE_SIMPLE_CONVERSION_FUNCTIONS(DependencyScanningWorker, CXDependencyScannerWorker) @@ -142,9 +187,9 @@ clang_experimental_DependencyScannerService_create_v0(CXDependencyMode Format) { // FIXME: Pass default CASOpts and nullptr as CachingOnDiskFileSystem now. CASOptions CASOpts; IntrusiveRefCntPtr FS; - return wrap(new DependencyScanningService( + return wrap(new DependencyScannerService{DependencyScanningService( ScanningMode::DependencyDirectivesScan, unwrap(Format), CASOpts, - /*CAS=*/nullptr, /*ActionCache=*/nullptr, FS)); + /*CAS=*/nullptr, /*ActionCache=*/nullptr, FS)}); } ScanningOutputFormat DependencyScannerServiceOptions::getFormat() const { @@ -180,10 +225,10 @@ clang_experimental_DependencyScannerService_create_v1( FS = llvm::cantFail( llvm::cas::createCachingOnDiskFileSystem(CAS)); } - return wrap(new DependencyScanningService( + return wrap(new DependencyScannerService{DependencyScanningService( ScanningMode::DependencyDirectivesScan, Format, unwrap(Opts)->CASOpts, std::move(CAS), std::move(Cache), std::move(FS), - unwrap(Opts)->OptimizeArgs)); + unwrap(Opts)->OptimizeArgs)}); } void clang_experimental_DependencyScannerService_dispose_v0( @@ -213,17 +258,17 @@ void clang_experimental_FileDependenciesList_dispose( } CXDependencyScannerWorker clang_experimental_DependencyScannerWorker_create_v0( - CXDependencyScannerService Service) { - ScanningOutputFormat Format = unwrap(Service)->getFormat(); + CXDependencyScannerService S) { + ScanningOutputFormat Format = unwrap(S)->Service.getFormat(); bool IsIncludeTreeOutput = Format == ScanningOutputFormat::IncludeTree || Format == ScanningOutputFormat::FullIncludeTree; llvm::IntrusiveRefCntPtr FS = llvm::vfs::createPhysicalFileSystem(); if (IsIncludeTreeOutput) - FS = llvm::cas::createCASProvidingFileSystem(unwrap(Service)->getCAS(), + FS = llvm::cas::createCASProvidingFileSystem(unwrap(S)->Service.getCAS(), std::move(FS)); - return wrap(new DependencyScanningWorker(*unwrap(Service), FS)); + return wrap(new DependencyScanningWorker(unwrap(S)->Service, FS)); } void clang_experimental_DependencyScannerWorker_dispose_v0( @@ -432,46 +477,6 @@ struct DependencyScannerWorkerScanSettings { MLO; }; -struct CStringsManager { - SmallVector>> OwnedCStr; - SmallVector>> OwnedStdStr; - - /// Doesn't own the string contents. - CXCStringArray createCStringsRef(ArrayRef Strings) { - OwnedCStr.push_back(std::make_unique>()); - std::vector &CStrings = *OwnedCStr.back(); - CStrings.reserve(Strings.size()); - for (const auto &String : Strings) - CStrings.push_back(String.c_str()); - return {CStrings.data(), CStrings.size()}; - } - - /// Doesn't own the string contents. - CXCStringArray createCStringsRef(const llvm::StringSet<> &StringsUnordered) { - std::vector Strings; - - for (auto SI = StringsUnordered.begin(), SE = StringsUnordered.end(); - SI != SE; ++SI) - Strings.push_back(SI->getKey()); - - llvm::sort(Strings); - - OwnedCStr.push_back(std::make_unique>()); - std::vector &CStrings = *OwnedCStr.back(); - CStrings.reserve(Strings.size()); - for (const auto &String : Strings) - CStrings.push_back(String.data()); - return {CStrings.data(), CStrings.size()}; - } - - /// Gets ownership of string contents. - CXCStringArray createCStringsOwned(std::vector &&Strings) { - OwnedStdStr.push_back( - std::make_unique>(std::move(Strings))); - return createCStringsRef(*OwnedStdStr.back()); - } -}; - struct DependencyGraph { TranslationUnitDeps TUDeps; SmallString<256> SerialDiagBuf; @@ -731,6 +736,38 @@ CXDiagnosticSet clang_experimental_DepGraph_getDiagnostics(CXDepGraph Graph) { return unwrap(Graph)->getDiagnosticSet(); } +CXCStringArray +clang_experimental_DependencyScannerService_getInvalidNegStatCachedPaths( + CXDependencyScannerService S) { + DependencyScanningService &Service = unwrap(S)->Service; + CStringsManager &StrMgr = unwrap(S)->StrMgr; + + // FIXME: CAS currently does not use the shared cache, and cannot produce + // the same diagnostics. We should add such a diagnostics to CAS as well. + if (Service.useCASFS()) + return {nullptr, 0}; + + DependencyScanningFilesystemSharedCache &SharedCache = + Service.getSharedCache(); + + // Note that it is critical that this FS is the same as the default virtual + // file system we pass to the DependencyScanningWorkers. + llvm::IntrusiveRefCntPtr FS = + llvm::vfs::createPhysicalFileSystem(); + + auto InvaidNegStatCachedPaths = + SharedCache.getInvalidNegativeStatCachedPaths(*FS); + + // FIXME: This code here creates copies of strings from + // InvaidNegStatCachedPaths. It is acceptable because this C-API is expected + // to be called only at the end of a CXDependencyScannerService's lifetime. + // In other words, it is called very infrequently. We can change + // CStringsManager's interface to accommodate handling arbitrary StringRefs + // (which may not be null terminated) if we want to avoid copying. + return StrMgr.createCStringsOwned( + {InvaidNegStatCachedPaths.begin(), InvaidNegStatCachedPaths.end()}); +} + static std::string lookupModuleOutput(const ModuleDeps &MD, ModuleOutputKind MOK, void *MLOContext, std::variant &Strings) { CXStringSet *createSet(const llvm::StringSet<> &StringsUnordered) { std::vector Strings; - - for (auto SI = StringsUnordered.begin(), - SE = StringsUnordered.end(); SI != SE; ++SI) + + for (auto SI = StringsUnordered.begin(), SE = StringsUnordered.end(); + SI != SE; ++SI) Strings.push_back(SI->getKey()); - + llvm::sort(Strings); - + CXStringSet *Set = new CXStringSet; Set->Count = Strings.size(); Set->Strings = new CXString[Set->Count]; diff --git a/clang/tools/libclang/libclang.map b/clang/tools/libclang/libclang.map index d59a68582b12b..63753a8afe98f 100644 --- a/clang/tools/libclang/libclang.map +++ b/clang/tools/libclang/libclang.map @@ -575,6 +575,7 @@ LLVM_21 { clang_experimental_DependencyScannerServiceOptions_setCWDOptimization; clang_experimental_DepGraphModule_isCWDIgnored; clang_experimental_DepGraphModule_isInStableDirs; + clang_experimental_DependencyScannerService_getInvalidNegStatCachedPaths; }; # Example of how to add a new symbol version entry. If you do add a new symbol diff --git a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp index 691cd3641eb04..772804de5b8b5 100644 --- a/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp +++ b/clang/unittests/Tooling/DependencyScanning/DependencyScanningFilesystemTest.cpp @@ -146,3 +146,27 @@ TEST(DependencyScanningFilesystem, CacheStatOnExists) { EXPECT_EQ(InstrumentingFS->NumStatusCalls, 2u); EXPECT_EQ(InstrumentingFS->NumExistsCalls, 0u); } + +TEST(DependencyScanningFilesystem, DiagnoseStaleStatFailures) { + auto InMemoryFS = llvm::makeIntrusiveRefCnt(); + InMemoryFS->setCurrentWorkingDirectory("/"); + + DependencyScanningFilesystemSharedCache SharedCache; + DependencyScanningWorkerFilesystem DepFS(SharedCache, InMemoryFS); + + bool Path1Exists = DepFS.exists("/path1.suffix"); + EXPECT_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); + + std::vector InvalidPaths = + SharedCache.getInvalidNegativeStatCachedPaths(*InMemoryFS.get()); + + EXPECT_EQ(InvalidPaths.size(), 1u); + ASSERT_STREQ("/path1.suffix", InvalidPaths[0].str().c_str()); +}