From a4a229b6373da29ee0cd3f0fbff03051c51ac039 Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Wed, 12 Mar 2025 19:49:43 +0100 Subject: [PATCH] Initialize `cpp_mangle_context_` in `Mangler`'s constructor (#5095) This is a followup of [a comment](https://github.com/carbon-language/carbon-lang/pull/5062/files/89e56d51858bcc18d4242d4e5c9ee0e7496d887e#r1979993815) in #5062. Add a mutable AST pointer to `FileContext`. This is necessary since we use [Clang with lack of const correctness](https://github.com/llvm/llvm-project/pull/130096#issuecomment-2704413782). Alternatives in Clang: * Change `ASTUnit::getASTContext() const` to return a non-const `ASTContext`. [Tried and was rejected upstream due to weakening const correctness](https://github.com/llvm/llvm-project/pull/130096). * Change `createMangleContext()` to be `const`. Tried that and it seems like it relies heavily on non const API. * Change `MangleContext::mangleName()` to `const`. Tried that but there are several lazy initialization and id creations happening that modify the context. See details in https://github.com/llvm/llvm-project/pull/130613. Alternatives in Carbon: * Use `const_cast` on `ASTContext` when calling `createMangleContext()`. * Make `FileContext::sem_ir_` point to a mutable `SemIR::File`. * Change `File::cpp_ast()` to be const while keeping it return a mutable pointer. Part of #4666. --- toolchain/driver/compile_subcommand.cpp | 7 ++++--- toolchain/lower/file_context.cpp | 4 +++- toolchain/lower/file_context.h | 8 +++++++- toolchain/lower/lower.cpp | 4 ++-- toolchain/lower/lower.h | 2 +- toolchain/lower/mangler.cpp | 10 +++------- toolchain/lower/mangler.h | 7 ++++++- toolchain/sem_ir/file.h | 3 ++- 8 files changed, 28 insertions(+), 17 deletions(-) diff --git a/toolchain/driver/compile_subcommand.cpp b/toolchain/driver/compile_subcommand.cpp index 871214caf411..9421b6497df3 100644 --- a/toolchain/driver/compile_subcommand.cpp +++ b/toolchain/driver/compile_subcommand.cpp @@ -598,9 +598,10 @@ auto CompilationUnit::RunLower( // TODO: Consider disabling instruction naming by default if we're not // producing textual LLVM IR. SemIR::InstNamer inst_namer(&*sem_ir_); - module_ = Lower::LowerToLLVM( - *llvm_context_, tree_and_subtrees_getters_for_debug_info, - input_filename_, *sem_ir_, &inst_namer, vlog_stream_); + module_ = Lower::LowerToLLVM(*llvm_context_, + tree_and_subtrees_getters_for_debug_info, + input_filename_, *sem_ir_, sem_ir_->cpp_ast(), + &inst_namer, vlog_stream_); }); if (vlog_stream_) { CARBON_VLOG("*** llvm::Module ***\n"); diff --git a/toolchain/lower/file_context.cpp b/toolchain/lower/file_context.cpp index 228064701e88..433ad7de981b 100644 --- a/toolchain/lower/file_context.cpp +++ b/toolchain/lower/file_context.cpp @@ -31,7 +31,8 @@ FileContext::FileContext( std::optional> tree_and_subtrees_getters_for_debug_info, llvm::StringRef module_name, const SemIR::File& sem_ir, - const SemIR::InstNamer* inst_namer, llvm::raw_ostream* vlog_stream) + clang::ASTUnit* cpp_ast, const SemIR::InstNamer* inst_namer, + llvm::raw_ostream* vlog_stream) : llvm_context_(&llvm_context), llvm_module_(std::make_unique(module_name, llvm_context)), di_builder_(*llvm_module_), @@ -42,6 +43,7 @@ FileContext::FileContext( tree_and_subtrees_getters_for_debug_info_( tree_and_subtrees_getters_for_debug_info), sem_ir_(&sem_ir), + cpp_ast_(cpp_ast), inst_namer_(inst_namer), vlog_stream_(vlog_stream) { CARBON_CHECK(!sem_ir.has_errors(), diff --git a/toolchain/lower/file_context.h b/toolchain/lower/file_context.h index 54d2345366fe..c748fd4a9770 100644 --- a/toolchain/lower/file_context.h +++ b/toolchain/lower/file_context.h @@ -33,7 +33,8 @@ class FileContext { std::optional> tree_and_subtrees_getters_for_debug_info, llvm::StringRef module_name, const SemIR::File& sem_ir, - const SemIR::InstNamer* inst_namer, llvm::raw_ostream* vlog_stream); + clang::ASTUnit* cpp_ast, const SemIR::InstNamer* inst_namer, + llvm::raw_ostream* vlog_stream); // Lowers the SemIR::File to LLVM IR. Should only be called once, and handles // the main execution loop. @@ -91,6 +92,7 @@ class FileContext { auto llvm_context() -> llvm::LLVMContext& { return *llvm_context_; } auto llvm_module() -> llvm::Module& { return *llvm_module_; } auto sem_ir() -> const SemIR::File& { return *sem_ir_; } + auto cpp_ast() -> clang::ASTUnit* { return cpp_ast_; } auto inst_namer() -> const SemIR::InstNamer* { return inst_namer_; } auto global_variables() -> const Map& { return global_variables_; @@ -162,6 +164,10 @@ class FileContext { // The input SemIR. const SemIR::File* const sem_ir_; + // A mutable Clang AST is necessary for lowering since using the AST in lower + // modifies it. + clang::ASTUnit* cpp_ast_; + // The instruction namer, if given. const SemIR::InstNamer* const inst_namer_; diff --git a/toolchain/lower/lower.cpp b/toolchain/lower/lower.cpp index 8dec5936967d..5a5afff7fba8 100644 --- a/toolchain/lower/lower.cpp +++ b/toolchain/lower/lower.cpp @@ -12,11 +12,11 @@ auto LowerToLLVM(llvm::LLVMContext& llvm_context, std::optional> tree_and_subtrees_getters_for_debug_info, llvm::StringRef module_name, const SemIR::File& sem_ir, - const SemIR::InstNamer* inst_namer, + clang::ASTUnit* cpp_ast, const SemIR::InstNamer* inst_namer, llvm::raw_ostream* vlog_stream) -> std::unique_ptr { FileContext context(llvm_context, tree_and_subtrees_getters_for_debug_info, - module_name, sem_ir, inst_namer, vlog_stream); + module_name, sem_ir, cpp_ast, inst_namer, vlog_stream); return context.Run(); } diff --git a/toolchain/lower/lower.h b/toolchain/lower/lower.h index b1c628c4a867..90aacdc7d12d 100644 --- a/toolchain/lower/lower.h +++ b/toolchain/lower/lower.h @@ -19,7 +19,7 @@ auto LowerToLLVM(llvm::LLVMContext& llvm_context, std::optional> tree_and_subtrees_getters_for_debug_info, llvm::StringRef module_name, const SemIR::File& sem_ir, - const SemIR::InstNamer* inst_namer, + clang::ASTUnit* cpp_ast, const SemIR::InstNamer* inst_namer, llvm::raw_ostream* vlog_stream) -> std::unique_ptr; diff --git a/toolchain/lower/mangler.cpp b/toolchain/lower/mangler.cpp index 87a898815e87..b4d21bb2a78a 100644 --- a/toolchain/lower/mangler.cpp +++ b/toolchain/lower/mangler.cpp @@ -162,13 +162,9 @@ auto Mangler::Mangle(SemIR::FunctionId function_id, } auto Mangler::MangleCppClang(const clang::NamedDecl* decl) -> std::string { - if (!cpp_mangle_context_) { - // We assume all declarations are from the same AST Context. - // TODO: Consider initializing this in the constructor. This is related to: - // https://github.com/carbon-language/carbon-lang/issues/4666. See - // https://github.com/carbon-language/carbon-lang/pull/5062/files/89e56d51858bcc18d4242d4e5c9ee0e7496d887e#r1979993815 - cpp_mangle_context_.reset(decl->getASTContext().createMangleContext()); - } + CARBON_CHECK( + cpp_mangle_context_, + "Mangling of a C++ imported declaration without a Clang `MangleContext`"); RawStringOstream cpp_mangled_name; cpp_mangle_context_->mangleName(decl, cpp_mangled_name); diff --git a/toolchain/lower/mangler.h b/toolchain/lower/mangler.h index 6345e3d4372e..ff6900baa541 100644 --- a/toolchain/lower/mangler.h +++ b/toolchain/lower/mangler.h @@ -22,7 +22,12 @@ class Mangler { public: // Initialize a new Mangler instance for mangling entities within the // specified `FileContext`. - explicit Mangler(FileContext& file_context) : file_context_(file_context) {} + explicit Mangler(FileContext& file_context) + : file_context_(file_context), + cpp_mangle_context_( + file_context.cpp_ast() + ? file_context.cpp_ast()->getASTContext().createMangleContext() + : nullptr) {} // Produce a deterministically unique mangled name for the function specified // by `function_id` and `specific_id`. diff --git a/toolchain/sem_ir/file.h b/toolchain/sem_ir/file.h index dd63814a9189..4ab45525b6c3 100644 --- a/toolchain/sem_ir/file.h +++ b/toolchain/sem_ir/file.h @@ -189,7 +189,8 @@ class File : public Printable { } auto cpp_ast() -> clang::ASTUnit* { return cpp_ast_; } // TODO: When the AST can be created before creating `File`, initialize the - // pointer in the constructor and remove this function. + // pointer in the constructor and remove this function. This is part of + // https://github.com/carbon-language/carbon-lang/issues/4666 auto set_cpp_ast(clang::ASTUnit* cpp_ast) -> void { cpp_ast_ = cpp_ast; } auto names() const -> NameStoreWrapper { return NameStoreWrapper(&identifiers());