carbon-language/carbon-lang
1
0
Fork 0
mirror of https://github.com/carbon-language/carbon-lang.git synced 2026-10-05 07:11:04 +01:00
Code Issues Packages Projects Releases 10 Wiki Activity

Fix crash from accessing a Check::Context during lowering (#7335)

Browse Source 
In generate_ast.cpp, an `CarbonExternalASTSource` is installed that has
a `Check::Context` pointer. During lowering, this `ExternalASTSource` is
still installed, and using it can cause a crash if the now-invalid
pointer is dereferenced.

Fix by adding a new `ReadOnlyASTSource` in sem_ir, and using that during
lowering.

`CarbonExternalASTSource` now inherits from `ReadOnlyASTSource` to avoid
some code duplication.

In generate_ast.cpp, we now always install a multiplex source, even if
there's only one child source. Clang internally keeps pointers to the
top-level `ExternalASTSource` installed via `setExternalSource`, and
those pointers aren't updated if `setExternalSource` is called again. By
using `MultiplexExternalSemaSource`, we can keep the top-level
`ExternalASTSource` pointer the same, and only update its children.

Using `MultiplexExternalSemaSource` this way requires a new constructor
and a method to modify its child sources; added a new LLVM patch adding
those.

https://github.com/carbon-language/carbon-lang/issues/7142
This commit is contained in:
Nicholas Bishop
2026-06-12 18:05:19 +00:00
committed by GitHub
parent f3b8e231ca
commit afd679129d
14 changed files with 495 additions and 169 deletions
Download Patch File Download Diff File Expand all files Collapse all files
toolchain/check/cpp/generate_ast.cpp
+29 -90
View File
@@ -41,6 +41,7 @@
#include "toolchain/diagnostics/format_providers.h"
#include "toolchain/parse/node_ids.h"
#include "toolchain/sem_ir/cpp_file.h"
#include "toolchain/sem_ir/read_only_ast_source.h"
#include "toolchain/sem_ir/typed_insts.h"
namespace Carbon::Check {
@@ -323,9 +324,10 @@ class ShallowCopyCompilerInvocation : public clang::CompilerInvocation {
};
// Provides clang AST nodes representing Carbon SemIR entities.
class CarbonExternalASTSource : public clang::ExternalSemaSource {
class CarbonExternalASTSource : public SemIR::ReadOnlyASTSource {
public:
explicit CarbonExternalASTSource(Context* context) : context_(context) {}
explicit CarbonExternalASTSource(Context* context)
: ReadOnlyASTSource(context->sem_ir()), context_(context) {}
auto StartTranslationUnit(clang::ASTConsumer* consumer) -> void override;
@@ -575,54 +577,6 @@ auto CarbonExternalASTSource::FindExternalVisibleDeclsByName(
}
}
// If this declaration declares a class type that is "owned" by Carbon, and not
// imported from C++, returns the corresponding type ID and `ClassType`.
// Otherwise returns `nullopt`.
static auto GetAsCarbonOwnedClass(Context& context,
const clang::TagDecl* tag_decl)
-> std::optional<std::pair<SemIR::TypeId, SemIR::ClassType>> {
// Quickly check whether we could possibly own this class.
// TODO: Once we multiplex with the ASTReader, handle
// ASTReader::completeVisibleDeclsMap setting this to `false`.
if (!tag_decl->hasExternalVisibleStorage()) {
return std::nullopt;
}
auto key = SemIR::ClangDeclKey::ForNonFunctionDecl(
const_cast<clang::TagDecl*>(tag_decl->getFirstDecl()));
auto clang_decl_id = context.clang_decls().LookupId(key);
if (!clang_decl_id.has_value()) {
return std::nullopt;
}
auto inst_id = context.clang_decls().Get(clang_decl_id).inst_id;
auto const_id = context.constant_values().Get(inst_id);
if (!const_id.has_value()) {
return std::nullopt;
}
auto class_type =
context.constant_values().TryGetInstAs<SemIR::ClassType>(const_id);
if (!class_type) {
return std::nullopt;
}
// Determine whether this class was imported from C++.
// TODO: This currently can't happen, because only Carbon classes have
// external lexical storage, but will happen once we support importing C++
// classes from AST files. Add a test once that is supported.
// TODO: Consider setting `extern_library_id` on classes imported from C++ to
// indicate the current file does not own them.
const auto& class_info = context.classes().Get(class_type->class_id);
if (class_info.parent_scope_id.has_value() &&
context.name_scopes().Get(class_info.parent_scope_id).is_cpp_scope()) {
return std::nullopt;
}
auto class_type_id = context.types().GetTypeIdForTypeConstantId(const_id);
return std::make_pair(class_type_id, *class_type);
}
auto CarbonExternalASTSource::CompleteType(clang::TagDecl* tag_decl) -> void {
auto* class_decl = dyn_cast<clang::CXXRecordDecl>(tag_decl);
if (!class_decl) {
@@ -631,7 +585,8 @@ auto CarbonExternalASTSource::CompleteType(clang::TagDecl* tag_decl) -> void {
return;
}
auto carbon_class_info = GetAsCarbonOwnedClass(*context_, tag_decl);
auto carbon_class_info =
SemIR::GetAsCarbonOwnedClass(context_->sem_ir(), tag_decl);
if (!carbon_class_info) {
return;
}
@@ -736,7 +691,8 @@ auto CarbonExternalASTSource::layoutRecordType(
llvm::DenseMap<const clang::CXXRecordDecl*, clang::CharUnits>& base_offsets,
llvm::DenseMap<const clang::CXXRecordDecl*, clang::CharUnits>&
vbase_offsets) -> bool {
auto carbon_class_info = GetAsCarbonOwnedClass(*context_, record_decl);
auto carbon_class_info =
SemIR::GetAsCarbonOwnedClass(context_->sem_ir(), record_decl);
if (!carbon_class_info) {
return false;
}
@@ -748,37 +704,11 @@ auto CarbonExternalASTSource::layoutRecordType(
// general.
CompleteTypeOrCheckFail(*context_, class_type_id);
// Set the overall size and alignment. We round up the size to an integer
// number of bytes in order to avoid surprising Clang too much.
auto layout = context_->sem_ir()
.types()
.GetCompleteTypeInfo(class_type_id)
.object_layout;
size = layout.size.bytes() * 8;
alignment = layout.alignment.bits();
auto& class_info = context_->classes().Get(class_type.class_id);
ExportAllFieldsToCpp(*context_, class_info);
// Fill in `field_offsets`.
CalculateCppFieldOffsets(*context_, class_type.class_id, field_offsets);
// Add offset for base class, if any.
if (const auto* class_decl = dyn_cast<clang::CXXRecordDecl>(record_decl);
class_decl && !class_decl->bases().empty()) {
CARBON_CHECK(class_decl->getNumBases() == 1,
"Carbon class with multiple bases");
const auto& base = *class_decl->bases_begin();
// TODO: If this class introduced a vptr, the base will be at an offset of
// `sizeof(void*)`, not 0.
base_offsets.insert(
{base.getType()->getAsCXXRecordDecl()->getCanonicalDecl(),
clang::CharUnits::Zero()});
// TODO: Support deriving from a C++ class with virtual bases.
CARBON_CHECK(class_decl->getNumVBases() == 0,
"Carbon class with multiple bases");
static_cast<void>(vbase_offsets);
}
return true;
return ReadOnlyASTSource::layoutRecordType(
record_decl, size, alignment, field_offsets, base_offsets, vbase_offsets);
}
// Parses a sequence of top-level declarations and forms a corresponding
@@ -945,17 +875,26 @@ auto GenerateAst(Context& context,
context.sem_ir().cpp_file()->CreateMangleContext();
auto& ast = clang_instance.getASTContext();
llvm::IntrusiveRefCntPtr<clang::ExternalSemaSource> carbon_source =
llvm::makeIntrusiveRefCnt<CarbonExternalASTSource>(&context);
// Always build a multiplex source, even if there's only one child
// source. During lowering, the `CarbonExternalASTSource` can no longer be
// used (because it uses `Check::Context`), so a `ReadOnlyASTSource` is
// installed instead. However, clang internally keeps pointers to the
// top-level `ExternalASTSource` installed via `setExternalSource`, and
// those pointers aren't updated if `setExternalSource` is called again. By
// using `MultiplexExternalSemaSource`, we can keep the top-level
// `ExternalASTSource` pointer the same, and only update its children.
auto multiplex_source_ref_cnt_ptr =
llvm::makeIntrusiveRefCnt<clang::MultiplexExternalSemaSource>();
auto* multiplex_source = cast<clang::MultiplexExternalSemaSource>(
multiplex_source_ref_cnt_ptr.get());
if (auto* existing_source = llvm::cast_or_null<clang::ExternalSemaSource>(
ast.getExternalSource())) {
auto multiplex_source =
llvm::makeIntrusiveRefCnt<clang::MultiplexExternalSemaSource>(
existing_source, std::move(carbon_source));
ast.setExternalSource(std::move(multiplex_source));
} else {
ast.setExternalSource(std::move(carbon_source));
multiplex_source->AddSource(existing_source);
}
multiplex_source->AddSource(
llvm::makeIntrusiveRefCnt<CarbonExternalASTSource>(&context));
ast.setExternalSource(std::move(multiplex_source_ref_cnt_ptr));
if (llvm::Error error = action.Execute()) {
// `Execute` currently never fails, but its contract allows it to.