From d5ec82e7acd11c0031c21a0b0726fda898b22dab Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Thu, 26 Feb 2026 18:44:08 -0800 Subject: [PATCH] Don't crash if clang setup fails. (#6804) Defer creating the CppContext until we have all of its components, so that we know they're not null. Don't track the action on the context, since it's not a reliable way of getting back to the compiler invocation on failure. Don't flush the diagnostics emitter from the emitter destructor since the derived class emitter will already have been destroyed at that point. Distinguish between clang setup failing and clang merely producing errors, and don't connect the check context to clang if clang setup failed. --------- Co-authored-by: David Blaikie --- toolchain/check/check_unit.cpp | 2 ++ toolchain/check/cpp/context.cpp | 7 ++-- toolchain/check/cpp/context.h | 32 +++++++------------ toolchain/check/cpp/generate_ast.cpp | 22 ++++++------- toolchain/check/cpp/import.cpp | 16 ++++------ .../testdata/interop/cpp/bad_flags.carbon | 22 +++++++++++++ .../interop/cpp/cpp_diagnostics.carbon | 4 +++ .../function/arithmetic_types_bridged.carbon | 1 - toolchain/diagnostics/emitter.h | 2 +- toolchain/diagnostics/emitter_test.cpp | 6 ++-- 10 files changed, 66 insertions(+), 48 deletions(-) create mode 100644 toolchain/check/testdata/interop/cpp/bad_flags.carbon diff --git a/toolchain/check/check_unit.cpp b/toolchain/check/check_unit.cpp index f517f8ff7303..4087d2b13276 100644 --- a/toolchain/check/check_unit.cpp +++ b/toolchain/check/check_unit.cpp @@ -619,6 +619,8 @@ auto CheckUnit::FinishRun() -> void { context_.inst_blocks().ReplacePlaceholder(reserved_id, block); } + emitter_.Flush(); + context_.sem_ir().set_has_errors(unit_and_imports_->err_tracker.seen_error()); // Verify that Context cleanly finished. diff --git a/toolchain/check/cpp/context.cpp b/toolchain/check/cpp/context.cpp index 49bffcb5c7b5..aba3f6d51105 100644 --- a/toolchain/check/cpp/context.cpp +++ b/toolchain/check/cpp/context.cpp @@ -8,8 +8,11 @@ namespace Carbon::Check { -CppContext::CppContext(std::unique_ptr action) - : action_(std::move(action)) {} +CppContext::CppContext(clang::CompilerInstance& instance, + std::unique_ptr parser) + : ast_context_(&instance.getASTContext()), + sema_(&instance.getSema()), + parser_(std::move(parser)) {} CppContext::~CppContext() = default; diff --git a/toolchain/check/cpp/context.h b/toolchain/check/cpp/context.h index 7cdaaab1167f..92234f3bde84 100644 --- a/toolchain/check/cpp/context.h +++ b/toolchain/check/cpp/context.h @@ -23,24 +23,13 @@ namespace Carbon::Check { // declarations, and similar values. class CppContext { public: - explicit CppContext(std::unique_ptr action); + explicit CppContext(clang::CompilerInstance& instance, + std::unique_ptr parser); ~CppContext(); - auto action() -> clang::FrontendAction& { return *action_; } - - auto ast_context() -> clang::ASTContext& { - return action_->getCompilerInstance().getASTContext(); - } - - auto sema() -> clang::Sema& { - return action_->getCompilerInstance().getSema(); - } - + auto ast_context() -> clang::ASTContext& { return *ast_context_; } + auto sema() -> clang::Sema& { return *sema_; } auto parser() -> clang::Parser& { return *parser_; } - auto set_parser(std::unique_ptr parser) { - CARBON_CHECK(!parser_); - parser_ = std::move(parser); - } auto clang_mangle_context() -> clang::MangleContext&; @@ -49,8 +38,14 @@ class CppContext { } private: - // The clang action that is generating the C++ AST. - std::unique_ptr action_; + // The Clang AST context. + clang::ASTContext* ast_context_; + + // The Clang semantic analysis engine. + clang::Sema* sema_; + + // The Clang parser. + std::unique_ptr parser_; // Per-Carbon-file start locations for corresponding Clang source buffers. // Owned and managed by code in location.cpp. @@ -58,9 +53,6 @@ class CppContext { // The Clang mangle context for the target in the ASTContext. std::unique_ptr clang_mangle_context_; - - // The Clang parser. - std::unique_ptr parser_; }; } // namespace Carbon::Check diff --git a/toolchain/check/cpp/generate_ast.cpp b/toolchain/check/cpp/generate_ast.cpp index 00c03b62d22e..2e8acefe6cb9 100644 --- a/toolchain/check/cpp/generate_ast.cpp +++ b/toolchain/check/cpp/generate_ast.cpp @@ -374,10 +374,10 @@ class GenerateASTAction : public clang::ASTFrontendAction { clang_instance.createSema(getTranslationUnitKind(), /*CompletionConsumer=*/nullptr); - context_->cpp_context()->set_parser(std::make_unique( + auto parser_ptr = std::make_unique( clang_instance.getPreprocessor(), clang_instance.getSema(), - /*SkipFunctionBodies=*/false)); - auto& parser = context_->cpp_context()->parser(); + /*SkipFunctionBodies=*/false); + auto& parser = *parser_ptr; clang_instance.getPreprocessor().EnterMainSourceFile(); if (auto* source = clang_instance.getASTContext().getExternalSource()) { @@ -387,6 +387,9 @@ class GenerateASTAction : public clang::ASTFrontendAction { parser.Initialize(); clang_instance.getSema().ActOnStartOfTranslationUnit(); + context_->set_cpp_context( + std::make_unique(clang_instance, std::move(parser_ptr))); + // Don't allow C++20 module declarations in inline Cpp code fragments. auto module_import_state = clang::Sema::ModuleImportState::NotACXX20Module; @@ -448,8 +451,6 @@ auto GenerateAst(Context& context, invocation->getPreprocessorOpts().addRemappedFile(file_name, includes_buffer.release()); - clang::DiagnosticErrorTrap trap(*diags); - auto clang_instance_ptr = std::make_unique(invocation); auto& clang_instance = *clang_instance_ptr; @@ -464,15 +465,12 @@ auto GenerateAst(Context& context, return false; } - context.set_cpp_context(std::make_unique( - std::make_unique(context))); - - if (!context.cpp_context()->action().BeginSourceFile(clang_instance, - inputs[0])) { + GenerateASTAction action(context); + if (!action.BeginSourceFile(clang_instance, inputs[0])) { return false; } - if (llvm::Error error = context.cpp_context()->action().Execute()) { + if (llvm::Error error = action.Execute()) { // `Execute` currently never fails, but its contract allows it to. context.TODO(SemIR::LocId::None, "failed to execute clang action: " + llvm::toString(std::move(error))); @@ -483,7 +481,7 @@ auto GenerateAst(Context& context, // diagnostic now. context.emitter().Flush(); - return !trap.hasErrorOccurred(); + return true; } auto FinishAst(Context& context) -> void { diff --git a/toolchain/check/cpp/import.cpp b/toolchain/check/cpp/import.cpp index 62f2428a9708..61512e80e15f 100644 --- a/toolchain/check/cpp/import.cpp +++ b/toolchain/check/cpp/import.cpp @@ -130,19 +130,17 @@ auto ImportCpp(Context& context, llvm::all_of(imports, [&](const Parse::Tree::PackagingNames& import) { return import.package_id == package_id; })); + auto name_scope_id = AddNamespace(context, package_id, imports); - - bool ast_has_error = - !GenerateAst(context, imports, fs, llvm_context, std::move(invocation)); - SemIR::NameScope& name_scope = context.name_scopes().Get(name_scope_id); name_scope.set_is_closed_import(true); - name_scope.set_clang_decl_context_id(context.clang_decls().Add( - {.key = - SemIR::ClangDeclKey(context.ast_context().getTranslationUnitDecl()), - .inst_id = name_scope.inst_id()})); - if (ast_has_error) { + if (GenerateAst(context, imports, fs, llvm_context, std::move(invocation))) { + name_scope.set_clang_decl_context_id(context.clang_decls().Add( + {.key = SemIR::ClangDeclKey( + context.ast_context().getTranslationUnitDecl()), + .inst_id = name_scope.inst_id()})); + } else { name_scope.set_has_error(); } } diff --git a/toolchain/check/testdata/interop/cpp/bad_flags.carbon b/toolchain/check/testdata/interop/cpp/bad_flags.carbon new file mode 100644 index 000000000000..096b3f628a6a --- /dev/null +++ b/toolchain/check/testdata/interop/cpp/bad_flags.carbon @@ -0,0 +1,22 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +// INCLUDE-FILE: toolchain/testing/testdata/min_prelude/none.carbon +// EXTRA-ARGS: --clang-arg=-include-pch --clang-arg=does_not_exist.pch +// +// AUTOUPDATE +// TIP: To test this file alone, run: +// TIP: bazel test //toolchain/testing:file_test --test_arg=--file_tests=toolchain/check/testdata/interop/cpp/bad_flags.carbon +// TIP: To dump output, run: +// TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/check/testdata/interop/cpp/bad_flags.carbon +// CHECK:STDERR: error: unable to read PCH file does_not_exist.pch: 'No such file or directory' [CppInteropParseError] +// CHECK:STDERR: +// CHECK:STDERR: error: PCH file 'does_not_exist.pch' not found: module file not found [CppInteropParseError] +// CHECK:STDERR: + +// --- fail_bad_args.carbon + +import Cpp inline ""; + +// Nothing to see here. We're just checking that this does not crash. diff --git a/toolchain/check/testdata/interop/cpp/cpp_diagnostics.carbon b/toolchain/check/testdata/interop/cpp/cpp_diagnostics.carbon index 13cdb1c8c38b..ba2d8580ba08 100644 --- a/toolchain/check/testdata/interop/cpp/cpp_diagnostics.carbon +++ b/toolchain/check/testdata/interop/cpp/cpp_diagnostics.carbon @@ -338,6 +338,10 @@ library "[[@TEST_NAME]]"; import Cpp library "fix_it_hints.h"; fn F() { + // CHECK:STDERR: fail_import_fix_it_hints.carbon:[[@LINE+4]]:3: error: member name `foo` not found in `Cpp` [MemberNameNotFoundInInstScope] + // CHECK:STDERR: Cpp.foo(); + // CHECK:STDERR: ^~~~~~~ + // CHECK:STDERR: Cpp.foo(); } diff --git a/toolchain/check/testdata/interop/cpp/function/arithmetic_types_bridged.carbon b/toolchain/check/testdata/interop/cpp/function/arithmetic_types_bridged.carbon index 70dba8c2cecd..ebdecd792ea7 100644 --- a/toolchain/check/testdata/interop/cpp/function/arithmetic_types_bridged.carbon +++ b/toolchain/check/testdata/interop/cpp/function/arithmetic_types_bridged.carbon @@ -988,7 +988,6 @@ fn F() { // CHECK:STDOUT: %Cpp: = namespace file.%Cpp.import_cpp, [concrete] { // CHECK:STDOUT: .foo = %foo.cpp_overload_set.value // CHECK:STDOUT: import Cpp//... -// CHECK:STDOUT: has_error // CHECK:STDOUT: } // CHECK:STDOUT: %foo.cpp_overload_set.value: %foo.cpp_overload_set.type = cpp_overload_set_value @foo.cpp_overload_set [concrete = constants.%foo.cpp_overload_set.value] // CHECK:STDOUT: %foo.decl: %foo.type = fn_decl @foo [concrete = constants.%foo] { diff --git a/toolchain/diagnostics/emitter.h b/toolchain/diagnostics/emitter.h index e495f84dca46..a21da0507438 100644 --- a/toolchain/diagnostics/emitter.h +++ b/toolchain/diagnostics/emitter.h @@ -164,7 +164,7 @@ class Emitter { // `consumer` is required to outlive the diagnostic emitter. explicit Emitter(Consumer* consumer) : consumer_(consumer) {} - virtual ~Emitter() { Flush(); } + virtual ~Emitter() = default; // Emits an error. // diff --git a/toolchain/diagnostics/emitter_test.cpp b/toolchain/diagnostics/emitter_test.cpp index 4a83d0350e20..40999754b7b0 100644 --- a/toolchain/diagnostics/emitter_test.cpp +++ b/toolchain/diagnostics/emitter_test.cpp @@ -251,9 +251,9 @@ TEST_F(EmitterTest, Flush) { flushed = false; } - // Destroying the emitter should flush. - EXPECT_TRUE(flushed); - flushed = false; + // Destroying the emitter should not flush, as that could call back into the + // base class emitter after the derived-class emitter has been destroyed. + EXPECT_FALSE(flushed); } } // namespace