diff --git a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h index 846fdc7253977..9a522a3e2fe25 100644 --- a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h +++ b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h @@ -242,6 +242,8 @@ class EntryRef { /// The underlying cached entry. const CachedFileSystemEntry &Entry; + friend class DependencyScanningWorkerFilesystem; + public: EntryRef(StringRef Name, const CachedFileSystemEntry &Entry) : Filename(Name), Entry(Entry) {} @@ -300,14 +302,15 @@ class DependencyScanningWorkerFilesystem /// /// Attempts to use the local and shared caches first, then falls back to /// using the underlying filesystem. - llvm::ErrorOr - getOrCreateFileSystemEntry(StringRef Filename, - bool DisableDirectivesScanning = false); + llvm::ErrorOr getOrCreateFileSystemEntry(StringRef Filename); -private: - /// Check whether the file should be scanned for preprocessor directives. - bool shouldScanForDirectives(StringRef Filename); + /// Ensure the directive tokens are populated for this file entry. + /// + /// Returns true if the directive tokens are populated for this file entry, + /// false if not (i.e. this entry is not a file or its scan fails). + bool ensureDirectiveTokensArePopulated(EntryRef Entry); +private: /// For a filename that's not yet associated with any entry in the caches, /// uses the underlying filesystem to either look up the entry based in the /// shared cache indexed by unique ID, or creates new entry from scratch. @@ -317,11 +320,6 @@ class DependencyScanningWorkerFilesystem computeAndStoreResult(StringRef OriginalFilename, StringRef FilenameForLookup); - /// Scan for preprocessor directives for the given entry if necessary and - /// returns a wrapper object with reference semantics. - EntryRef scanForDirectivesIfNecessary(const CachedFileSystemEntry &Entry, - StringRef Filename, bool Disable); - /// Represents a filesystem entry that has been stat-ed (and potentially read) /// and that's about to be inserted into the cache as `CachedFileSystemEntry`. struct TentativeEntry { diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp index 1b750cec41e1c..9b7812a1adb9e 100644 --- a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp +++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp @@ -41,24 +41,25 @@ DependencyScanningWorkerFilesystem::readFile(StringRef Filename) { return TentativeEntry(Stat, std::move(Buffer)); } -EntryRef DependencyScanningWorkerFilesystem::scanForDirectivesIfNecessary( - const CachedFileSystemEntry &Entry, StringRef Filename, bool Disable) { - if (Entry.isError() || Entry.isDirectory() || Disable || - !shouldScanForDirectives(Filename)) - return EntryRef(Filename, Entry); +bool DependencyScanningWorkerFilesystem::ensureDirectiveTokensArePopulated( + EntryRef Ref) { + auto &Entry = Ref.Entry; + + if (Entry.isError() || Entry.isDirectory()) + return false; CachedFileContents *Contents = Entry.getCachedContents(); assert(Contents && "contents not initialized"); // Double-checked locking. if (Contents->DepDirectives.load()) - return EntryRef(Filename, Entry); + return true; std::lock_guard GuardLock(Contents->ValueLock); // Double-checked locking. if (Contents->DepDirectives.load()) - return EntryRef(Filename, Entry); + return true; SmallVector Directives; // Scan the file for preprocessor directives that might affect the @@ -69,16 +70,16 @@ EntryRef DependencyScanningWorkerFilesystem::scanForDirectivesIfNecessary( Contents->DepDirectiveTokens.clear(); // FIXME: Propagate the diagnostic if desired by the client. Contents->DepDirectives.store(new std::optional()); - return EntryRef(Filename, Entry); + return false; } // This function performed double-checked locking using `DepDirectives`. // Assigning it must be the last thing this function does, otherwise other - // threads may skip the - // critical section (`DepDirectives != nullptr`), leading to a data race. + // threads may skip the critical section (`DepDirectives != nullptr`), leading + // to a data race. Contents->DepDirectives.store( new std::optional(std::move(Directives))); - return EntryRef(Filename, Entry); + return true; } DependencyScanningFilesystemSharedCache:: @@ -161,34 +162,11 @@ DependencyScanningFilesystemSharedCache::CacheShard:: return *EntriesByFilename.insert({Filename, &Entry}).first->getValue(); } -/// Whitelist file extensions that should be minimized, treating no extension as -/// a source file that should be minimized. -/// -/// This is kinda hacky, it would be better if we knew what kind of file Clang -/// was expecting instead. -static bool shouldScanForDirectivesBasedOnExtension(StringRef Filename) { - StringRef Ext = llvm::sys::path::extension(Filename); - if (Ext.empty()) - return true; // C++ standard library - return llvm::StringSwitch(Ext) - .CasesLower(".c", ".cc", ".cpp", ".c++", ".cxx", true) - .CasesLower(".h", ".hh", ".hpp", ".h++", ".hxx", true) - .CasesLower(".m", ".mm", true) - .CasesLower(".i", ".ii", ".mi", ".mmi", true) - .CasesLower(".def", ".inc", true) - .Default(false); -} - static bool shouldCacheStatFailures(StringRef Filename) { StringRef Ext = llvm::sys::path::extension(Filename); if (Ext.empty()) return false; // This may be the module cache directory. - // Only cache stat failures on files that are not expected to change during - // the build. - StringRef FName = llvm::sys::path::filename(Filename); - if (FName == "module.modulemap" || FName == "module.map") - return true; - return shouldScanForDirectivesBasedOnExtension(Filename); + return true; } DependencyScanningWorkerFilesystem::DependencyScanningWorkerFilesystem( @@ -201,11 +179,6 @@ DependencyScanningWorkerFilesystem::DependencyScanningWorkerFilesystem( updateWorkingDirForCacheLookup(); } -bool DependencyScanningWorkerFilesystem::shouldScanForDirectives( - StringRef Filename) { - return shouldScanForDirectivesBasedOnExtension(Filename); -} - const CachedFileSystemEntry & DependencyScanningWorkerFilesystem::getOrEmplaceSharedEntryForUID( TentativeEntry TEntry) { @@ -259,7 +232,7 @@ DependencyScanningWorkerFilesystem::computeAndStoreResult( llvm::ErrorOr DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry( - StringRef OriginalFilename, bool DisableDirectivesScanning) { + StringRef OriginalFilename) { StringRef FilenameForLookup; SmallString<256> PathBuf; if (llvm::sys::path::is_absolute_gnu(OriginalFilename)) { @@ -276,15 +249,11 @@ DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry( assert(llvm::sys::path::is_absolute_gnu(FilenameForLookup)); if (const auto *Entry = findEntryByFilenameWithWriteThrough(FilenameForLookup)) - return scanForDirectivesIfNecessary(*Entry, OriginalFilename, - DisableDirectivesScanning) - .unwrapError(); + return EntryRef(OriginalFilename, *Entry).unwrapError(); auto MaybeEntry = computeAndStoreResult(OriginalFilename, FilenameForLookup); if (!MaybeEntry) return MaybeEntry.getError(); - return scanForDirectivesIfNecessary(*MaybeEntry, OriginalFilename, - DisableDirectivesScanning) - .unwrapError(); + return EntryRef(OriginalFilename, *MaybeEntry).unwrapError(); } llvm::ErrorOr diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp index 76f3d950a13b8..33b43417a6613 100644 --- a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp +++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp @@ -372,7 +372,8 @@ class DependencyScanningAction : public tooling::ToolAction { -> std::optional> { if (llvm::ErrorOr Entry = LocalDepFS->getOrCreateFileSystemEntry(File.getName())) - return Entry->getDirectiveTokens(); + if (LocalDepFS->ensureDirectiveTokensArePopulated(*Entry)) + return Entry->getDirectiveTokens(); return std::nullopt; }; } diff --git a/clang/test/ClangScanDeps/modules-extension.c b/clang/test/ClangScanDeps/modules-extension.c new file mode 100644 index 0000000000000..0f27f608440f4 --- /dev/null +++ b/clang/test/ClangScanDeps/modules-extension.c @@ -0,0 +1,33 @@ +// RUN: rm -rf %t +// RUN: split-file %s %t + +// This test checks that source files with uncommon extensions still undergo +// dependency directives scan. If header.pch would not and b.h would, the scan +// would fail when parsing `void function(B)` and not knowing the symbol B. + +//--- module.modulemap +module __PCH { header "header.pch" } +module B { header "b.h" } + +//--- header.pch +#include "b.h" +void function(B); + +//--- b.h +typedef int B; + +//--- tu.c +int main() { + function(0); + return 0; +} + +//--- cdb.json.in +[{ + "directory": "DIR", + "file": "DIR/tu.c", + "command": "clang -c DIR/tu.c -fmodules -fmodules-cache-path=DIR/cache -fimplicit-module-maps -include DIR/header.pch" +}] + +// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.in > %t/cdb.json +// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full > %t/deps.json