shylie/llvm-project
1
0
Fork 0
Code Issues Pull Requests Actions 6 Packages Projects Releases Wiki Activity

[clang] Replace finish() with destructors for DiagnosticConsumer (#183831)

Browse Source 
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.
This commit is contained in:
Jan Svoboda 2026-03-02 12:51:53 -08:00 committed by GitHub
parent a4d786630c
commit 8a9049198d
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
13 changed files with 19 additions and 160 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
clang/include/clang/Basic/Diagnostic.h
View File

@ -1743,7 +1743,8 @@ public:
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 @@ public:
/// 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.

2
clang/include/clang/DependencyScanning/DependencyScannerImpl.h
View File

@ -46,7 +46,6 @@ public:
DiagnosticConsumer *DiagConsumer);
bool hasScanned() const { return Scanned; }
bool hasDiagConsumerFinished() const { return DiagConsumerFinished; }
private:
DependencyScanningService &Service;
@ -57,7 +56,6 @@ private:
std::optional<CompilerInstance> ScanInstanceStorage;
std::shared_ptr<ModuleDepCollector> MDC;
bool Scanned = false;
bool DiagConsumerFinished = false;
};
// Helper functions and data types.

5
clang/include/clang/Frontend/ChainedDiagnosticConsumer.h
View File

@ -47,11 +47,6 @@ public:
Primary->EndSourceFile();
}
void finish() override {
Secondary->finish();
Primary->finish();
}
bool IncludeInDiagnosticCounts() const override {
return Primary->IncludeInDiagnosticCounts();
}

9
clang/lib/DependencyScanning/DependencyScannerImpl.cpp
View File

@ -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; }

4
clang/lib/DependencyScanning/DependencyScanningWorker.cpp
View File

@ -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();
}

5
clang/lib/Frontend/CompilerInstance.cpp
View File

@ -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))

6
clang/lib/Frontend/SerializedDiagnosticPrinter.cpp
View File

@ -146,7 +146,7 @@ public:
EmitPreamble();
}
~SDiagsWriter() override {}
~SDiagsWriter() override;
void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
const Diagnostic &Info) override;
@ -155,8 +155,6 @@ public:
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;

2
clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
View File

@ -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.

25
clang/lib/Tooling/Tooling.cpp
View File

@ -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(

6
clang/tools/driver/cc1_main.cpp
View File

@ -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;
}

6
clang/tools/driver/driver.cpp
View File

@ -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

1
clang/unittests/DependencyScanning/CMakeLists.txt
View File

@ -1,6 +1,5 @@
add_clang_unittest(ClangDependencyScanningTests
DependencyScanningFilesystemTest.cpp
DependencyScanningWorkerTest.cpp
CLANG_LIBS
clangDependencyScanning
clangFrontend # For TextDiagnosticPrinter.

101
clang/unittests/DependencyScanning/DependencyScanningWorkerTest.cpp
View File

@ -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);
}
}