https://github.com/jansvoboda11 created
https://github.com/llvm/llvm-project/pull/86347
Since b4c83a13f664582015ea22924b9a0c6290d41f5b, `Preprocessor` and `Lexer` are
aware of the concept of scanning dependency directives. This makes it possible
to scan for them on-demand rather than eagerly on the first filesystem
operation (open, or even just stat).
This might improve performance, but is also necessary for the "PCH as module"
mode. Some precompiled header sources use the ".pch" file extension, which
means they were not getting scanned for dependency directives. This was okay
when the PCH was the main input file in a separate scan step, because there we
just lex the file in a scanning-specific frontend action. But when such source
gets treated as a module implicitly loaded from a TU, it will get compiled as
any other module - with Sema - which will result in compilation errors. (See
attached test case.)
>From 4bf2dd2d760e79464f25e9cc97487ddbebf0ceba Mon Sep 17 00:00:00 2001
From: Jan Svoboda <jan_svob...@apple.com>
Date: Fri, 22 Mar 2024 13:29:43 -0700
Subject: [PATCH] [clang][deps] Lazy dependency directives
Since b4c83a13f664582015ea22924b9a0c6290d41f5b, `Preprocessor` and `Lexer` are
aware of the concept of scanning dependency directives. This makes it possible
to scan for them on-demand rather than eagerly on the first filesystem
operation (open, or even just stat).
This might improve performance, but is also necessary for the "PCH as module"
mode. Some precompiled header sources use the ".pch" file extension, which
means they were not getting scanned for dependency directives. This was okay
when the PCH was the main input file in a separate scan step, because there we
just lex the file in a scanning-specific frontend action. But when such source
gets treated as a module implicitly loaded from a TU, it will get compiled as
any other module - with Sema - which will result in compilation errors. (See
attached test case.)
---
.../DependencyScanningFilesystem.h | 18 +++---
.../DependencyScanningFilesystem.cpp | 62 +++++--------------
.../DependencyScanningWorker.cpp | 4 +-
clang/test/ClangScanDeps/modules-extension.c | 33 ++++++++++
4 files changed, 58 insertions(+), 59 deletions(-)
create mode 100644 clang/test/ClangScanDeps/modules-extension.c
diff --git
a/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
b/clang/include/clang/Tooling/DependencyScanning/DependencyScanningFilesystem.h
index 846fdc7253977f..870ef8b2f45bb0 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,13 @@ class DependencyScanningWorkerFilesystem
///
/// Attempts to use the local and shared caches first, then falls back to
/// using the underlying filesystem.
- llvm::ErrorOr<EntryRef>
- getOrCreateFileSystemEntry(StringRef Filename,
- bool DisableDirectivesScanning = false);
+ llvm::ErrorOr<EntryRef> getOrCreateFileSystemEntry(StringRef Filename);
-private:
- /// Check whether the file should be scanned for preprocessor directives.
- bool shouldScanForDirectives(StringRef Filename);
+ /// Scan for preprocessor directives for the given entry if necessary and
+ /// returns a wrapper object with reference semantics.
+ bool scanForDirectives(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 +318,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 1b750cec41e1cc..8fd1990440bbb2 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningFilesystem.cpp
@@ -41,24 +41,24 @@ 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::scanForDirectives(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<std::mutex> GuardLock(Contents->ValueLock);
// Double-checked locking.
if (Contents->DepDirectives.load())
- return EntryRef(Filename, Entry);
+ return true;
SmallVector<dependency_directives_scan::Directive, 64> Directives;
// Scan the file for preprocessor directives that might affect the
@@ -69,16 +69,16 @@ EntryRef
DependencyScanningWorkerFilesystem::scanForDirectivesIfNecessary(
Contents->DepDirectiveTokens.clear();
// FIXME: Propagate the diagnostic if desired by the client.
Contents->DepDirectives.store(new std::optional<DependencyDirectivesTy>());
- 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<DependencyDirectivesTy>(std::move(Directives)));
- return EntryRef(Filename, Entry);
+ return true;
}
DependencyScanningFilesystemSharedCache::
@@ -161,34 +161,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<bool>(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 +178,6 @@
DependencyScanningWorkerFilesystem::DependencyScanningWorkerFilesystem(
updateWorkingDirForCacheLookup();
}
-bool DependencyScanningWorkerFilesystem::shouldScanForDirectives(
- StringRef Filename) {
- return shouldScanForDirectivesBasedOnExtension(Filename);
-}
-
const CachedFileSystemEntry &
DependencyScanningWorkerFilesystem::getOrEmplaceSharedEntryForUID(
TentativeEntry TEntry) {
@@ -259,7 +231,7 @@ DependencyScanningWorkerFilesystem::computeAndStoreResult(
llvm::ErrorOr<EntryRef>
DependencyScanningWorkerFilesystem::getOrCreateFileSystemEntry(
- StringRef OriginalFilename, bool DisableDirectivesScanning) {
+ StringRef OriginalFilename) {
StringRef FilenameForLookup;
SmallString<256> PathBuf;
if (llvm::sys::path::is_absolute_gnu(OriginalFilename)) {
@@ -276,15 +248,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<llvm::vfs::Status>
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
index 76f3d950a13b81..f240573c3437ba 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -371,8 +371,10 @@ class DependencyScanningAction : public
tooling::ToolAction {
[LocalDepFS = std::move(LocalDepFS)](FileEntryRef File)
-> std::optional<ArrayRef<dependency_directives_scan::Directive>> {
if (llvm::ErrorOr<EntryRef> Entry =
- LocalDepFS->getOrCreateFileSystemEntry(File.getName()))
+ LocalDepFS->getOrCreateFileSystemEntry(File.getName())) {
+ LocalDepFS->scanForDirectives(*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 00000000000000..0f27f608440f45
--- /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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
