llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Jan Svoboda (jansvoboda11) <details> <summary>Changes</summary> 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. --- Full diff: https://github.com/llvm/llvm-project/pull/183831.diff 13 Files Affected: - (modified) clang/include/clang/Basic/Diagnostic.h (+2-5) - (modified) clang/include/clang/DependencyScanning/DependencyScannerImpl.h (-2) - (modified) clang/include/clang/Frontend/ChainedDiagnosticConsumer.h (-5) - (modified) clang/lib/DependencyScanning/DependencyScannerImpl.cpp (-5) - (modified) clang/lib/DependencyScanning/DependencyScanningWorker.cpp (-4) - (modified) clang/lib/Frontend/CompilerInstance.cpp (-5) - (modified) clang/lib/Frontend/SerializedDiagnosticPrinter.cpp (+2-4) - (modified) clang/lib/Frontend/VerifyDiagnosticConsumer.cpp (-2) - (modified) clang/lib/Tooling/Tooling.cpp (+7-18) - (modified) clang/tools/driver/cc1_main.cpp (+3-3) - (modified) clang/tools/driver/driver.cpp (+4-2) - (modified) clang/unittests/DependencyScanning/CMakeLists.txt (-1) - (removed) clang/unittests/DependencyScanning/DependencyScanningWorkerTest.cpp (-101) ``````````diff 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 5e1d12e95c162..49427cc8365da 100644 --- a/clang/lib/DependencyScanning/DependencyScannerImpl.cpp +++ b/clang/lib/DependencyScanning/DependencyScannerImpl.cpp @@ -777,7 +777,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); @@ -800,9 +799,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); @@ -967,6 +963,5 @@ bool CompilerInstanceWithContext::computeDependencies( } bool CompilerInstanceWithContext::finalize() { - DiagConsumer->finish(); 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 1e256a9d5614c..b0e69dcceb251 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); - } -} `````````` </details> https://github.com/llvm/llvm-project/pull/183831 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
