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

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

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.

Originally landed in #7335, reverted in #7353 due to ASAN errors.
Changes since original:
* Use LLVM RTTI to make `Lower::Context::Finalize` less brittle.
Add LLVM RTTI to `ReadOnlyASTSource` (and `CarbonExternalASTSource`).
Change Finalize so that instead of just deleting the last multiplex
child source, it erases any multiplex child sources that match
`ReadOnlyASTSource`; this includes `CarbonExternalASTSource` since it's
a subclass.
* Fix ASAN error by updating the `MultiplexExternalSemaSource` earlier
in lowering. It is sometimes accessed during PrepareToLower, so update
it in `Context::GetFileContext` rather than `Context::Finalize`.

Fixes https://github.com/carbon-language/carbon-lang/issues/7142
This commit is contained in:
Nicholas Bishop
2026-06-15 20:30:07 +00:00
committed by GitHub
parent c3b6b084d4
commit f2c94517cc
12 changed files with 514 additions and 169 deletions
Download Patch File Download Diff File Expand all files Collapse all files
toolchain/check/cpp/generate_ast.cpp
+57 -90
View File
@@ -24,6 +24,7 @@
#include "common/map.h"
#include "common/raw_string_ostream.h"
#include "llvm/ADT/IntrusiveRefCntPtr.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/raw_ostream.h"
#include "toolchain/base/kind_switch.h"
@@ -41,6 +42,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 +325,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;
@@ -345,6 +348,13 @@ class CarbonExternalASTSource : public clang::ExternalSemaSource {
llvm::DenseMap<const clang::CXXRecordDecl*, clang::CharUnits>&
vbase_offsets) -> bool override;
auto isA(const void* class_id) const -> bool override {
return class_id == &id || ReadOnlyASTSource::isA(class_id);
}
static auto classof(const ExternalASTSource* s) -> bool {
return s->isA(&id);
}
private:
// Builds the top-level C++ namespace `Carbon` and adds it to the translation
// unit.
@@ -380,9 +390,14 @@ class CarbonExternalASTSource : public clang::ExternalSemaSource {
SemIR::ImportIRInst(clang_source_loc_id));
}
// For LLVM RTTI.
static char id;
Check::Context* context_;
};
char CarbonExternalASTSource::id;
} // namespace
void CarbonExternalASTSource::StartTranslationUnit(
@@ -575,54 +590,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 +598,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 +704,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 +717,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 +888,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.
@@ -1008,6 +960,21 @@ auto FinishAst(Context& context) -> void {
context.cpp_context()->sema().ActOnEndOfTranslationUnit();
// Remove the `CarbonExternalASTSource` installed in `GenerateAst` and
// replace it with a `ReadOnlyASTSource`. This is necessary because
// the source may be accessed later during lowering, but the
// `CarbonExternalASTSource` has a pointer to `Check::Context` that
// will not remain valid.
auto* multiplex_source = cast<clang::MultiplexExternalSemaSource>(
context.ast_context().getExternalSource());
auto& child_sources = multiplex_source->GetSources();
llvm::erase_if(child_sources, [](const auto& src) {
// `CarbonExternalASTSource` inherits from `ReadOnlyASTSource`.
return llvm::isa<SemIR::ReadOnlyASTSource>(src.get());
});
multiplex_source->AddSource(
llvm::makeIntrusiveRefCnt<SemIR::ReadOnlyASTSource>(context.sem_ir()));
// We don't call FrontendAction::EndSourceFile, because that destroys the AST.
context.set_cpp_context(nullptr);