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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
