Skip to content

Commit 407a2f2

Browse files
authored
[clang] Move state out of PreprocessorOptions (1/n) (#86358)
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.
1 parent 9937952 commit 407a2f2

File tree

7 files changed

+65
-46
lines changed

7 files changed

+65
-46
lines changed

clang/include/clang/Frontend/FrontendActions.h

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,12 +34,18 @@ class InitOnlyAction : public FrontendAction {
3434

3535
/// Preprocessor-based frontend action that also loads PCH files.
3636
class ReadPCHAndPreprocessAction : public FrontendAction {
37+
llvm::unique_function<void(CompilerInstance &)> AdjustCI;
38+
3739
void ExecuteAction() override;
3840

3941
std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
4042
StringRef InFile) override;
4143

4244
public:
45+
ReadPCHAndPreprocessAction(
46+
llvm::unique_function<void(CompilerInstance &)> AdjustCI)
47+
: AdjustCI(std::move(AdjustCI)) {}
48+
4349
bool usesPreprocessorOnly() const override { return false; }
4450
};
4551

@@ -321,11 +327,15 @@ class PrintPreprocessedAction : public PreprocessorFrontendAction {
321327

322328
class GetDependenciesByModuleNameAction : public PreprocessOnlyAction {
323329
StringRef ModuleName;
330+
llvm::unique_function<void(CompilerInstance &)> AdjustCI;
331+
324332
void ExecuteAction() override;
325333

326334
public:
327-
GetDependenciesByModuleNameAction(StringRef ModuleName)
328-
: ModuleName(ModuleName) {}
335+
GetDependenciesByModuleNameAction(
336+
StringRef ModuleName,
337+
llvm::unique_function<void(CompilerInstance &)> AdjustCI)
338+
: ModuleName(ModuleName), AdjustCI(std::move(AdjustCI)) {}
329339
};
330340

331341
} // end namespace clang

clang/include/clang/Lex/Preprocessor.h

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -736,6 +736,19 @@ class Preprocessor {
736736
State ConditionalStackState = Off;
737737
} PreambleConditionalStack;
738738

739+
/// Function for getting the dependency preprocessor directives of a file.
740+
///
741+
/// These are directives derived from a special form of lexing where the
742+
/// source input is scanned for the preprocessor directives that might have an
743+
/// effect on the dependencies for a compilation unit.
744+
///
745+
/// Enables a client to cache the directives for a file and provide them
746+
/// across multiple compiler invocations.
747+
/// FIXME: Allow returning an error.
748+
using DependencyDirectivesFn = llvm::unique_function<std::optional<
749+
ArrayRef<dependency_directives_scan::Directive>>(FileEntryRef)>;
750+
DependencyDirectivesFn DependencyDirectivesForFile;
751+
739752
/// The current top of the stack that we're lexing from if
740753
/// not expanding a macro and we are lexing directly from source code.
741754
///
@@ -1270,6 +1283,11 @@ class Preprocessor {
12701283
/// false if it is producing tokens to be consumed by Parse and Sema.
12711284
bool isPreprocessedOutput() const { return PreprocessedOutput; }
12721285

1286+
/// Set the function used to get dependency directives for a file.
1287+
void setDependencyDirectivesFn(DependencyDirectivesFn Fn) {
1288+
DependencyDirectivesForFile = std::move(Fn);
1289+
}
1290+
12731291
/// Return true if we are lexing directly from the specified lexer.
12741292
bool isCurrentLexer(const PreprocessorLexer *L) const {
12751293
return CurPPLexer == L;

clang/include/clang/Lex/PreprocessorOptions.h

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -208,19 +208,6 @@ class PreprocessorOptions {
208208
/// build it again.
209209
std::shared_ptr<FailedModulesSet> FailedModules;
210210

211-
/// Function for getting the dependency preprocessor directives of a file.
212-
///
213-
/// These are directives derived from a special form of lexing where the
214-
/// source input is scanned for the preprocessor directives that might have an
215-
/// effect on the dependencies for a compilation unit.
216-
///
217-
/// Enables a client to cache the directives for a file and provide them
218-
/// across multiple compiler invocations.
219-
/// FIXME: Allow returning an error.
220-
std::function<std::optional<ArrayRef<dependency_directives_scan::Directive>>(
221-
FileEntryRef)>
222-
DependencyDirectivesForFile;
223-
224211
/// Set up preprocessor for RunAnalysis action.
225212
bool SetUpStaticAnalyzer = false;
226213

clang/lib/Frontend/FrontendActions.cpp

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,10 @@ void InitOnlyAction::ExecuteAction() {
6969

7070
// Basically PreprocessOnlyAction::ExecuteAction.
7171
void ReadPCHAndPreprocessAction::ExecuteAction() {
72-
Preprocessor &PP = getCompilerInstance().getPreprocessor();
72+
CompilerInstance &CI = getCompilerInstance();
73+
AdjustCI(CI);
74+
75+
Preprocessor &PP = CI.getPreprocessor();
7376

7477
// Ignore unknown pragmas.
7578
PP.IgnorePragmas();
@@ -1188,6 +1191,8 @@ void PrintDependencyDirectivesSourceMinimizerAction::ExecuteAction() {
11881191

11891192
void GetDependenciesByModuleNameAction::ExecuteAction() {
11901193
CompilerInstance &CI = getCompilerInstance();
1194+
AdjustCI(CI);
1195+
11911196
Preprocessor &PP = CI.getPreprocessor();
11921197
SourceManager &SM = PP.getSourceManager();
11931198
FileID MainFileID = SM.getMainFileID();

clang/lib/Lex/PPLexerChange.cpp

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -93,16 +93,10 @@ bool Preprocessor::EnterSourceFile(FileID FID, ConstSearchDirIterator CurDir,
9393
}
9494

9595
Lexer *TheLexer = new Lexer(FID, *InputFile, *this, IsFirstIncludeOfFile);
96-
if (getPreprocessorOpts().DependencyDirectivesForFile &&
97-
FID != PredefinesFileID) {
98-
if (OptionalFileEntryRef File = SourceMgr.getFileEntryRefForID(FID)) {
99-
if (std::optional<ArrayRef<dependency_directives_scan::Directive>>
100-
DepDirectives =
101-
getPreprocessorOpts().DependencyDirectivesForFile(*File)) {
96+
if (DependencyDirectivesForFile && FID != PredefinesFileID)
97+
if (OptionalFileEntryRef File = SourceMgr.getFileEntryRefForID(FID))
98+
if (auto DepDirectives = DependencyDirectivesForFile(*File))
10299
TheLexer->DepDirectives = *DepDirectives;
103-
}
104-
}
105-
}
106100

107101
EnterSourceFileWithLexer(TheLexer, CurDir);
108102
return false;

clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

Lines changed: 20 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -363,20 +363,22 @@ class DependencyScanningAction : public tooling::ToolAction {
363363
PrebuiltModuleVFSMap, ScanInstance.getDiagnostics()))
364364
return false;
365365

366-
// Use the dependency scanning optimized file system if requested to do so.
367-
if (DepFS) {
368-
llvm::IntrusiveRefCntPtr<DependencyScanningWorkerFilesystem> LocalDepFS =
369-
DepFS;
370-
ScanInstance.getPreprocessorOpts().DependencyDirectivesForFile =
371-
[LocalDepFS = std::move(LocalDepFS)](FileEntryRef File)
372-
-> std::optional<ArrayRef<dependency_directives_scan::Directive>> {
373-
if (llvm::ErrorOr<EntryRef> Entry =
374-
LocalDepFS->getOrCreateFileSystemEntry(File.getName()))
375-
if (LocalDepFS->ensureDirectiveTokensArePopulated(*Entry))
376-
return Entry->getDirectiveTokens();
377-
return std::nullopt;
378-
};
379-
}
366+
auto AdjustCI = [&](CompilerInstance &CI) {
367+
// Set up the dependency scanning file system callback if requested.
368+
if (DepFS) {
369+
auto GetDependencyDirectives = [LocalDepFS = DepFS](FileEntryRef File)
370+
-> std::optional<ArrayRef<dependency_directives_scan::Directive>> {
371+
if (llvm::ErrorOr<EntryRef> Entry =
372+
LocalDepFS->getOrCreateFileSystemEntry(File.getName()))
373+
if (LocalDepFS->ensureDirectiveTokensArePopulated(*Entry))
374+
return Entry->getDirectiveTokens();
375+
return std::nullopt;
376+
};
377+
378+
CI.getPreprocessor().setDependencyDirectivesFn(
379+
std::move(GetDependencyDirectives));
380+
}
381+
};
380382

381383
// Create the dependency collector that will collect the produced
382384
// dependencies.
@@ -428,9 +430,11 @@ class DependencyScanningAction : public tooling::ToolAction {
428430
std::unique_ptr<FrontendAction> Action;
429431

430432
if (ModuleName)
431-
Action = std::make_unique<GetDependenciesByModuleNameAction>(*ModuleName);
433+
Action = std::make_unique<GetDependenciesByModuleNameAction>(
434+
*ModuleName, std::move(AdjustCI));
432435
else
433-
Action = std::make_unique<ReadPCHAndPreprocessAction>();
436+
Action =
437+
std::make_unique<ReadPCHAndPreprocessAction>(std::move(AdjustCI));
434438

435439
if (ScanInstance.getDiagnostics().hasErrorOccurred())
436440
return false;

clang/unittests/Lex/PPDependencyDirectivesTest.cpp

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -117,11 +117,6 @@ TEST_F(PPDependencyDirectivesTest, MacroGuard) {
117117
};
118118

119119
auto PPOpts = std::make_shared<PreprocessorOptions>();
120-
PPOpts->DependencyDirectivesForFile = [&](FileEntryRef File)
121-
-> std::optional<ArrayRef<dependency_directives_scan::Directive>> {
122-
return getDependencyDirectives(File);
123-
};
124-
125120
TrivialModuleLoader ModLoader;
126121
HeaderSearch HeaderInfo(std::make_shared<HeaderSearchOptions>(), SourceMgr,
127122
Diags, LangOpts, Target.get());
@@ -130,6 +125,12 @@ TEST_F(PPDependencyDirectivesTest, MacroGuard) {
130125
/*OwnsHeaderSearch =*/false);
131126
PP.Initialize(*Target);
132127

128+
PP.setDependencyDirectivesFn(
129+
[&](FileEntryRef File)
130+
-> std::optional<ArrayRef<dependency_directives_scan::Directive>> {
131+
return getDependencyDirectives(File);
132+
});
133+
133134
SmallVector<StringRef> IncludedFiles;
134135
PP.addPPCallbacks(std::make_unique<IncludeCollector>(PP, IncludedFiles));
135136
PP.EnterMainSourceFile();

0 commit comments

Comments
 (0)