From bd2483b553288f907a7b761fce9c093dcfaf4217 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Tue, 5 Aug 2025 14:21:30 -0700 Subject: [PATCH] Don't use `Check::Context` when emitting C++ diagnostics. (#5910) Diagnostics can get flushed after the context is destroyed. Should fix an issue found by msan. --- toolchain/check/import_cpp.cpp | 124 ++++++++++++++++++--------------- 1 file changed, 67 insertions(+), 57 deletions(-) diff --git a/toolchain/check/import_cpp.cpp b/toolchain/check/import_cpp.cpp index 8df9bbe022cb..9637b306dbe1 100644 --- a/toolchain/check/import_cpp.cpp +++ b/toolchain/check/import_cpp.cpp @@ -93,13 +93,12 @@ static auto AddIdentifierName(Context& context, llvm::StringRef name) // Adds the given source location and an `ImportIRInst` referring to it in // `ImportIRId::Cpp`. -static auto AddImportIRInst(Context& context, +static auto AddImportIRInst(SemIR::File& file, clang::SourceLocation clang_source_loc) -> SemIR::ImportIRInstId { SemIR::ClangSourceLocId clang_source_loc_id = - context.sem_ir().clang_source_locs().Add(clang_source_loc); - return context.import_ir_insts().Add( - SemIR::ImportIRInst(clang_source_loc_id)); + file.clang_source_locs().Add(clang_source_loc); + return file.import_ir_insts().Add(SemIR::ImportIRInst(clang_source_loc_id)); } namespace { @@ -116,15 +115,22 @@ namespace { // wrong C++ diagnostics, and at the end of the Carbon program. class CarbonClangDiagnosticConsumer : public clang::DiagnosticConsumer { public: - // Creates an instance with the location that triggers calling Clang. - // `context` must not be null. + // Creates an instance with the location that triggers calling Clang. The + // `context` is not stored here, and the diagnostics consumer is expected to + // outlive it. explicit CarbonClangDiagnosticConsumer( - Context* context, std::shared_ptr invocation) - : context_(context), invocation_(std::move(invocation)) { - context->emitter().AddFlushFn([this] { EmitDiagnostics(); }); + Context& context, std::shared_ptr invocation) + : sem_ir_(&context.sem_ir()), + emitter_(&context.emitter()), + invocation_(std::move(invocation)) { + emitter_->AddFlushFn([this] { EmitDiagnostics(); }); } ~CarbonClangDiagnosticConsumer() override { + // Do not inspect `emitter_` here; it's typically destroyed before the + // consumer is. + // TODO: If Clang produces diagnostics after check finishes, they'll get + // added to the list of pending diagnostics and never emitted. CARBON_CHECK(diagnostic_infos_.empty(), "Missing flush before destroying diagnostic consumer"); } @@ -136,7 +142,7 @@ class CarbonClangDiagnosticConsumer : public clang::DiagnosticConsumer { DiagnosticConsumer::HandleDiagnostic(diag_level, info); SemIR::ImportIRInstId clang_import_ir_inst_id = - AddImportIRInst(*context_, info.getLocation()); + AddImportIRInst(*sem_ir_, info.getLocation()); llvm::SmallString<256> message; info.FormatDiagnostic(message); @@ -163,57 +169,57 @@ class CarbonClangDiagnosticConsumer : public clang::DiagnosticConsumer { .snippet = snippet_stream.TakeStr()}); } + // Returns the diagnostic to use for a given Clang diagnostic level. + static auto GetDiagnostic(clang::DiagnosticsEngine::Level level) + -> const Diagnostics::DiagnosticBase& { + switch (level) { + case clang::DiagnosticsEngine::Ignored: { + CARBON_FATAL("Emitting an ignored diagnostic"); + break; + } + case clang::DiagnosticsEngine::Note: { + CARBON_DIAGNOSTIC(CppInteropParseNote, Note, "{0}", std::string); + return CppInteropParseNote; + } + case clang::DiagnosticsEngine::Remark: + case clang::DiagnosticsEngine::Warning: { + // TODO: Add a distinct Remark level to Carbon diagnostics, and stop + // mapping remarks to warnings. + CARBON_DIAGNOSTIC(CppInteropParseWarning, Warning, "{0}", std::string); + return CppInteropParseWarning; + } + case clang::DiagnosticsEngine::Error: + case clang::DiagnosticsEngine::Fatal: { + CARBON_DIAGNOSTIC(CppInteropParseError, Error, "{0}", std::string); + return CppInteropParseError; + } + } + } + // Outputs Carbon diagnostics based on the collected Clang diagnostics. Must // be called after the AST is set in the context. auto EmitDiagnostics() -> void { - CARBON_CHECK(context_->sem_ir().cpp_ast(), + CARBON_CHECK(sem_ir_->cpp_ast(), "Attempted to emit diagnostics before the AST Unit is loaded"); for (size_t i = 0; i != diagnostic_infos_.size(); ++i) { const ClangDiagnosticInfo& info = diagnostic_infos_[i]; - switch (info.level) { - case clang::DiagnosticsEngine::Ignored: - case clang::DiagnosticsEngine::Note: - case clang::DiagnosticsEngine::Remark: { - context_->TODO( - SemIR::LocId(info.import_ir_inst_id), - llvm::formatv( - "Unsupported: C++ diagnostic level for diagnostic\n{0}", - info.message)); - break; - } - case clang::DiagnosticsEngine::Warning: - case clang::DiagnosticsEngine::Error: - case clang::DiagnosticsEngine::Fatal: { - CARBON_DIAGNOSTIC(CppInteropParseWarning, Warning, "{0}", - std::string); - CARBON_DIAGNOSTIC(CppInteropParseError, Error, "{0}", std::string); - auto builder = context_->emitter().Build( - SemIR::LocId(info.import_ir_inst_id), - info.level == clang::DiagnosticsEngine::Warning - ? CppInteropParseWarning - : CppInteropParseError, - info.message); - builder.OverrideSnippet(info.snippet); - for (; - i + 1 < diagnostic_infos_.size() && - diagnostic_infos_[i + 1].level == clang::DiagnosticsEngine::Note; - ++i) { - const ClangDiagnosticInfo& note_info = diagnostic_infos_[i + 1]; - CARBON_DIAGNOSTIC(CppInteropParseNote, Note, "{0}", std::string); - builder - .Note(SemIR::LocId(note_info.import_ir_inst_id), - CppInteropParseNote, note_info.message) - .OverrideSnippet(note_info.snippet); - } - // TODO: This will apply all current Carbon annotation functions. We - // should instead track how Clang's context notes and Carbon's - // annotation functions are interleaved, and interleave the notes in - // the same order. - builder.Emit(); - break; - } + auto builder = emitter_->Build(SemIR::LocId(info.import_ir_inst_id), + GetDiagnostic(info.level), info.message); + builder.OverrideSnippet(info.snippet); + for (; i + 1 < diagnostic_infos_.size() && + diagnostic_infos_[i + 1].level == clang::DiagnosticsEngine::Note; + ++i) { + const ClangDiagnosticInfo& note_info = diagnostic_infos_[i + 1]; + builder + .Note(SemIR::LocId(note_info.import_ir_inst_id), + GetDiagnostic(note_info.level), note_info.message) + .OverrideSnippet(note_info.snippet); } + // TODO: This will apply all current Carbon annotation functions. We + // should instead track how Clang's context notes and Carbon's annotation + // functions are interleaved, and interleave the notes in the same order. + builder.Emit(); } diagnostic_infos_.clear(); } @@ -267,8 +273,11 @@ class CarbonClangDiagnosticConsumer : public clang::DiagnosticConsumer { std::string snippet; }; - // The type-checking context in which we're running Clang. - Context* context_; + // The Carbon file that this C++ compilation is attached to. + SemIR::File* sem_ir_; + + // The diagnostic emitter that we're emitting diagnostics into. + DiagnosticEmitterBase* emitter_; // The compiler invocation that is producing the diagnostics. std::shared_ptr invocation_; @@ -316,7 +325,7 @@ static auto GenerateAst( llvm::IntrusiveRefCntPtr diags( clang::CompilerInstance::createDiagnostics( *fs, invocation->getDiagnosticOpts(), - new CarbonClangDiagnosticConsumer(&context, invocation), + new CarbonClangDiagnosticConsumer(context, invocation), /*ShouldOwnClient=*/true)); // Extract the input from the frontend invocation and make sure it makes @@ -587,7 +596,8 @@ static auto BuildClassDecl(Context& context, static auto ImportCXXRecordDecl(Context& context, clang::CXXRecordDecl* clang_decl) -> SemIR::InstId { - auto import_ir_inst_id = AddImportIRInst(context, clang_decl->getLocation()); + auto import_ir_inst_id = + AddImportIRInst(context.sem_ir(), clang_decl->getLocation()); auto [class_id, class_inst_id] = BuildClassDecl( context, import_ir_inst_id, GetParentNameScopeId(context, clang_decl),