[Lldb-commits] [lldb] [lldb][TypeSystemClang] Pass ClangASTMetadata around by value (PR #102161)
via lldb-commits
lldb-commits at lists.llvm.org
Tue Aug 6 08:22:06 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-lldb
Author: Michael Buch (Michael137)
<details>
<summary>Changes</summary>
This patch changes the return type of `GetMetadata` from a `ClangASTMetadata*` to a `std::optional<ClangASTMetadata>`. Except for one call-site (`SetDeclIsForcefullyCompleted`), we never actually make use of the mutability of the returned metadata. And we never make use of the pointer-identity. By passing `ClangASTMetadata` by-value (the type is fairly small, size of 2 64-bit pointers) we'll avoid some questions surrounding the lifetimes/ownership/mutability of this metadata.
For consistency, we also change the parameter to `SetMetadata` from `ClangASTMetadata&` to `ClangASTMetadata` (which is an NFC since we copy the data anyway).
This came up after some changes we plan to make where we [create redeclaration chains for decls in the LLDB AST](https://github.com/llvm/llvm-project/pull/95100). We want to avoid having to dig out the canonical decl of the declaration chain for retrieving/setting the metadata. It should just be copied across all decls in the chain. This is easier to guarantee when everything is done by-value.
---
Full diff: https://github.com/llvm/llvm-project/pull/102161.diff
7 Files Affected:
- (modified) lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp (+5-6)
- (modified) lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.h (+1-1)
- (modified) lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp (+3-3)
- (modified) lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.cpp (+2-4)
- (modified) lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp (+27-23)
- (modified) lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h (+7-6)
- (modified) lldb/unittests/Symbol/TestClangASTImporter.cpp (+3-3)
``````````diff
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
index 44071d1ea71c7..f6fb446634b18 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp
@@ -84,8 +84,7 @@ clang::Decl *ClangASTImporter::CopyDecl(clang::ASTContext *dst_ast,
LLDB_LOG_ERROR(log, result.takeError(), "Couldn't import decl: {0}");
if (log) {
lldb::user_id_t user_id = LLDB_INVALID_UID;
- ClangASTMetadata *metadata = GetDeclMetadata(decl);
- if (metadata)
+ if (auto metadata = GetDeclMetadata(decl))
user_id = metadata->GetUserID();
if (NamedDecl *named_decl = dyn_cast<NamedDecl>(decl))
@@ -950,7 +949,8 @@ bool ClangASTImporter::RequireCompleteType(clang::QualType type) {
return true;
}
-ClangASTMetadata *ClangASTImporter::GetDeclMetadata(const clang::Decl *decl) {
+std::optional<ClangASTMetadata>
+ClangASTImporter::GetDeclMetadata(const clang::Decl *decl) {
DeclOrigin decl_origin = GetDeclOrigin(decl);
if (decl_origin.Valid()) {
@@ -1105,7 +1105,7 @@ ClangASTImporter::ASTImporterDelegate::ImportImpl(Decl *From) {
// If we have a forcefully completed type, try to find an actual definition
// for it in other modules.
- const ClangASTMetadata *md = m_main.GetDeclMetadata(From);
+ auto md = m_main.GetDeclMetadata(From);
auto *td = dyn_cast<TagDecl>(From);
if (td && md && md->IsForcefullyCompleted()) {
Log *log = GetLog(LLDBLog::Expressions);
@@ -1284,8 +1284,7 @@ void ClangASTImporter::ASTImporterDelegate::Imported(clang::Decl *from,
}
lldb::user_id_t user_id = LLDB_INVALID_UID;
- ClangASTMetadata *metadata = m_main.GetDeclMetadata(from);
- if (metadata)
+ if (auto metadata = m_main.GetDeclMetadata(from))
user_id = metadata->GetUserID();
if (log) {
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.h b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.h
index bc962e544d2f1..6231f0fb25939 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.h
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.h
@@ -189,7 +189,7 @@ class ClangASTImporter {
/// is only a shallow clone that lacks any contents.
void SetDeclOrigin(const clang::Decl *decl, clang::Decl *original_decl);
- ClangASTMetadata *GetDeclMetadata(const clang::Decl *decl);
+ std::optional<ClangASTMetadata> GetDeclMetadata(const clang::Decl *decl);
//
// Namespace maps
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
index 35038a56440d5..2933d90550ffe 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangUserExpression.cpp
@@ -219,9 +219,9 @@ void ClangUserExpression::ScanContext(ExecutionContext &exe_ctx, Status &err) {
// whatever runtime the debug info says the object pointer belongs to. Do
// that here.
- ClangASTMetadata *metadata =
- TypeSystemClang::DeclContextGetMetaData(decl_context, function_decl);
- if (metadata && metadata->HasObjectPtr()) {
+ if (auto metadata = TypeSystemClang::DeclContextGetMetaData(decl_context,
+ function_decl);
+ metadata && metadata->HasObjectPtr()) {
lldb::LanguageType language = metadata->GetObjectPtrLanguage();
if (language == lldb::eLanguageTypeC_plus_plus) {
if (m_enforce_valid_object) {
diff --git a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.cpp b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.cpp
index 6894cdccaf95a..2626c86a8cbf9 100644
--- a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.cpp
+++ b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.cpp
@@ -398,9 +398,8 @@ bool AppleObjCDeclVendor::FinishDecl(clang::ObjCInterfaceDecl *interface_decl) {
Log *log(
GetLog(LLDBLog::Expressions)); // FIXME - a more appropriate log channel?
- ClangASTMetadata *metadata = m_ast_ctx->GetMetadata(interface_decl);
ObjCLanguageRuntime::ObjCISA objc_isa = 0;
- if (metadata)
+ if (auto metadata = m_ast_ctx->GetMetadata(interface_decl))
objc_isa = metadata->GetISAPtr();
if (!objc_isa)
@@ -559,8 +558,7 @@ uint32_t AppleObjCDeclVendor::FindDecls(ConstString name, bool append,
ast_ctx.getObjCInterfaceType(result_iface_decl);
uint64_t isa_value = LLDB_INVALID_ADDRESS;
- ClangASTMetadata *metadata = m_ast_ctx->GetMetadata(result_iface_decl);
- if (metadata)
+ if (auto metadata = m_ast_ctx->GetMetadata(result_iface_decl))
isa_value = metadata->GetISAPtr();
LLDB_LOGF(log,
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index 484ca04fe04c9..ef21232e18472 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -1787,8 +1787,8 @@ bool TypeSystemClang::RecordHasFields(const RecordDecl *record_decl) {
// -flimit-debug-info instead of just seeing nothing if this is a base class
// (since we were hiding empty base classes), or nothing when you turn open
// an valiable whose type was incomplete.
- ClangASTMetadata *meta_data = GetMetadata(record_decl);
- if (meta_data && meta_data->IsForcefullyCompleted())
+ if (auto meta_data = GetMetadata(record_decl);
+ meta_data && meta_data->IsForcefullyCompleted())
return true;
return false;
@@ -2465,27 +2465,31 @@ void TypeSystemClang::SetMetadataAsUserID(const clang::Type *type,
}
void TypeSystemClang::SetMetadata(const clang::Decl *object,
- ClangASTMetadata &metadata) {
+ ClangASTMetadata metadata) {
m_decl_metadata[object] = metadata;
}
void TypeSystemClang::SetMetadata(const clang::Type *object,
- ClangASTMetadata &metadata) {
+ ClangASTMetadata metadata) {
m_type_metadata[object] = metadata;
}
-ClangASTMetadata *TypeSystemClang::GetMetadata(const clang::Decl *object) {
+std::optional<ClangASTMetadata>
+TypeSystemClang::GetMetadata(const clang::Decl *object) {
auto It = m_decl_metadata.find(object);
if (It != m_decl_metadata.end())
- return &It->second;
- return nullptr;
+ return It->second;
+
+ return std::nullopt;
}
-ClangASTMetadata *TypeSystemClang::GetMetadata(const clang::Type *object) {
+std::optional<ClangASTMetadata>
+TypeSystemClang::GetMetadata(const clang::Type *object) {
auto It = m_type_metadata.find(object);
if (It != m_type_metadata.end())
- return &It->second;
- return nullptr;
+ return It->second;
+
+ return std::nullopt;
}
void TypeSystemClang::SetCXXRecordDeclAccess(const clang::CXXRecordDecl *object,
@@ -2934,9 +2938,10 @@ bool TypeSystemClang::IsRuntimeGeneratedType(
clang::ObjCInterfaceDecl *result_iface_decl =
llvm::dyn_cast<clang::ObjCInterfaceDecl>(decl_ctx);
- ClangASTMetadata *ast_metadata = GetMetadata(result_iface_decl);
+ auto ast_metadata = GetMetadata(result_iface_decl);
if (!ast_metadata)
return false;
+
return (ast_metadata->GetISAPtr() != 0);
}
@@ -3622,8 +3627,7 @@ bool TypeSystemClang::IsPossibleDynamicType(lldb::opaque_compiler_type_t type,
if (is_complete)
success = cxx_record_decl->isDynamicClass();
else {
- ClangASTMetadata *metadata = GetMetadata(cxx_record_decl);
- if (metadata)
+ if (auto metadata = GetMetadata(cxx_record_decl))
success = metadata->GetIsDynamicCXXType();
else {
is_complete = GetType(pointee_qual_type).GetCompleteType();
@@ -5325,7 +5329,7 @@ GetDynamicArrayInfo(TypeSystemClang &ast, SymbolFile *sym_file,
clang::QualType qual_type,
const ExecutionContext *exe_ctx) {
if (qual_type->isIncompleteArrayType())
- if (auto *metadata = ast.GetMetadata(qual_type.getTypePtr()))
+ if (auto metadata = ast.GetMetadata(qual_type.getTypePtr()))
return sym_file->GetDynamicArrayInfoForUID(metadata->GetUserID(),
exe_ctx);
return std::nullopt;
@@ -8866,8 +8870,7 @@ void TypeSystemClang::DumpTypeDescription(lldb::opaque_compiler_type_t type,
CompilerType ct(weak_from_this(), type);
const clang::Type *clang_type = ClangUtil::GetQualType(ct).getTypePtr();
- ClangASTMetadata *metadata = GetMetadata(clang_type);
- if (metadata) {
+ if (auto metadata = GetMetadata(clang_type)) {
metadata->Dump(&s);
}
}
@@ -9488,7 +9491,7 @@ bool TypeSystemClang::DeclContextIsClassMethod(void *opaque_decl_ctx) {
return true;
} else if (clang::FunctionDecl *fun_decl =
llvm::dyn_cast<clang::FunctionDecl>(decl_ctx)) {
- if (ClangASTMetadata *metadata = GetMetadata(fun_decl))
+ if (auto metadata = GetMetadata(fun_decl))
return metadata->HasObjectPtr();
}
@@ -9541,7 +9544,7 @@ TypeSystemClang::DeclContextGetLanguage(void *opaque_decl_ctx) {
} else if (llvm::isa<clang::CXXMethodDecl>(decl_ctx)) {
return eLanguageTypeC_plus_plus;
} else if (auto *fun_decl = llvm::dyn_cast<clang::FunctionDecl>(decl_ctx)) {
- if (ClangASTMetadata *metadata = GetMetadata(fun_decl))
+ if (auto metadata = GetMetadata(fun_decl))
return metadata->GetObjectPtrLanguage();
}
@@ -9591,7 +9594,7 @@ TypeSystemClang::DeclContextGetAsNamespaceDecl(const CompilerDeclContext &dc) {
return nullptr;
}
-ClangASTMetadata *
+std::optional<ClangASTMetadata>
TypeSystemClang::DeclContextGetMetaData(const CompilerDeclContext &dc,
const Decl *object) {
TypeSystemClang *ast = llvm::cast<TypeSystemClang>(dc.GetTypeSystem());
@@ -9825,8 +9828,7 @@ bool TypeSystemClang::IsForcefullyCompleted(lldb::opaque_compiler_type_t type) {
if (record_type) {
const clang::RecordDecl *record_decl = record_type->getDecl();
assert(record_decl);
- ClangASTMetadata *metadata = GetMetadata(record_decl);
- if (metadata)
+ if (auto metadata = GetMetadata(record_decl))
return metadata->IsForcefullyCompleted();
}
}
@@ -9836,11 +9838,13 @@ bool TypeSystemClang::IsForcefullyCompleted(lldb::opaque_compiler_type_t type) {
bool TypeSystemClang::SetDeclIsForcefullyCompleted(const clang::TagDecl *td) {
if (td == nullptr)
return false;
- ClangASTMetadata *metadata = GetMetadata(td);
- if (metadata == nullptr)
+ auto metadata = GetMetadata(td);
+ if (!metadata)
return false;
m_has_forcefully_completed_types = true;
metadata->SetIsForcefullyCompleted();
+ SetMetadata(td, *metadata);
+
return true;
}
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
index 56a5c0a516706..0aec1bcaaf9a7 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h
@@ -191,11 +191,11 @@ class TypeSystemClang : public TypeSystem {
void SetMetadataAsUserID(const clang::Decl *decl, lldb::user_id_t user_id);
void SetMetadataAsUserID(const clang::Type *type, lldb::user_id_t user_id);
- void SetMetadata(const clang::Decl *object, ClangASTMetadata &meta_data);
+ void SetMetadata(const clang::Decl *object, ClangASTMetadata meta_data);
- void SetMetadata(const clang::Type *object, ClangASTMetadata &meta_data);
- ClangASTMetadata *GetMetadata(const clang::Decl *object);
- ClangASTMetadata *GetMetadata(const clang::Type *object);
+ void SetMetadata(const clang::Type *object, ClangASTMetadata meta_data);
+ std::optional<ClangASTMetadata> GetMetadata(const clang::Decl *object);
+ std::optional<ClangASTMetadata> GetMetadata(const clang::Type *object);
void SetCXXRecordDeclAccess(const clang::CXXRecordDecl *object,
clang::AccessSpecifier access);
@@ -616,8 +616,9 @@ class TypeSystemClang : public TypeSystem {
static clang::NamespaceDecl *
DeclContextGetAsNamespaceDecl(const CompilerDeclContext &dc);
- static ClangASTMetadata *DeclContextGetMetaData(const CompilerDeclContext &dc,
- const clang::Decl *object);
+ static std::optional<ClangASTMetadata>
+ DeclContextGetMetaData(const CompilerDeclContext &dc,
+ const clang::Decl *object);
static clang::ASTContext *
DeclContextGetTypeSystemClang(const CompilerDeclContext &dc);
diff --git a/lldb/unittests/Symbol/TestClangASTImporter.cpp b/lldb/unittests/Symbol/TestClangASTImporter.cpp
index de59efe533eb9..41c7ed75155f3 100644
--- a/lldb/unittests/Symbol/TestClangASTImporter.cpp
+++ b/lldb/unittests/Symbol/TestClangASTImporter.cpp
@@ -188,7 +188,7 @@ TEST_F(TestClangASTImporter, MetadataPropagation) {
ASSERT_NE(nullptr, imported);
// Check that we got the same Metadata.
- ASSERT_NE(nullptr, importer.GetDeclMetadata(imported));
+ ASSERT_NE(std::nullopt, importer.GetDeclMetadata(imported));
EXPECT_EQ(metadata, importer.GetDeclMetadata(imported)->GetUserID());
}
@@ -219,7 +219,7 @@ TEST_F(TestClangASTImporter, MetadataPropagationIndirectImport) {
ASSERT_NE(nullptr, imported);
// Check that we got the same Metadata.
- ASSERT_NE(nullptr, importer.GetDeclMetadata(imported));
+ ASSERT_NE(std::nullopt, importer.GetDeclMetadata(imported));
EXPECT_EQ(metadata, importer.GetDeclMetadata(imported)->GetUserID());
}
@@ -244,7 +244,7 @@ TEST_F(TestClangASTImporter, MetadataPropagationAfterCopying) {
source.ast->SetMetadataAsUserID(source.record_decl, metadata);
// Check that we got the same Metadata.
- ASSERT_NE(nullptr, importer.GetDeclMetadata(imported));
+ ASSERT_NE(std::nullopt, importer.GetDeclMetadata(imported));
EXPECT_EQ(metadata, importer.GetDeclMetadata(imported)->GetUserID());
}
``````````
</details>
https://github.com/llvm/llvm-project/pull/102161
More information about the lldb-commits
mailing list