From 063a201b6dd41b7e4e03540267e2bfbea242c5ad Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Tue, 22 Aug 2023 13:45:04 -0700 Subject: [PATCH] Remove automatic flushing in the SortingDiagnosticConsumer destructor. (#3134) We already needed to manually flush diagnostics along every path out of the driver, so switch to a CHECK-failure if we get this wrong rather than a potential use-after-lifetime bug. Switch the driver to flush explicitly rather than using a bunch of cleanup lambdas, now that we have checking that we get this right. This is a follow-up after #3126. --- .../diagnostics/sorting_diagnostic_consumer.h | 9 +++++- toolchain/driver/driver.cpp | 28 ++++++++----------- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/toolchain/diagnostics/sorting_diagnostic_consumer.h b/toolchain/diagnostics/sorting_diagnostic_consumer.h index d0a6d998c599..e36ac6e6267e 100644 --- a/toolchain/diagnostics/sorting_diagnostic_consumer.h +++ b/toolchain/diagnostics/sorting_diagnostic_consumer.h @@ -17,7 +17,14 @@ class SortingDiagnosticConsumer : public DiagnosticConsumer { explicit SortingDiagnosticConsumer(DiagnosticConsumer& next_consumer) : next_consumer_(&next_consumer) {} - ~SortingDiagnosticConsumer() override { Flush(); } + ~SortingDiagnosticConsumer() override { + // We choose not to automatically flush diagnostics here, because they are + // likely to refer to data that gets destroyed before the diagnostics + // consumer is destroyed, because the diagnostics consumer is typically + // created before the objects that diagnostics refer into are created. + CARBON_CHECK(diagnostics_.empty()) + << "Must flush diagnostics consumer before destroying it"; + } // Buffers the diagnostic. auto HandleDiagnostic(Diagnostic diagnostic) -> void override { diff --git a/toolchain/driver/driver.cpp b/toolchain/driver/driver.cpp index 24c50a62683b..a9f427eed0df 100644 --- a/toolchain/driver/driver.cpp +++ b/toolchain/driver/driver.cpp @@ -388,6 +388,9 @@ auto Driver::Compile(const CompileOptions& options) -> bool { StreamDiagnosticConsumer stream_consumer(error_stream_); DiagnosticConsumer* consumer = &stream_consumer; + + // Note, the diagnostics consumer must be flushed before each `return` in this + // function, as diagnostics can refer to state that lives on our stack. std::unique_ptr sorting_consumer; if (vlog_stream_ == nullptr && !options.stream_errors) { sorting_consumer = std::make_unique(*consumer); @@ -398,20 +401,16 @@ auto Driver::Compile(const CompileOptions& options) -> bool { << options.input_file_name << "' ***\n"; auto source = SourceBuffer::CreateFromFile(fs_, options.input_file_name); CARBON_VLOG() << "*** SourceBuffer::CreateFromFile done ***\n"; - // Require flushing the consumer before the source buffer is destroyed, - // because diagnostics may reference the buffer. - auto flush_for_source = llvm::make_scope_exit([&]() { consumer->Flush(); }); if (!source.ok()) { error_stream_ << "ERROR: Unable to open input source file: " << source.error(); + consumer->Flush(); return false; } CARBON_VLOG() << "*** file:\n```\n" << source->text() << "\n```\n"; CARBON_VLOG() << "*** TokenizedBuffer::Lex ***\n"; auto tokenized_source = TokenizedBuffer::Lex(*source, *consumer); - // Diagnostics may reference the tokenized buffer. - auto flush_for_tokens = llvm::make_scope_exit([&]() { consumer->Flush(); }); bool has_errors = tokenized_source.has_errors(); CARBON_VLOG() << "*** TokenizedBuffer::Lex done ***\n"; if (options.dump_tokens) { @@ -421,13 +420,12 @@ auto Driver::Compile(const CompileOptions& options) -> bool { } CARBON_VLOG() << "tokenized_buffer: " << tokenized_source; if (options.phase == Phase::Lex) { + consumer->Flush(); return !has_errors; } CARBON_VLOG() << "*** ParseTree::Parse ***\n"; auto parse_tree = ParseTree::Parse(tokenized_source, *consumer, vlog_stream_); - // Diagnostics may reference the parse tree. - auto flush_for_parse = llvm::make_scope_exit([&]() { consumer->Flush(); }); has_errors |= parse_tree.has_errors(); CARBON_VLOG() << "*** ParseTree::Parse done ***\n"; if (options.dump_parse_tree) { @@ -436,6 +434,7 @@ auto Driver::Compile(const CompileOptions& options) -> bool { } CARBON_VLOG() << "parse_tree: " << parse_tree; if (options.phase == Phase::Parse) { + consumer->Flush(); return !has_errors; } @@ -443,12 +442,15 @@ auto Driver::Compile(const CompileOptions& options) -> bool { CARBON_VLOG() << "*** SemanticsIR::MakeFromParseTree ***\n"; const SemanticsIR semantics_ir = SemanticsIR::MakeFromParseTree( builtin_ir, tokenized_source, parse_tree, *consumer, vlog_stream_); - // Diagnostics may reference the semantics IR. - auto flush_for_ir = llvm::make_scope_exit([&]() { consumer->Flush(); }); + + // We've finished all steps that can produce diagnostics. Emit the + // diagnostics now, so that the developer sees them sooner and doesn't need + // to wait for code generation. + consumer->Flush(); + has_errors |= semantics_ir.has_errors(); CARBON_VLOG() << "*** SemanticsIR::MakeFromParseTree done ***\n"; if (options.dump_raw_semantics_ir) { - consumer->Flush(); semantics_ir.Print(output_stream_, options.builtin_semantics_ir); if (options.dump_semantics_ir) { output_stream_ << "\n"; @@ -470,12 +472,6 @@ auto Driver::Compile(const CompileOptions& options) -> bool { return false; } - // Emit diagnostics now, so that the developer sees them sooner and doesn't - // need to wait for code generation. - // TODO: If we allow lowering to produce warnings, should we interleave them - // with diagnostics produced by earlier steps? - consumer->Flush(); - CARBON_VLOG() << "*** LowerToLLVM ***\n"; llvm::LLVMContext llvm_context; const std::unique_ptr module = LowerToLLVM(