[clang] [clang] Move state out of `PreprocessorOptions` (1/n) (PR #86358)
Jan Svoboda via cfe-commits
cfe-commits at lists.llvm.org
Fri Mar 29 11:20:49 PDT 2024
https://github.com/jansvoboda11 updated https://github.com/llvm/llvm-project/pull/86358
>From 109e6039abe8bd3b598f1fdc2dbd7bca783ff4e6 Mon Sep 17 00:00:00 2001
From: Jan Svoboda <jan_svoboda at apple.com>
Date: Fri, 22 Mar 2024 15:54:14 -0700
Subject: [PATCH 1/5] [clang] Move state out of `PreprocessorOptions` (1/n)
An instance of `PreprocessorOptions` is part of `CompilerInvocation` which is supposed to be a value type. The `DependencyDirectivesForFile` member is problematic, since it holds an owning reference of the scanning VFS. This makes it not a true value type, and it can keep potentially large chunk of memory (the local cache in the scanning VFS) alive for longer than clients might expect. Let's move it into the `Preprocessor` instead.
---
.../include/clang/Frontend/FrontendActions.h | 12 +++++++++--
clang/include/clang/Lex/Preprocessor.h | 18 ++++++++++++++++
clang/include/clang/Lex/PreprocessorOptions.h | 13 ------------
clang/lib/Frontend/FrontendActions.cpp | 7 ++++++-
clang/lib/Lex/PPLexerChange.cpp | 12 +++--------
.../DependencyScanningWorker.cpp | 21 +++++++++++--------
6 files changed, 49 insertions(+), 34 deletions(-)
diff --git a/clang/include/clang/Frontend/FrontendActions.h b/clang/include/clang/Frontend/FrontendActions.h
index a620ddfc40447d..575cf9ed2ac777 100644
--- a/clang/include/clang/Frontend/FrontendActions.h
+++ b/clang/include/clang/Frontend/FrontendActions.h
@@ -34,12 +34,17 @@ class InitOnlyAction : public FrontendAction {
/// Preprocessor-based frontend action that also loads PCH files.
class ReadPCHAndPreprocessAction : public FrontendAction {
+ llvm::function_ref<void(CompilerInstance &)> OnCI;
+
void ExecuteAction() override;
std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
StringRef InFile) override;
public:
+ ReadPCHAndPreprocessAction(llvm::function_ref<void(CompilerInstance &)> OnCI)
+ : OnCI(OnCI) {}
+
bool usesPreprocessorOnly() const override { return false; }
};
@@ -321,11 +326,14 @@ class PrintPreprocessedAction : public PreprocessorFrontendAction {
class GetDependenciesByModuleNameAction : public PreprocessOnlyAction {
StringRef ModuleName;
+ llvm::function_ref<void(CompilerInstance &)> OnCI;
+
void ExecuteAction() override;
public:
- GetDependenciesByModuleNameAction(StringRef ModuleName)
- : ModuleName(ModuleName) {}
+ GetDependenciesByModuleNameAction(
+ StringRef ModuleName, llvm::function_ref<void(CompilerInstance &)> OnCI)
+ : ModuleName(ModuleName), OnCI(OnCI) {}
};
} // end namespace clang
diff --git a/clang/include/clang/Lex/Preprocessor.h b/clang/include/clang/Lex/Preprocessor.h
index 0836b7d439bb04..10a7ba4db25625 100644
--- a/clang/include/clang/Lex/Preprocessor.h
+++ b/clang/include/clang/Lex/Preprocessor.h
@@ -736,6 +736,19 @@ class Preprocessor {
State ConditionalStackState = Off;
} PreambleConditionalStack;
+ /// Function for getting the dependency preprocessor directives of a file.
+ ///
+ /// These are directives derived from a special form of lexing where the
+ /// source input is scanned for the preprocessor directives that might have an
+ /// effect on the dependencies for a compilation unit.
+ ///
+ /// Enables a client to cache the directives for a file and provide them
+ /// across multiple compiler invocations.
+ /// FIXME: Allow returning an error.
+ using DependencyDirectivesFn = std::function<std::optional<
+ ArrayRef<dependency_directives_scan::Directive>>(FileEntryRef)>;
+ DependencyDirectivesFn DependencyDirectivesForFile;
+
/// The current top of the stack that we're lexing from if
/// not expanding a macro and we are lexing directly from source code.
///
@@ -1270,6 +1283,11 @@ class Preprocessor {
/// false if it is producing tokens to be consumed by Parse and Sema.
bool isPreprocessedOutput() const { return PreprocessedOutput; }
+ /// Set the function used to get dependency directives for a file.
+ void setDependencyDirectivesFn(DependencyDirectivesFn Fn) {
+ DependencyDirectivesForFile = std::move(Fn);
+ }
+
/// Return true if we are lexing directly from the specified lexer.
bool isCurrentLexer(const PreprocessorLexer *L) const {
return CurPPLexer == L;
diff --git a/clang/include/clang/Lex/PreprocessorOptions.h b/clang/include/clang/Lex/PreprocessorOptions.h
index f841e4a028df50..24e0ca61d41432 100644
--- a/clang/include/clang/Lex/PreprocessorOptions.h
+++ b/clang/include/clang/Lex/PreprocessorOptions.h
@@ -208,19 +208,6 @@ class PreprocessorOptions {
/// build it again.
std::shared_ptr<FailedModulesSet> FailedModules;
- /// Function for getting the dependency preprocessor directives of a file.
- ///
- /// These are directives derived from a special form of lexing where the
- /// source input is scanned for the preprocessor directives that might have an
- /// effect on the dependencies for a compilation unit.
- ///
- /// Enables a client to cache the directives for a file and provide them
- /// across multiple compiler invocations.
- /// FIXME: Allow returning an error.
- std::function<std::optional<ArrayRef<dependency_directives_scan::Directive>>(
- FileEntryRef)>
- DependencyDirectivesForFile;
-
/// Set up preprocessor for RunAnalysis action.
bool SetUpStaticAnalyzer = false;
diff --git a/clang/lib/Frontend/FrontendActions.cpp b/clang/lib/Frontend/FrontendActions.cpp
index 3fd1cdd3b47942..b31e877b9f11f8 100644
--- a/clang/lib/Frontend/FrontendActions.cpp
+++ b/clang/lib/Frontend/FrontendActions.cpp
@@ -69,7 +69,10 @@ void InitOnlyAction::ExecuteAction() {
// Basically PreprocessOnlyAction::ExecuteAction.
void ReadPCHAndPreprocessAction::ExecuteAction() {
- Preprocessor &PP = getCompilerInstance().getPreprocessor();
+ CompilerInstance &CI = getCompilerInstance();
+ OnCI(CI);
+
+ Preprocessor &PP = CI.getPreprocessor();
// Ignore unknown pragmas.
PP.IgnorePragmas();
@@ -1188,6 +1191,8 @@ void PrintDependencyDirectivesSourceMinimizerAction::ExecuteAction() {
void GetDependenciesByModuleNameAction::ExecuteAction() {
CompilerInstance &CI = getCompilerInstance();
+ OnCI(CI);
+
Preprocessor &PP = CI.getPreprocessor();
SourceManager &SM = PP.getSourceManager();
FileID MainFileID = SM.getMainFileID();
diff --git a/clang/lib/Lex/PPLexerChange.cpp b/clang/lib/Lex/PPLexerChange.cpp
index 3b1b6df1dbae4e..a0cc2b516574c1 100644
--- a/clang/lib/Lex/PPLexerChange.cpp
+++ b/clang/lib/Lex/PPLexerChange.cpp
@@ -93,16 +93,10 @@ bool Preprocessor::EnterSourceFile(FileID FID, ConstSearchDirIterator CurDir,
}
Lexer *TheLexer = new Lexer(FID, *InputFile, *this, IsFirstIncludeOfFile);
- if (getPreprocessorOpts().DependencyDirectivesForFile &&
- FID != PredefinesFileID) {
- if (OptionalFileEntryRef File = SourceMgr.getFileEntryRefForID(FID)) {
- if (std::optional<ArrayRef<dependency_directives_scan::Directive>>
- DepDirectives =
- getPreprocessorOpts().DependencyDirectivesForFile(*File)) {
+ if (DependencyDirectivesForFile && FID != PredefinesFileID)
+ if (OptionalFileEntryRef File = SourceMgr.getFileEntryRefForID(FID))
+ if (auto DepDirectives = DependencyDirectivesForFile(*File))
TheLexer->DepDirectives = *DepDirectives;
- }
- }
- }
EnterSourceFileWithLexer(TheLexer, CurDir);
return false;
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
index 33b43417a6613d..d62014a4434bd0 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -363,12 +363,12 @@ class DependencyScanningAction : public tooling::ToolAction {
PrebuiltModuleVFSMap, ScanInstance.getDiagnostics()))
return false;
- // Use the dependency scanning optimized file system if requested to do so.
- if (DepFS) {
- llvm::IntrusiveRefCntPtr<DependencyScanningWorkerFilesystem> LocalDepFS =
- DepFS;
- ScanInstance.getPreprocessorOpts().DependencyDirectivesForFile =
- [LocalDepFS = std::move(LocalDepFS)](FileEntryRef File)
+ // Set up the dependency scanning file system callback if requested.
+ auto AdjustCI = [&](CompilerInstance &CI) {
+ if (!DepFS)
+ return;
+
+ auto GetDependencyDirectives = [LocalDepFS = DepFS](FileEntryRef File)
-> std::optional<ArrayRef<dependency_directives_scan::Directive>> {
if (llvm::ErrorOr<EntryRef> Entry =
LocalDepFS->getOrCreateFileSystemEntry(File.getName()))
@@ -376,7 +376,9 @@ class DependencyScanningAction : public tooling::ToolAction {
return Entry->getDirectiveTokens();
return std::nullopt;
};
- }
+
+ CI.getPreprocessor().setDependencyDirectivesFn(GetDependencyDirectives);
+ };
// Create the dependency collector that will collect the produced
// dependencies.
@@ -428,9 +430,10 @@ class DependencyScanningAction : public tooling::ToolAction {
std::unique_ptr<FrontendAction> Action;
if (ModuleName)
- Action = std::make_unique<GetDependenciesByModuleNameAction>(*ModuleName);
+ Action = std::make_unique<GetDependenciesByModuleNameAction>(*ModuleName,
+ AdjustCI);
else
- Action = std::make_unique<ReadPCHAndPreprocessAction>();
+ Action = std::make_unique<ReadPCHAndPreprocessAction>(AdjustCI);
if (ScanInstance.getDiagnostics().hasErrorOccurred())
return false;
>From 1337dcf5fa8891c37e10f1370880c60d1deda303 Mon Sep 17 00:00:00 2001
From: Jan Svoboda <jan_svoboda at apple.com>
Date: Fri, 22 Mar 2024 16:52:07 -0700
Subject: [PATCH 2/5] Fix unit test
---
clang/unittests/Lex/PPDependencyDirectivesTest.cpp | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/clang/unittests/Lex/PPDependencyDirectivesTest.cpp b/clang/unittests/Lex/PPDependencyDirectivesTest.cpp
index 6ff87f720a559f..5fbda20911f057 100644
--- a/clang/unittests/Lex/PPDependencyDirectivesTest.cpp
+++ b/clang/unittests/Lex/PPDependencyDirectivesTest.cpp
@@ -117,11 +117,6 @@ TEST_F(PPDependencyDirectivesTest, MacroGuard) {
};
auto PPOpts = std::make_shared<PreprocessorOptions>();
- PPOpts->DependencyDirectivesForFile = [&](FileEntryRef File)
- -> std::optional<ArrayRef<dependency_directives_scan::Directive>> {
- return getDependencyDirectives(File);
- };
-
TrivialModuleLoader ModLoader;
HeaderSearch HeaderInfo(std::make_shared<HeaderSearchOptions>(), SourceMgr,
Diags, LangOpts, Target.get());
@@ -130,6 +125,11 @@ TEST_F(PPDependencyDirectivesTest, MacroGuard) {
/*OwnsHeaderSearch =*/false);
PP.Initialize(*Target);
+ PP.setDependencyDirectivesFn([&](FileEntryRef File)
+ -> std::optional<ArrayRef<dependency_directives_scan::Directive>> {
+ return getDependencyDirectives(File);
+ });
+
SmallVector<StringRef> IncludedFiles;
PP.addPPCallbacks(std::make_unique<IncludeCollector>(PP, IncludedFiles));
PP.EnterMainSourceFile();
>From 1b3f9de6a6c8e879155f00a4ff8908b196eade8a Mon Sep 17 00:00:00 2001
From: Jan Svoboda <jan_svoboda at apple.com>
Date: Fri, 22 Mar 2024 16:58:43 -0700
Subject: [PATCH 3/5] Formatting
---
clang/unittests/Lex/PPDependencyDirectivesTest.cpp | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/clang/unittests/Lex/PPDependencyDirectivesTest.cpp b/clang/unittests/Lex/PPDependencyDirectivesTest.cpp
index 5fbda20911f057..0c396720ece66d 100644
--- a/clang/unittests/Lex/PPDependencyDirectivesTest.cpp
+++ b/clang/unittests/Lex/PPDependencyDirectivesTest.cpp
@@ -125,10 +125,11 @@ TEST_F(PPDependencyDirectivesTest, MacroGuard) {
/*OwnsHeaderSearch =*/false);
PP.Initialize(*Target);
- PP.setDependencyDirectivesFn([&](FileEntryRef File)
- -> std::optional<ArrayRef<dependency_directives_scan::Directive>> {
- return getDependencyDirectives(File);
- });
+ PP.setDependencyDirectivesFn(
+ [&](FileEntryRef File)
+ -> std::optional<ArrayRef<dependency_directives_scan::Directive>> {
+ return getDependencyDirectives(File);
+ });
SmallVector<StringRef> IncludedFiles;
PP.addPPCallbacks(std::make_unique<IncludeCollector>(PP, IncludedFiles));
>From d5dcbdf2e91225172779165f84ca2d8549a01107 Mon Sep 17 00:00:00 2001
From: Jan Svoboda <jan_svoboda at apple.com>
Date: Thu, 28 Mar 2024 14:38:01 -0700
Subject: [PATCH 4/5] Rename OnCI & use unique_function
---
clang/include/clang/Frontend/FrontendActions.h | 14 ++++++++------
clang/include/clang/Lex/Preprocessor.h | 2 +-
clang/lib/Frontend/FrontendActions.cpp | 4 ++--
.../DependencyScanningWorker.cpp | 10 ++++++----
4 files changed, 17 insertions(+), 13 deletions(-)
diff --git a/clang/include/clang/Frontend/FrontendActions.h b/clang/include/clang/Frontend/FrontendActions.h
index 575cf9ed2ac777..0518a8823a03ea 100644
--- a/clang/include/clang/Frontend/FrontendActions.h
+++ b/clang/include/clang/Frontend/FrontendActions.h
@@ -34,7 +34,7 @@ class InitOnlyAction : public FrontendAction {
/// Preprocessor-based frontend action that also loads PCH files.
class ReadPCHAndPreprocessAction : public FrontendAction {
- llvm::function_ref<void(CompilerInstance &)> OnCI;
+ llvm::unique_function<void(CompilerInstance &)> AdjustCI;
void ExecuteAction() override;
@@ -42,8 +42,9 @@ class ReadPCHAndPreprocessAction : public FrontendAction {
StringRef InFile) override;
public:
- ReadPCHAndPreprocessAction(llvm::function_ref<void(CompilerInstance &)> OnCI)
- : OnCI(OnCI) {}
+ ReadPCHAndPreprocessAction(
+ llvm::unique_function<void(CompilerInstance &)> AdjustCI)
+ : AdjustCI(std::move(AdjustCI)) {}
bool usesPreprocessorOnly() const override { return false; }
};
@@ -326,14 +327,15 @@ class PrintPreprocessedAction : public PreprocessorFrontendAction {
class GetDependenciesByModuleNameAction : public PreprocessOnlyAction {
StringRef ModuleName;
- llvm::function_ref<void(CompilerInstance &)> OnCI;
+ llvm::unique_function<void(CompilerInstance &)> AdjustCI;
void ExecuteAction() override;
public:
GetDependenciesByModuleNameAction(
- StringRef ModuleName, llvm::function_ref<void(CompilerInstance &)> OnCI)
- : ModuleName(ModuleName), OnCI(OnCI) {}
+ StringRef ModuleName,
+ llvm::unique_function<void(CompilerInstance &)> AdjustCI)
+ : ModuleName(ModuleName), AdjustCI(std::move(AdjustCI)) {}
};
} // end namespace clang
diff --git a/clang/include/clang/Lex/Preprocessor.h b/clang/include/clang/Lex/Preprocessor.h
index 10a7ba4db25625..24e146a589a758 100644
--- a/clang/include/clang/Lex/Preprocessor.h
+++ b/clang/include/clang/Lex/Preprocessor.h
@@ -745,7 +745,7 @@ class Preprocessor {
/// Enables a client to cache the directives for a file and provide them
/// across multiple compiler invocations.
/// FIXME: Allow returning an error.
- using DependencyDirectivesFn = std::function<std::optional<
+ using DependencyDirectivesFn = llvm::unique_function<std::optional<
ArrayRef<dependency_directives_scan::Directive>>(FileEntryRef)>;
DependencyDirectivesFn DependencyDirectivesForFile;
diff --git a/clang/lib/Frontend/FrontendActions.cpp b/clang/lib/Frontend/FrontendActions.cpp
index b31e877b9f11f8..0bc26b694cfc8d 100644
--- a/clang/lib/Frontend/FrontendActions.cpp
+++ b/clang/lib/Frontend/FrontendActions.cpp
@@ -70,7 +70,7 @@ void InitOnlyAction::ExecuteAction() {
// Basically PreprocessOnlyAction::ExecuteAction.
void ReadPCHAndPreprocessAction::ExecuteAction() {
CompilerInstance &CI = getCompilerInstance();
- OnCI(CI);
+ AdjustCI(CI);
Preprocessor &PP = CI.getPreprocessor();
@@ -1191,7 +1191,7 @@ void PrintDependencyDirectivesSourceMinimizerAction::ExecuteAction() {
void GetDependenciesByModuleNameAction::ExecuteAction() {
CompilerInstance &CI = getCompilerInstance();
- OnCI(CI);
+ AdjustCI(CI);
Preprocessor &PP = CI.getPreprocessor();
SourceManager &SM = PP.getSourceManager();
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
index d62014a4434bd0..ce5b996d5beb7e 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -377,7 +377,8 @@ class DependencyScanningAction : public tooling::ToolAction {
return std::nullopt;
};
- CI.getPreprocessor().setDependencyDirectivesFn(GetDependencyDirectives);
+ CI.getPreprocessor().setDependencyDirectivesFn(
+ std::move(GetDependencyDirectives));
};
// Create the dependency collector that will collect the produced
@@ -430,10 +431,11 @@ class DependencyScanningAction : public tooling::ToolAction {
std::unique_ptr<FrontendAction> Action;
if (ModuleName)
- Action = std::make_unique<GetDependenciesByModuleNameAction>(*ModuleName,
- AdjustCI);
+ Action = std::make_unique<GetDependenciesByModuleNameAction>(
+ *ModuleName, std::move(AdjustCI));
else
- Action = std::make_unique<ReadPCHAndPreprocessAction>(AdjustCI);
+ Action =
+ std::make_unique<ReadPCHAndPreprocessAction>(std::move(AdjustCI));
if (ScanInstance.getDiagnostics().hasErrorOccurred())
return false;
>From 48c6b54c5f7ffcbe5ceca82eda53273f84f4da28 Mon Sep 17 00:00:00 2001
From: Jan Svoboda <jan_svoboda at apple.com>
Date: Fri, 29 Mar 2024 11:17:04 -0700
Subject: [PATCH 5/5] Invert condition
This will make integration with downstream easier
---
.../DependencyScanningWorker.cpp | 29 +++++++++----------
1 file changed, 14 insertions(+), 15 deletions(-)
diff --git a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
index ce5b996d5beb7e..492b8f1e2b3863 100644
--- a/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ b/clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -363,22 +363,21 @@ class DependencyScanningAction : public tooling::ToolAction {
PrebuiltModuleVFSMap, ScanInstance.getDiagnostics()))
return false;
- // Set up the dependency scanning file system callback if requested.
auto AdjustCI = [&](CompilerInstance &CI) {
- if (!DepFS)
- return;
-
- auto GetDependencyDirectives = [LocalDepFS = DepFS](FileEntryRef File)
- -> std::optional<ArrayRef<dependency_directives_scan::Directive>> {
- if (llvm::ErrorOr<EntryRef> Entry =
- LocalDepFS->getOrCreateFileSystemEntry(File.getName()))
- if (LocalDepFS->ensureDirectiveTokensArePopulated(*Entry))
- return Entry->getDirectiveTokens();
- return std::nullopt;
- };
-
- CI.getPreprocessor().setDependencyDirectivesFn(
- std::move(GetDependencyDirectives));
+ // Set up the dependency scanning file system callback if requested.
+ if (DepFS) {
+ auto GetDependencyDirectives = [LocalDepFS = DepFS](FileEntryRef File)
+ -> std::optional<ArrayRef<dependency_directives_scan::Directive>> {
+ if (llvm::ErrorOr<EntryRef> Entry =
+ LocalDepFS->getOrCreateFileSystemEntry(File.getName()))
+ if (LocalDepFS->ensureDirectiveTokensArePopulated(*Entry))
+ return Entry->getDirectiveTokens();
+ return std::nullopt;
+ };
+
+ CI.getPreprocessor().setDependencyDirectivesFn(
+ std::move(GetDependencyDirectives));
+ }
};
// Create the dependency collector that will collect the produced
More information about the cfe-commits
mailing list