From 31598ae5fe23977bacec204c80d10de706f8b307 Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Mon, 10 Mar 2025 10:41:43 +0100 Subject: [PATCH 1/3] Initialize `cpp_mangle_context_` in `Mangler`'s constructor This is a followup of [a comment](https://github.com/carbon-language/carbon-lang/pull/5062/files/89e56d51858bcc18d4242d4e5c9ee0e7496d887e#r1979993815) in #5062. Change `File::cpp_ast()` to be `const` while keeping it returning a non-const pointer to `clang::ASTUnit`. This makes the fact we use [Clang with lack of const correctness](https://github.com/llvm/llvm-project/pull/130096#issuecomment-2704413782) explicit. Alternatives: * Change `ASTUnit::getASTContext() const` to return a non-const `ASTContext`. [Rejected 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 Clang `MangleContext::mangleName()` to `const`. * Use `const_cast` on `ASTContext` when calling `createMangleContext()`. --- toolchain/lower/mangler.cpp | 10 +++------- toolchain/lower/mangler.h | 9 ++++++++- toolchain/sem_ir/file.h | 2 +- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/toolchain/lower/mangler.cpp b/toolchain/lower/mangler.cpp index 87a898815e876..b4d21bb2a78a5 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 6345e3d4372ea..3dd32b16fe299 100644 --- a/toolchain/lower/mangler.h +++ b/toolchain/lower/mangler.h @@ -22,7 +22,14 @@ 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.sem_ir().cpp_ast() + ? file_context.sem_ir() + .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 dd63814a9189c..514139498c7bc 100644 --- a/toolchain/sem_ir/file.h +++ b/toolchain/sem_ir/file.h @@ -187,7 +187,7 @@ class File : public Printable { auto import_cpps() const -> const ValueStore& { return import_cpps_; } - auto cpp_ast() -> clang::ASTUnit* { return cpp_ast_; } + auto cpp_ast() const -> 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. auto set_cpp_ast(clang::ASTUnit* cpp_ast) -> void { cpp_ast_ = cpp_ast; } From 6ec0b8366682d43b75a780d2c18eaad37a77a597 Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Mon, 10 Mar 2025 13:11:32 +0100 Subject: [PATCH 2/3] Clarify TODO is part of #4666 --- toolchain/sem_ir/file.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/toolchain/sem_ir/file.h b/toolchain/sem_ir/file.h index 514139498c7bc..278385b8218de 100644 --- a/toolchain/sem_ir/file.h +++ b/toolchain/sem_ir/file.h @@ -189,7 +189,8 @@ class File : public Printable { } auto cpp_ast() const -> 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()); From 599c45b51be9fa4c61b387cecfdda97b6f814724 Mon Sep 17 00:00:00 2001 From: Boaz Brickner Date: Tue, 11 Mar 2025 12:32:45 +0100 Subject: [PATCH 3/3] Instead of accessing a mutable AST from a `const File`, propagate a mutable AST to `FileContext` --- 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.h | 10 ++++------ toolchain/sem_ir/file.h | 2 +- 7 files changed, 22 insertions(+), 15 deletions(-) diff --git a/toolchain/driver/compile_subcommand.cpp b/toolchain/driver/compile_subcommand.cpp index de9909854b1ee..a13d8e8f72b22 100644 --- a/toolchain/driver/compile_subcommand.cpp +++ b/toolchain/driver/compile_subcommand.cpp @@ -581,9 +581,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 228064701e88f..433ad7de981b3 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 54d2345366fef..c748fd4a9770a 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 8dec5936967d1..5a5afff7fba89 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 b1c628c4a8670..90aacdc7d12db 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.h b/toolchain/lower/mangler.h index 3dd32b16fe299..ff6900baa5417 100644 --- a/toolchain/lower/mangler.h +++ b/toolchain/lower/mangler.h @@ -24,12 +24,10 @@ class Mangler { // specified `FileContext`. explicit Mangler(FileContext& file_context) : file_context_(file_context), - cpp_mangle_context_(file_context.sem_ir().cpp_ast() - ? file_context.sem_ir() - .cpp_ast() - ->getASTContext() - .createMangleContext() - : nullptr) {} + 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 278385b8218de..4ab45525b6c31 100644 --- a/toolchain/sem_ir/file.h +++ b/toolchain/sem_ir/file.h @@ -187,7 +187,7 @@ class File : public Printable { auto import_cpps() const -> const ValueStore& { return import_cpps_; } - auto cpp_ast() const -> clang::ASTUnit* { return cpp_ast_; } + 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. This is part of // https://github.com/carbon-language/carbon-lang/issues/4666