From 12a89e14b83ac3db9e44f535a43bb11e7b6c3601 Mon Sep 17 00:00:00 2001 From: Raphael Isemann Date: Thu, 22 Jul 2021 13:31:23 +0200 Subject: [PATCH] [lldb][NFCI] Remove redundant accessibility heuristic in the DWARF parser LLDB's DWARF parser has some heuristics for guessing and fixing up the accessibility of C++ class/struct members after they were already created in the internal Clang AST. The heuristic is that if a struct/class has a base class, then it's actually a class and it's members are private unless otherwise specified. From what I can see this heuristic isn't sound and also unnecessary. The idea that inheritance implies that the `class` keyword was used and the default visibility is `private` is incorrect. Also both GCC and Clang use `DW_TAG_structure_type` and `DW_TAG_class_type` for `struct` and `class` types respectively, so the default visibility we infer from that information is always correct and there is no need to fix it up. And finally, the access specifiers we set in the Clang AST are anyway unused within LLDB. The expression parser explicitly ignores them to give users access to private members and there is not SBAPI functionality that exposes this information. This patch removes all the heuristic code for the reasons above and instead just relies on the access values we infer from the tag kind and explicit annotations in DWARF. This patch is NFCI. Reviewed By: werat Differential Revision: https://reviews.llvm.org/D105463 --- .../SymbolFile/DWARF/DWARFASTParserClang.cpp | 44 +++------------------- .../Plugins/SymbolFile/DWARF/DWARFASTParserClang.h | 4 +- .../Plugins/TypeSystem/Clang/TypeSystemClang.cpp | 36 ------------------ .../Plugins/TypeSystem/Clang/TypeSystemClang.h | 7 ---- 4 files changed, 6 insertions(+), 85 deletions(-) diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp index f10fbce..4cf4ea6 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp @@ -1983,15 +1983,12 @@ bool DWARFASTParserClang::CompleteRecordType(const DWARFDIE &die, } std::vector> bases; - std::vector member_accessibilities; - bool is_a_class = false; // Parse members and base classes first std::vector member_function_dies; DelayedPropertyList delayed_properties; - ParseChildMembers(die, clang_type, bases, member_accessibilities, - member_function_dies, delayed_properties, - default_accessibility, is_a_class, layout_info); + ParseChildMembers(die, clang_type, bases, member_function_dies, + delayed_properties, default_accessibility, layout_info); // Now parse any methods if there were any... for (const DWARFDIE &die : member_function_dies) @@ -2012,31 +2009,6 @@ bool DWARFASTParserClang::CompleteRecordType(const DWARFDIE &die, } } - // If we have a DW_TAG_structure_type instead of a DW_TAG_class_type we - // need to tell the clang type it is actually a class. - if (!type_is_objc_object_or_interface) { - if (is_a_class && tag_decl_kind != clang::TTK_Class) - m_ast.SetTagTypeKind(ClangUtil::GetQualType(clang_type), - clang::TTK_Class); - } - - // Since DW_TAG_structure_type gets used for both classes and - // structures, we may need to set any DW_TAG_member fields to have a - // "private" access if none was specified. When we parsed the child - // members we tracked that actual accessibility value for each - // DW_TAG_member in the "member_accessibilities" array. If the value - // for the member is zero, then it was set to the - // "default_accessibility" which for structs was "public". Below we - // correct this by setting any fields to "private" that weren't - // correctly set. - if (is_a_class && !member_accessibilities.empty()) { - // This is a class and all members that didn't have their access - // specified are private. - m_ast.SetDefaultAccessForRecordFields( - m_ast.GetAsRecordDecl(clang_type), eAccessPrivate, - &member_accessibilities.front(), member_accessibilities.size()); - } - 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 @@ -2349,7 +2321,6 @@ Function *DWARFASTParserClang::ParseFunctionFromDWARF(CompileUnit &comp_unit, void DWARFASTParserClang::ParseSingleMember( const DWARFDIE &die, const DWARFDIE &parent_die, const lldb_private::CompilerType &class_clang_type, - std::vector &member_accessibilities, lldb::AccessType default_accessibility, DelayedPropertyList &delayed_properties, lldb_private::ClangASTImporter::LayoutInfo &layout_info, @@ -2557,7 +2528,6 @@ void DWARFASTParserClang::ParseSingleMember( if (accessibility == eAccessNone) accessibility = default_accessibility; - member_accessibilities.push_back(accessibility); uint64_t field_bit_offset = (member_byte_offset == UINT32_MAX ? 0 : (member_byte_offset * 8)); @@ -2767,10 +2737,9 @@ void DWARFASTParserClang::ParseSingleMember( bool DWARFASTParserClang::ParseChildMembers( const DWARFDIE &parent_die, CompilerType &class_clang_type, std::vector> &base_classes, - std::vector &member_accessibilities, std::vector &member_function_dies, DelayedPropertyList &delayed_properties, AccessType &default_accessibility, - bool &is_a_class, ClangASTImporter::LayoutInfo &layout_info) { + ClangASTImporter::LayoutInfo &layout_info) { if (!parent_die) return false; @@ -2790,8 +2759,8 @@ bool DWARFASTParserClang::ParseChildMembers( case DW_TAG_member: case DW_TAG_APPLE_property: ParseSingleMember(die, parent_die, class_clang_type, - member_accessibilities, default_accessibility, - delayed_properties, layout_info, last_field_info); + default_accessibility, delayed_properties, layout_info, + last_field_info); break; case DW_TAG_subprogram: @@ -2800,9 +2769,6 @@ bool DWARFASTParserClang::ParseChildMembers( break; case DW_TAG_inheritance: { - is_a_class = true; - if (default_accessibility == eAccessNone) - default_accessibility = eAccessPrivate; // TODO: implement DW_TAG_inheritance type parsing DWARFAttributes attributes; const size_t num_attributes = die.GetAttributes(attributes); diff --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h index e13716b..9bf6240 100644 --- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h +++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h @@ -111,10 +111,9 @@ protected: bool ParseChildMembers( const DWARFDIE &die, lldb_private::CompilerType &class_compiler_type, std::vector> &base_classes, - std::vector &member_accessibilities, std::vector &member_function_dies, DelayedPropertyList &delayed_properties, - lldb::AccessType &default_accessibility, bool &is_a_class, + lldb::AccessType &default_accessibility, lldb_private::ClangASTImporter::LayoutInfo &layout_info); size_t @@ -194,7 +193,6 @@ private: void ParseSingleMember(const DWARFDIE &die, const DWARFDIE &parent_die, const lldb_private::CompilerType &class_clang_type, - std::vector &member_accessibilities, lldb::AccessType default_accessibility, DelayedPropertyList &delayed_properties, lldb_private::ClangASTImporter::LayoutInfo &layout_info, diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp index df21c00..7150fdc 100644 --- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp +++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp @@ -2575,42 +2575,6 @@ ClangASTMetadata *TypeSystemClang::GetMetadata(const clang::Type *object) { return nullptr; } -bool TypeSystemClang::SetTagTypeKind(clang::QualType tag_qual_type, - int kind) const { - const clang::Type *clang_type = tag_qual_type.getTypePtr(); - if (clang_type) { - const clang::TagType *tag_type = llvm::dyn_cast(clang_type); - if (tag_type) { - clang::TagDecl *tag_decl = - llvm::dyn_cast(tag_type->getDecl()); - if (tag_decl) { - tag_decl->setTagKind((clang::TagDecl::TagKind)kind); - return true; - } - } - } - return false; -} - -bool TypeSystemClang::SetDefaultAccessForRecordFields( - clang::RecordDecl *record_decl, int default_accessibility, - int *assigned_accessibilities, size_t num_assigned_accessibilities) { - if (record_decl) { - uint32_t field_idx; - clang::RecordDecl::field_iterator field, field_end; - for (field = record_decl->field_begin(), - field_end = record_decl->field_end(), field_idx = 0; - field != field_end; ++field, ++field_idx) { - // If no accessibility was assigned, assign the correct one - if (field_idx < num_assigned_accessibilities && - assigned_accessibilities[field_idx] == clang::AS_none) - field->setAccess((clang::AccessSpecifier)default_accessibility); - } - return true; - } - return false; -} - clang::DeclContext * TypeSystemClang::GetDeclContextForType(const CompilerType &type) { return GetDeclContextForType(ClangUtil::GetQualType(type)); diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h index 62679e9..701e4ca 100644 --- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h +++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h @@ -380,13 +380,6 @@ public: bool isForwardDecl, bool isInternal, ClangASTMetadata *metadata = nullptr); - bool SetTagTypeKind(clang::QualType type, int kind) const; - - bool SetDefaultAccessForRecordFields(clang::RecordDecl *record_decl, - int default_accessibility, - int *assigned_accessibilities, - size_t num_assigned_accessibilities); - // Returns a mask containing bits from the TypeSystemClang::eTypeXXX // enumerations -- 2.7.4