[clang] 8a90491 - [clang] Replace `finish()` with destructors for `DiagnosticConsumer` (#183831)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Mar 2 12:51:58 PST 2026
Author: Jan Svoboda
Date: 2026-03-02T12:51:53-08:00
New Revision: 8a9049198d180956a5f69c567bf235d36cd215a7
URL: https://github.com/llvm/llvm-project/commit/8a9049198d180956a5f69c567bf235d36cd215a7
DIFF: https://github.com/llvm/llvm-project/commit/8a9049198d180956a5f69c567bf235d36cd215a7.diff
LOG: [clang] Replace `finish()` with destructors for `DiagnosticConsumer` (#183831)
The `DiagnosticConsumer::finish()` API has historically been a source of
friction. Lots of different clients must manually ensure it gets called
for all consumers to work correctly. Idiomatic C++ uses destructors for
this. In Clang, there are some cases where destructors don't run
automatically, such as under `-disable-free` or some signal handling
code in `clang_main()`. This PR squeezes the complexity of ensuring
those destructors do run out of library code and into the tools that
already deal with the complexities of `-disable-free` and signal
handling.
Added:
Modified:
clang/include/clang/Basic/Diagnostic.h
clang/include/clang/DependencyScanning/DependencyScannerImpl.h
clang/include/clang/Frontend/ChainedDiagnosticConsumer.h
clang/lib/DependencyScanning/DependencyScannerImpl.cpp
clang/lib/DependencyScanning/DependencyScanningWorker.cpp
clang/lib/Frontend/CompilerInstance.cpp
clang/lib/Frontend/SerializedDiagnosticPrinter.cpp
clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
clang/lib/Tooling/Tooling.cpp
clang/tools/driver/cc1_main.cpp
clang/tools/driver/driver.cpp
clang/unittests/DependencyScanning/CMakeLists.txt
Removed:
clang/unittests/DependencyScanning/DependencyScanningWorkerTest.cpp
################################################################################
diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h
index 674c4d03c8b1b..d54ddbbeff472 100644
--- a/clang/include/clang/Basic/Diagnostic.h
+++ b/clang/include/clang/Basic/Diagnostic.h
@@ -1743,7 +1743,8 @@ class StoredDiagnostic {
llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const StoredDiagnostic &);
/// Abstract interface, implemented by clients of the front-end, which
-/// formats and prints fully processed diagnostics.
+/// formats and prints fully processed diagnostics. The destructor must be
+/// called even with -disable-free.
class DiagnosticConsumer {
protected:
unsigned NumWarnings = 0; ///< Number of warnings reported
@@ -1778,10 +1779,6 @@ class DiagnosticConsumer {
/// BeginSourceFile() are inaccessible.
virtual void EndSourceFile() {}
- /// Callback to inform the diagnostic client that processing of all
- /// source files has ended.
- virtual void finish() {}
-
/// Indicates whether the diagnostics handled by this
/// DiagnosticConsumer should be included in the number of diagnostics
/// reported by DiagnosticsEngine.
diff --git a/clang/include/clang/DependencyScanning/DependencyScannerImpl.h b/clang/include/clang/DependencyScanning/DependencyScannerImpl.h
index d50371512667d..2f91c7d9eba0a 100644
--- a/clang/include/clang/DependencyScanning/DependencyScannerImpl.h
+++ b/clang/include/clang/DependencyScanning/DependencyScannerImpl.h
@@ -46,7 +46,6 @@ class DependencyScanningAction {
DiagnosticConsumer *DiagConsumer);
bool hasScanned() const { return Scanned; }
- bool hasDiagConsumerFinished() const { return DiagConsumerFinished; }
private:
DependencyScanningService &Service;
@@ -57,7 +56,6 @@ class DependencyScanningAction {
std::optional<CompilerInstance> ScanInstanceStorage;
std::shared_ptr<ModuleDepCollector> MDC;
bool Scanned = false;
- bool DiagConsumerFinished = false;
};
// Helper functions and data types.
diff --git a/clang/include/clang/Frontend/ChainedDiagnosticConsumer.h b/clang/include/clang/Frontend/ChainedDiagnosticConsumer.h
index 839b5e7fa0a3e..f840ca85bd48d 100644
--- a/clang/include/clang/Frontend/ChainedDiagnosticConsumer.h
+++ b/clang/include/clang/Frontend/ChainedDiagnosticConsumer.h
@@ -47,11 +47,6 @@ class ChainedDiagnosticConsumer : public DiagnosticConsumer {
Primary->EndSourceFile();
}
- void finish() override {
- Secondary->finish();
- Primary->finish();
- }
-
bool IncludeInDiagnosticCounts() const override {
return Primary->IncludeInDiagnosticCounts();
}
diff --git a/clang/lib/DependencyScanning/DependencyScannerImpl.cpp b/clang/lib/DependencyScanning/DependencyScannerImpl.cpp
index 8af349319d333..fba7fdd067121 100644
--- a/clang/lib/DependencyScanning/DependencyScannerImpl.cpp
+++ b/clang/lib/DependencyScanning/DependencyScannerImpl.cpp
@@ -774,7 +774,6 @@ bool DependencyScanningAction::runInvocation(
std::move(PCHContainerOps), std::move(ModCache));
CompilerInstance &ScanInstance = *ScanInstanceStorage;
- assert(!DiagConsumerFinished && "attempt to reuse finished consumer");
initializeScanCompilerInstance(ScanInstance, FS, DiagConsumer, Service,
DepFS);
@@ -797,9 +796,6 @@ bool DependencyScanningAction::runInvocation(
ReadPCHAndPreprocessAction Action;
const bool Result = ScanInstance.ExecuteAction(Action);
- // ExecuteAction is responsible for calling finish.
- DiagConsumerFinished = true;
-
if (Result) {
if (MDC)
MDC->applyDiscoveredDependencies(*OriginalInvocation);
@@ -963,7 +959,4 @@ bool CompilerInstanceWithContext::computeDependencies(
return true;
}
-bool CompilerInstanceWithContext::finalize() {
- DiagConsumer->finish();
- return true;
-}
+bool CompilerInstanceWithContext::finalize() { return true; }
diff --git a/clang/lib/DependencyScanning/DependencyScanningWorker.cpp b/clang/lib/DependencyScanning/DependencyScanningWorker.cpp
index 60e5103fde6ef..d86026e147dc3 100644
--- a/clang/lib/DependencyScanning/DependencyScanningWorker.cpp
+++ b/clang/lib/DependencyScanning/DependencyScanningWorker.cpp
@@ -106,10 +106,6 @@ bool DependencyScanningWorker::computeDependencies(
return createAndRunToolInvocation(Cmd, Action, FS, PCHContainerOps, Diags);
});
- // Ensure finish() is called even if we never reached ExecuteAction().
- if (!Action.hasDiagConsumerFinished())
- DiagConsumer.finish();
-
return Success && Action.hasScanned();
}
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index 0a2dd8827a255..135923058bc55 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -966,11 +966,6 @@ bool CompilerInstance::ExecuteAction(FrontendAction &Act) {
// DesiredStackSpace available.
noteBottomOfStack();
- llvm::scope_exit FinishDiagnosticClient([&]() {
- // Notify the diagnostic client that all files were processed.
- getDiagnosticClient().finish();
- });
-
raw_ostream &OS = getVerboseOutputStream();
if (!Act.PrepareToExecute(*this))
diff --git a/clang/lib/Frontend/SerializedDiagnosticPrinter.cpp b/clang/lib/Frontend/SerializedDiagnosticPrinter.cpp
index 1dffe7cb2b9f0..58d080f5504c2 100644
--- a/clang/lib/Frontend/SerializedDiagnosticPrinter.cpp
+++ b/clang/lib/Frontend/SerializedDiagnosticPrinter.cpp
@@ -146,7 +146,7 @@ class SDiagsWriter : public DiagnosticConsumer {
EmitPreamble();
}
- ~SDiagsWriter() override {}
+ ~SDiagsWriter() override;
void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
const Diagnostic &Info) override;
@@ -155,8 +155,6 @@ class SDiagsWriter : public DiagnosticConsumer {
LangOpts = &LO;
}
- void finish() override;
-
private:
/// Build a DiagnosticsEngine to emit diagnostics about the diagnostics
DiagnosticsEngine *getMetaDiags();
@@ -770,7 +768,7 @@ void SDiagsWriter::RemoveOldDiagnostics() {
MergeChildRecords = false;
}
-void SDiagsWriter::finish() {
+SDiagsWriter::~SDiagsWriter() {
assert(!IsFinishing);
IsFinishing = true;
diff --git a/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp b/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
index e263e10bb065a..c8895da5a0f11 100644
--- a/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
+++ b/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
@@ -688,8 +688,6 @@ VerifyDiagnosticConsumer::~VerifyDiagnosticConsumer() {
assert(!CurrentPreprocessor && "CurrentPreprocessor should be invalid!");
SrcManager = nullptr;
CheckDiagnostics();
- assert(!Diags.ownsClient() &&
- "The VerifyDiagnosticConsumer takes over ownership of the client!");
}
// DiagnosticConsumer interface.
diff --git a/clang/lib/Tooling/Tooling.cpp b/clang/lib/Tooling/Tooling.cpp
index 1e9c21bef3d34..9830096c2d0b9 100644
--- a/clang/lib/Tooling/Tooling.cpp
+++ b/clang/lib/Tooling/Tooling.cpp
@@ -397,14 +397,10 @@ bool ToolInvocation::run() {
ArrayRef<const char *> CC1Args = ArrayRef(Argv).drop_front();
std::unique_ptr<CompilerInvocation> Invocation(
newInvocation(&*Diagnostics, CC1Args, BinaryName));
- if (Diagnostics->hasErrorOccurred()) {
- Diagnostics->getClient()->finish();
+ if (Diagnostics->hasErrorOccurred())
return false;
- }
- const bool Success = Action->runInvocation(
- std::move(Invocation), Files, std::move(PCHContainerOps), DiagConsumer);
- Diagnostics->getClient()->finish();
- return Success;
+ return Action->runInvocation(std::move(Invocation), Files,
+ std::move(PCHContainerOps), DiagConsumer);
}
const std::unique_ptr<driver::Driver> Driver(
@@ -417,23 +413,16 @@ bool ToolInvocation::run() {
Driver->setCheckInputsExist(false);
const std::unique_ptr<driver::Compilation> Compilation(
Driver->BuildCompilation(llvm::ArrayRef(Argv)));
- if (!Compilation) {
- Diagnostics->getClient()->finish();
+ if (!Compilation)
return false;
- }
const llvm::opt::ArgStringList *const CC1Args =
getCC1Arguments(&*Diagnostics, Compilation.get());
- if (!CC1Args) {
- Diagnostics->getClient()->finish();
+ if (!CC1Args)
return false;
- }
std::unique_ptr<CompilerInvocation> Invocation(
newInvocation(&*Diagnostics, *CC1Args, BinaryName));
- const bool Success =
- runInvocation(BinaryName, Compilation.get(), std::move(Invocation),
- std::move(PCHContainerOps));
- Diagnostics->getClient()->finish();
- return Success;
+ return runInvocation(BinaryName, Compilation.get(), std::move(Invocation),
+ std::move(PCHContainerOps));
}
bool ToolInvocation::runInvocation(
diff --git a/clang/tools/driver/cc1_main.cpp b/clang/tools/driver/cc1_main.cpp
index 1adb217014973..0a9ded1cf213a 100644
--- a/clang/tools/driver/cc1_main.cpp
+++ b/clang/tools/driver/cc1_main.cpp
@@ -289,10 +289,8 @@ int cc1_main(ArrayRef<const char *> Argv, const char *Argv0, void *MainAddr) {
static_cast<void*>(&Clang->getDiagnostics()));
DiagsBuffer->FlushDiagnostics(Clang->getDiagnostics());
- if (!Success) {
- Clang->getDiagnosticClient().finish();
+ if (!Success)
return 1;
- }
// Execute the frontend actions.
{
@@ -339,6 +337,8 @@ int cc1_main(ArrayRef<const char *> Argv, const char *Argv0, void *MainAddr) {
// When running with -disable-free, don't do any destruction or shutdown.
if (Clang->getFrontendOpts().DisableFree) {
+ // DiagnosticConsumer must be always destroyed.
+ Clang->getDiagnosticClient().~DiagnosticConsumer();
llvm::BuryPointer(std::move(Clang));
return !Success;
}
diff --git a/clang/tools/driver/driver.cpp b/clang/tools/driver/driver.cpp
index 1fffa579a9c8c..f22c4b7050b8e 100644
--- a/clang/tools/driver/driver.cpp
+++ b/clang/tools/driver/driver.cpp
@@ -455,8 +455,6 @@ int clang_main(int Argc, char **Argv, const llvm::ToolContext &ToolContext) {
*C, *FailingCommand))
Res = 1;
- Diags.getClient()->finish();
-
if (!UseNewCC1Process && IsCrash) {
// When crashing in -fintegrated-cc1 mode, bury the timer pointers, because
// the internal linked list might point to already released stack frames.
@@ -484,6 +482,8 @@ int clang_main(int Argc, char **Argv, const llvm::ToolContext &ToolContext) {
// llvm-ifs, exit with code 255 (-1) on failure.
if (CommandRes > 128 && CommandRes != 255) {
llvm::sys::unregisterHandlers();
+ // DiagnosticConsumer must be always destroyed.
+ Diags.getClient()->~DiagnosticConsumer();
raise(CommandRes - 128);
}
// When cc1 runs out-of-process (CLANG_SPAWN_CC1), ExecuteAndWait returns -2
@@ -491,6 +491,8 @@ int clang_main(int Argc, char **Argv, const llvm::ToolContext &ToolContext) {
// so resignal with SIGABRT to ensure the driver exits via signal.
if (CommandRes == -2) {
llvm::sys::unregisterHandlers();
+ // DiagnosticConsumer must be always destroyed.
+ Diags.getClient()->~DiagnosticConsumer();
raise(SIGABRT);
}
#endif
diff --git a/clang/unittests/DependencyScanning/CMakeLists.txt b/clang/unittests/DependencyScanning/CMakeLists.txt
index 40425820d4d08..3e7f902483968 100644
--- a/clang/unittests/DependencyScanning/CMakeLists.txt
+++ b/clang/unittests/DependencyScanning/CMakeLists.txt
@@ -1,6 +1,5 @@
add_clang_unittest(ClangDependencyScanningTests
DependencyScanningFilesystemTest.cpp
- DependencyScanningWorkerTest.cpp
CLANG_LIBS
clangDependencyScanning
clangFrontend # For TextDiagnosticPrinter.
diff --git a/clang/unittests/DependencyScanning/DependencyScanningWorkerTest.cpp b/clang/unittests/DependencyScanning/DependencyScanningWorkerTest.cpp
deleted file mode 100644
index 9fbebcbc4e1ca..0000000000000
--- a/clang/unittests/DependencyScanning/DependencyScanningWorkerTest.cpp
+++ /dev/null
@@ -1,101 +0,0 @@
-//===- DependencyScanningWorkerTest.cpp -----------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#include "clang/DependencyScanning/DependencyScanningWorker.h"
-#include "clang/DependencyScanning/DependencyScanningUtils.h"
-#include "llvm/Support/FormatVariadic.h"
-#include "gtest/gtest.h"
-#include <string>
-
-using namespace clang;
-using namespace dependencies;
-
-TEST(DependencyScanner, ScanDepsWithDiagConsumer) {
- StringRef CWD = "/root";
-
- auto VFS = llvm::makeIntrusiveRefCnt<llvm::vfs::InMemoryFileSystem>();
- VFS->setCurrentWorkingDirectory(CWD);
- auto Sept = llvm::sys::path::get_separator();
- std::string HeaderPath =
- std::string(llvm::formatv("{0}root{0}header.h", Sept));
- std::string TestPath = std::string(llvm::formatv("{0}root{0}test.cpp", Sept));
- std::string AsmPath = std::string(llvm::formatv("{0}root{0}test.s", Sept));
-
- VFS->addFile(HeaderPath, 0, llvm::MemoryBuffer::getMemBuffer("\n"));
- VFS->addFile(TestPath, 0,
- llvm::MemoryBuffer::getMemBuffer("#include \"header.h\"\n"));
- VFS->addFile(AsmPath, 0, llvm::MemoryBuffer::getMemBuffer(""));
-
- DependencyScanningServiceOptions Opts;
- Opts.MakeVFS = [&] { return VFS; };
- Opts.Format = ScanningOutputFormat::Make;
- DependencyScanningService Service(std::move(Opts));
- DependencyScanningWorker Worker(Service);
-
- llvm::DenseSet<ModuleID> AlreadySeen;
- FullDependencyConsumer DC(AlreadySeen);
- CallbackActionController AC(nullptr);
-
- struct EnsureFinishedConsumer : public DiagnosticConsumer {
- bool Finished = false;
- void finish() override { Finished = true; }
- };
-
- {
- // Check that a successful scan calls DiagConsumer.finish().
- std::vector<std::string> Args = {"clang",
- "-cc1",
- "-triple",
- "x86_64-apple-macosx10.7",
- "-emit-obj",
- "test.cpp",
- "-o"
- "test.cpp.o"};
-
- EnsureFinishedConsumer DiagConsumer;
- bool Success = Worker.computeDependencies(CWD, Args, DC, AC, DiagConsumer);
-
- EXPECT_TRUE(Success);
- EXPECT_EQ(DiagConsumer.getNumErrors(), 0u);
- EXPECT_TRUE(DiagConsumer.Finished);
- }
-
- {
- // Check that an invalid command-line, which never enters the scanning
- // action calls DiagConsumer.finish().
- std::vector<std::string> Args = {"clang", "-cc1", "-invalid-arg"};
- EnsureFinishedConsumer DiagConsumer;
- bool Success = Worker.computeDependencies(CWD, Args, DC, AC, DiagConsumer);
-
- EXPECT_FALSE(Success);
- EXPECT_GE(DiagConsumer.getNumErrors(), 1u);
- EXPECT_TRUE(DiagConsumer.Finished);
- }
-
- {
- // Check that a valid command line that produces no scanning jobs calls
- // DiagConsumer.finish().
- std::vector<std::string> Args = {"clang",
- "-cc1",
- "-triple",
- "x86_64-apple-macosx10.7",
- "-emit-obj",
- "-x",
- "assembler",
- "test.s",
- "-o"
- "test.cpp.o"};
-
- EnsureFinishedConsumer DiagConsumer;
- bool Success = Worker.computeDependencies(CWD, Args, DC, AC, DiagConsumer);
-
- EXPECT_FALSE(Success);
- EXPECT_EQ(DiagConsumer.getNumErrors(), 1u);
- EXPECT_TRUE(DiagConsumer.Finished);
- }
-}
More information about the cfe-commits
mailing list