[lldb][TypeSystemClang] Pass ClangASTMetadata around by value #102161
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) ChangesThis patch changes the return type of For consistency, we also change the parameter to This came up after some changes we plan to make where we create redeclaration chains for decls in the LLDB AST. 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:
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());
}
|
JDevlieghere
approved these changes
Aug 6, 2024
| ClangASTMetadata *metadata = | ||
| TypeSystemClang::DeclContextGetMetaData(decl_context, function_decl); | ||
| if (metadata && metadata->HasObjectPtr()) { | ||
| if (auto metadata = TypeSystemClang::DeclContextGetMetaData(decl_context, |
There was a problem hiding this comment.
Nit: I know everyone has a different opinion on auto, but to me it's not obvious from the method name what the return type is, let alone that it's wrapped in an optional.
There was a problem hiding this comment.
Yea that's fair, i don't mind adding back the typenames
adrian-prantl
approved these changes
Aug 6, 2024
Michael137
added a commit
to Michael137/llvm-project
that referenced
this pull request
Aug 7, 2024
…d/objc types This is a follow-up to llvm#102161 where we changed the `GetMetadata`/`SetMetadata` APIs to pass `ClangASTMetadata` by-value, instead of `ClangASTMetadata *`, which wasn't a very friendly API. This patch continues from there and changes `CreateRecordType`/`CreateObjCClass` to take the metadata by-value as well. As a drive-by change, I also changed `DelayedAddObjCClassProperty` to store the metadata by-value, instead of in a `std::unique_ptr`, which AFAICT, was done solely due to the TypeSystemClang APIs taking the metadata by pointer.
Michael137
added a commit
that referenced
this pull request
Aug 7, 2024
…d/objc types (#102296) This is a follow-up to #102161 where we changed the `GetMetadata`/`SetMetadata` APIs to pass `ClangASTMetadata` by-value, instead of `ClangASTMetadata *`, which wasn't a very friendly API. This patch continues from there and changes `CreateRecordType`/`CreateObjCClass` to take the metadata by-value as well. As a drive-by change, I also changed `DelayedAddObjCClassProperty` to store the metadata by-value, instead of in a `std::unique_ptr`, which AFAICT, was done solely due to the TypeSystemClang APIs taking the metadata by pointer. This meant we could also get rid of the user-provided copy constructors.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch changes the return type of
GetMetadatafrom aClangASTMetadata*to astd::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 passingClangASTMetadataby-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
SetMetadatafromClangASTMetadata&toClangASTMetadata(which is an NFC since we copy the data anyway).This came up during some changes we plan to make where we create redeclaration chains for decls in the LLDB AST. 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.