From 970f38ead68b1767599a86930c93112f721c21be Mon Sep 17 00:00:00 2001 From: Zachary Turner Date: Thu, 25 Oct 2018 20:44:56 +0000 Subject: [PATCH] [NFC] Refactor SetBaseClasses and DeleteBaseClasses. We currently had a 2-step process where we had to call SetBaseClassesForType and DeleteBaseClasses. Every single caller followed this exact 2-step process, and there was manual memory management going on with raw pointers. We can do better than this by storing a vector of unique_ptrs and passing this around. This makes for a cleaner API, and we only need to call one method so there is no possibility of a user forgetting to call DeleteBaseClassSpecifiers. In addition to this, it also makes for a *simpler* API. Part of why I wanted to do this is because when I was implementing the native PDB interface I had to spend some time understanding exactly what I was deleting and why. ClangAST has significant mental overhead associated with it, and reducing the API surface can go along way to making it simpler for people to understand. Differential Revision: https://reviews.llvm.org/D53590 llvm-svn: 345312 --- lldb/include/lldb/Symbol/ClangASTContext.h | 12 ++---- .../SymbolFile/DWARF/DWARFASTParserClang.cpp | 36 ++++++++-------- .../Plugins/SymbolFile/DWARF/DWARFASTParserClang.h | 20 ++++----- .../SymbolFile/NativePDB/UdtRecordCompleter.cpp | 11 +++-- .../SymbolFile/NativePDB/UdtRecordCompleter.h | 2 +- .../source/Plugins/SymbolFile/PDB/PDBASTParser.cpp | 25 +++++------ lldb/source/Symbol/ClangASTContext.cpp | 50 +++++++++++----------- lldb/unittests/Symbol/TestClangASTContext.cpp | 25 ++++++----- 8 files changed, 87 insertions(+), 94 deletions(-) diff --git a/lldb/include/lldb/Symbol/ClangASTContext.h b/lldb/include/lldb/Symbol/ClangASTContext.h index d616a01..c726a40 100644 --- a/lldb/include/lldb/Symbol/ClangASTContext.h +++ b/lldb/include/lldb/Symbol/ClangASTContext.h @@ -858,18 +858,14 @@ public: void AddMethodOverridesForCXXRecordType(lldb::opaque_compiler_type_t type); // C++ Base Classes - clang::CXXBaseSpecifier * + std::unique_ptr CreateBaseClassSpecifier(lldb::opaque_compiler_type_t type, lldb::AccessType access, bool is_virtual, bool base_of_class); - static void DeleteBaseClassSpecifiers(clang::CXXBaseSpecifier **base_classes, - unsigned num_base_classes); - - bool - SetBaseClassesForClassType(lldb::opaque_compiler_type_t type, - clang::CXXBaseSpecifier const *const *base_classes, - unsigned num_base_classes); + bool TransferBaseClasses( + lldb::opaque_compiler_type_t type, + std::vector> bases); static bool SetObjCSuperClass(const CompilerType &type, const CompilerType &superclass_compiler_type); diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp index 30c0458..718ae66 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp @@ -2184,14 +2184,14 @@ bool DWARFASTParserClang::CompleteTypeFromDWARF(const DWARFDIE &die, } SymbolContext sc(die.GetLLDBCompileUnit()); - std::vector base_classes; + std::vector> bases; std::vector member_accessibilities; bool is_a_class = false; // Parse members and base classes first DWARFDIECollection member_function_dies; DelayedPropertyList delayed_properties; - ParseChildMembers(sc, die, clang_type, class_language, base_classes, + ParseChildMembers(sc, die, clang_type, class_language, bases, member_accessibilities, member_function_dies, delayed_properties, default_accessibility, is_a_class, layout_info); @@ -2255,11 +2255,11 @@ bool DWARFASTParserClang::CompleteTypeFromDWARF(const DWARFDIE &die, &member_accessibilities.front(), member_accessibilities.size()); } - if (!base_classes.empty()) { + if (!bases.empty()) { // Make sure all base classes refer to complete types and not forward // declarations. If we don't do this, clang will crash with an - // assertion in the call to clang_type.SetBaseClassesForClassType() - for (auto &base_class : base_classes) { + // assertion in the call to clang_type.TransferBaseClasses() + for (const auto &base_class : bases) { clang::TypeSourceInfo *type_source_info = base_class->getTypeSourceInfo(); if (type_source_info) { @@ -2278,7 +2278,7 @@ bool DWARFASTParserClang::CompleteTypeFromDWARF(const DWARFDIE &die, // We have no choice other than to pretend that the base class // is complete. If we don't do this, clang will crash when we // call setBases() inside of - // "clang_type.SetBaseClassesForClassType()" below. Since we + // "clang_type.TransferBaseClasses()" below. Since we // provide layout assistance, all ivars in this class and other // classes will be fine, this is the best we can do short of // crashing. @@ -2290,14 +2290,9 @@ bool DWARFASTParserClang::CompleteTypeFromDWARF(const DWARFDIE &die, } } } - m_ast.SetBaseClassesForClassType(clang_type.GetOpaqueQualType(), - &base_classes.front(), - base_classes.size()); - - // Clang will copy each CXXBaseSpecifier in "base_classes" so we have - // to free them all. - ClangASTContext::DeleteBaseClassSpecifiers(&base_classes.front(), - base_classes.size()); + + m_ast.TransferBaseClasses(clang_type.GetOpaqueQualType(), + std::move(bases)); } } } @@ -2675,7 +2670,7 @@ Function *DWARFASTParserClang::ParseFunctionFromDWARF(const SymbolContext &sc, bool DWARFASTParserClang::ParseChildMembers( const SymbolContext &sc, const DWARFDIE &parent_die, CompilerType &class_clang_type, const LanguageType class_language, - std::vector &base_classes, + std::vector> &base_classes, std::vector &member_accessibilities, DWARFDIECollection &member_function_dies, DelayedPropertyList &delayed_properties, AccessType &default_accessibility, @@ -3271,9 +3266,14 @@ bool DWARFASTParserClang::ParseChildMembers( if (class_language == eLanguageTypeObjC) { ast->SetObjCSuperClass(class_clang_type, base_class_clang_type); } else { - base_classes.push_back(ast->CreateBaseClassSpecifier( - base_class_clang_type.GetOpaqueQualType(), accessibility, - is_virtual, is_base_of_class)); + std::unique_ptr result = + ast->CreateBaseClassSpecifier( + base_class_clang_type.GetOpaqueQualType(), accessibility, + is_virtual, is_base_of_class); + if (!result) + break; + + base_classes.push_back(std::move(result)); if (is_virtual) { // Do not specify any offset for virtual inheritance. The DWARF diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h index 57c1fc0..f444ee2 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h @@ -80,16 +80,16 @@ protected: lldb_private::ClangASTContext::TemplateParameterInfos &template_param_infos); - bool - ParseChildMembers(const lldb_private::SymbolContext &sc, const DWARFDIE &die, - lldb_private::CompilerType &class_compiler_type, - const lldb::LanguageType class_language, - std::vector &base_classes, - std::vector &member_accessibilities, - DWARFDIECollection &member_function_dies, - DelayedPropertyList &delayed_properties, - lldb::AccessType &default_accessibility, bool &is_a_class, - lldb_private::ClangASTImporter::LayoutInfo &layout_info); + bool ParseChildMembers( + const lldb_private::SymbolContext &sc, const DWARFDIE &die, + lldb_private::CompilerType &class_compiler_type, + const lldb::LanguageType class_language, + std::vector> &base_classes, + std::vector &member_accessibilities, + DWARFDIECollection &member_function_dies, + DelayedPropertyList &delayed_properties, + lldb::AccessType &default_accessibility, bool &is_a_class, + lldb_private::ClangASTImporter::LayoutInfo &layout_info); size_t ParseChildParameters(const lldb_private::SymbolContext &sc, diff --git a/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp b/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp index 2539f0c..88848a6 100644 --- a/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp +++ b/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp @@ -59,12 +59,12 @@ lldb::opaque_compiler_type_t UdtRecordCompleter::AddBaseClassForTypeIndex( CVType udt_cvt = m_symbol_file.m_index->tpi().getType(ti); lldb::opaque_compiler_type_t base_qt = base_ct.GetOpaqueQualType(); - clang::CXXBaseSpecifier *base_spec = + std::unique_ptr base_spec = m_symbol_file.GetASTContext().CreateBaseClassSpecifier( base_qt, TranslateMemberAccess(access), false, udt_cvt.kind() == LF_CLASS); - - m_bases.push_back(base_spec); + lldbassert(base_spec); + m_bases.push_back(std::move(base_spec)); return base_qt; } @@ -172,9 +172,8 @@ Error UdtRecordCompleter::visitKnownMember(CVMemberRecord &cvr, void UdtRecordCompleter::complete() { ClangASTContext &clang = m_symbol_file.GetASTContext(); - clang.SetBaseClassesForClassType(m_derived_ct.GetOpaqueQualType(), - m_bases.data(), m_bases.size()); - ClangASTContext::DeleteBaseClassSpecifiers(m_bases.data(), m_bases.size()); + clang.TransferBaseClasses(m_derived_ct.GetOpaqueQualType(), + std::move(m_bases)); clang.AddMethodOverridesForCXXRecordType(m_derived_ct.GetOpaqueQualType()); ClangASTContext::BuildIndirectFields(m_derived_ct); diff --git a/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h b/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h index 76c1ad5..f20452e 100644 --- a/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h +++ b/lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h @@ -40,7 +40,7 @@ class UdtRecordCompleter : public llvm::codeview::TypeVisitorCallbacks { CompilerType &m_derived_ct; clang::TagDecl &m_tag_decl; SymbolFileNativePDB &m_symbol_file; - std::vector m_bases; + std::vector> m_bases; ClangASTImporter::LayoutInfo m_layout; public: diff --git a/lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp b/lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp index 150e4c8..96795ff 100644 --- a/lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp +++ b/lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp @@ -1208,7 +1208,8 @@ void PDBASTParser::AddRecordBases( lldb_private::CompilerType &record_type, int record_kind, PDBBaseClassSymbolEnumerator &bases_enum, lldb_private::ClangASTImporter::LayoutInfo &layout_info) const { - std::vector base_classes; + std::vector> base_classes; + while (auto base = bases_enum.getNext()) { auto base_type = symbol_file.ResolveTypeUID(base->getTypeId()); if (!base_type) @@ -1229,13 +1230,13 @@ void PDBASTParser::AddRecordBases( auto is_virtual = base->isVirtualBaseClass(); - auto base_class_spec = m_ast.CreateBaseClassSpecifier( - base_comp_type.GetOpaqueQualType(), access, is_virtual, - record_kind == clang::TTK_Class); - if (!base_class_spec) - continue; + std::unique_ptr base_spec = + m_ast.CreateBaseClassSpecifier(base_comp_type.GetOpaqueQualType(), + access, is_virtual, + record_kind == clang::TTK_Class); + lldbassert(base_spec); - base_classes.push_back(base_class_spec); + base_classes.push_back(std::move(base_spec)); if (is_virtual) continue; @@ -1247,13 +1248,9 @@ void PDBASTParser::AddRecordBases( auto offset = clang::CharUnits::fromQuantity(base->getOffset()); layout_info.base_offsets.insert(std::make_pair(decl, offset)); } - if (!base_classes.empty()) { - m_ast.SetBaseClassesForClassType(record_type.GetOpaqueQualType(), - &base_classes.front(), - base_classes.size()); - ClangASTContext::DeleteBaseClassSpecifiers(&base_classes.front(), - base_classes.size()); - } + + m_ast.TransferBaseClasses(record_type.GetOpaqueQualType(), + std::move(base_classes)); } void PDBASTParser::AddRecordMethods(lldb_private::SymbolFile &symbol_file, diff --git a/lldb/source/Symbol/ClangASTContext.cpp b/lldb/source/Symbol/ClangASTContext.cpp index 39ffc4c..f0ec5e0 100644 --- a/lldb/source/Symbol/ClangASTContext.cpp +++ b/lldb/source/Symbol/ClangASTContext.cpp @@ -8239,39 +8239,37 @@ void ClangASTContext::AddMethodOverridesForCXXRecordType( #pragma mark C++ Base Classes -clang::CXXBaseSpecifier * +std::unique_ptr ClangASTContext::CreateBaseClassSpecifier(lldb::opaque_compiler_type_t type, AccessType access, bool is_virtual, bool base_of_class) { - if (type) - return new clang::CXXBaseSpecifier( - clang::SourceRange(), is_virtual, base_of_class, - ClangASTContext::ConvertAccessTypeToAccessSpecifier(access), - getASTContext()->getTrivialTypeSourceInfo(GetQualType(type)), - clang::SourceLocation()); - return nullptr; -} + if (!type) + return nullptr; -void ClangASTContext::DeleteBaseClassSpecifiers( - clang::CXXBaseSpecifier **base_classes, unsigned num_base_classes) { - for (unsigned i = 0; i < num_base_classes; ++i) { - delete base_classes[i]; - base_classes[i] = nullptr; - } + return llvm::make_unique( + clang::SourceRange(), is_virtual, base_of_class, + ClangASTContext::ConvertAccessTypeToAccessSpecifier(access), + getASTContext()->getTrivialTypeSourceInfo(GetQualType(type)), + clang::SourceLocation()); } -bool ClangASTContext::SetBaseClassesForClassType( +bool ClangASTContext::TransferBaseClasses( lldb::opaque_compiler_type_t type, - clang::CXXBaseSpecifier const *const *base_classes, - unsigned num_base_classes) { - if (type) { - clang::CXXRecordDecl *cxx_record_decl = GetAsCXXRecordDecl(type); - if (cxx_record_decl) { - cxx_record_decl->setBases(base_classes, num_base_classes); - return true; - } - } - return false; + std::vector> bases) { + if (!type) + return false; + clang::CXXRecordDecl *cxx_record_decl = GetAsCXXRecordDecl(type); + if (!cxx_record_decl) + return false; + std::vector raw_bases; + raw_bases.reserve(bases.size()); + + // Clang will make a copy of them, so it's ok that we pass pointers that we're + // about to destroy. + for (auto &b : bases) + raw_bases.push_back(b.get()); + cxx_record_decl->setBases(raw_bases.data(), raw_bases.size()); + return true; } bool ClangASTContext::SetObjCSuperClass( diff --git a/lldb/unittests/Symbol/TestClangASTContext.cpp b/lldb/unittests/Symbol/TestClangASTContext.cpp index c9737fa..e422e3b 100644 --- a/lldb/unittests/Symbol/TestClangASTContext.cpp +++ b/lldb/unittests/Symbol/TestClangASTContext.cpp @@ -337,15 +337,19 @@ TEST_F(TestClangASTContext, TestRecordHasFields) { EXPECT_NE(nullptr, non_empty_base_field_decl); EXPECT_TRUE(ClangASTContext::RecordHasFields(non_empty_base_decl)); + std::vector> bases; + // Test that a record with no direct fields, but fields in a base returns true CompilerType empty_derived = m_ast->CreateRecordType( nullptr, lldb::eAccessPublic, "EmptyDerived", clang::TTK_Struct, lldb::eLanguageTypeC_plus_plus, nullptr); ClangASTContext::StartTagDeclarationDefinition(empty_derived); - CXXBaseSpecifier *non_empty_base_spec = m_ast->CreateBaseClassSpecifier( - non_empty_base.GetOpaqueQualType(), lldb::eAccessPublic, false, false); - bool result = m_ast->SetBaseClassesForClassType( - empty_derived.GetOpaqueQualType(), &non_empty_base_spec, 1); + std::unique_ptr non_empty_base_spec = + m_ast->CreateBaseClassSpecifier(non_empty_base.GetOpaqueQualType(), + lldb::eAccessPublic, false, false); + bases.push_back(std::move(non_empty_base_spec)); + bool result = m_ast->TransferBaseClasses(empty_derived.GetOpaqueQualType(), + std::move(bases)); ClangASTContext::CompleteTagDeclarationDefinition(empty_derived); EXPECT_TRUE(result); CXXRecordDecl *empty_derived_non_empty_base_cxx_decl = @@ -363,10 +367,12 @@ TEST_F(TestClangASTContext, TestRecordHasFields) { nullptr, lldb::eAccessPublic, "EmptyDerived2", clang::TTK_Struct, lldb::eLanguageTypeC_plus_plus, nullptr); ClangASTContext::StartTagDeclarationDefinition(empty_derived2); - CXXBaseSpecifier *non_empty_vbase_spec = m_ast->CreateBaseClassSpecifier( - non_empty_base.GetOpaqueQualType(), lldb::eAccessPublic, true, false); - result = m_ast->SetBaseClassesForClassType(empty_derived2.GetOpaqueQualType(), - &non_empty_vbase_spec, 1); + std::unique_ptr non_empty_vbase_spec = + m_ast->CreateBaseClassSpecifier(non_empty_base.GetOpaqueQualType(), + lldb::eAccessPublic, true, false); + bases.push_back(std::move(non_empty_vbase_spec)); + result = m_ast->TransferBaseClasses(empty_derived2.GetOpaqueQualType(), + std::move(bases)); ClangASTContext::CompleteTagDeclarationDefinition(empty_derived2); EXPECT_TRUE(result); CXXRecordDecl *empty_derived_non_empty_vbase_cxx_decl = @@ -377,9 +383,6 @@ TEST_F(TestClangASTContext, TestRecordHasFields) { empty_derived_non_empty_vbase_cxx_decl, false)); EXPECT_TRUE( ClangASTContext::RecordHasFields(empty_derived_non_empty_vbase_decl)); - - delete non_empty_base_spec; - delete non_empty_vbase_spec; } TEST_F(TestClangASTContext, TemplateArguments) { -- 2.7.4