[lldb][NFC] Remove redundant accessibility heuristic in the DWARF parser

[teemperor]

Summary

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 this code for the reasons above. This should be NFC.

[werat]

Given that the access modifiers are not used by LLDB and none of the tests fail because of this cleanup, looks good to me :)

[shafik]

So it is not obvious to me looking at these changes, are we just removing code that gets access wrong or are we removing all setting of access?

I was look at clang Sema code and it does use access to make some decisions and sometimes asserts, it is not obvious to me if we can hit these caeses through expression parsing though.

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h

protected:
   bool ParseTemplateParameterInfos(
       const DWARFDIE &parent_die,
       lldb_private::TypeSystemClang::TemplateParameterInfos
           &template_param_infos);
 
   bool ParseChildMembers(
       const DWARFDIE &die, lldb_private::CompilerType &class_compiler_type,
       std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> &base_classes,
-      std::vector<int> &member_accessibilities,
       std::vector<DWARFDIE> &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
   ParseChildParameters(clang::DeclContext *containing_decl_ctx,
                        const DWARFDIE &parent_die, bool skip_artificial,
                        bool &is_static, bool &is_variadic,
                        bool &has_template_params,
                        std::vector<lldb_private::CompilerType> &function_args,
bool NextBitfieldOffsetIsValid(const uint64_t next_bit_offset) const {
       // bit offset than any previous bitfield + size.
       return (bit_size + bit_offset) <= next_bit_offset;
     }
   };
 
   void
   ParseSingleMember(const DWARFDIE &die, const DWARFDIE &parent_die,
                     const lldb_private::CompilerType &class_clang_type,
-                    std::vector<int> &member_accessibilities,
                     lldb::AccessType default_accessibility,
                     DelayedPropertyList &delayed_properties,
                     lldb_private::ClangASTImporter::LayoutInfo &layout_info,
                     FieldInfo &last_field_info);
 
   bool CompleteRecordType(const DWARFDIE &die, lldb_private::Type *type,
                           lldb_private::CompilerType &clang_type);
   bool CompleteEnumType(const DWARFDIE &die, lldb_private::Type *type,

lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp

if (tag == DW_TAG_structure_type) {
       tag_decl_kind = clang::TTK_Union;
       default_accessibility = eAccessPublic;
     } else if (tag == DW_TAG_class_type) {
       tag_decl_kind = clang::TTK_Class;
       default_accessibility = eAccessPrivate;
     }
 
     std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> bases;
-    std::vector<int> member_accessibilities;
-    bool is_a_class = false;
     // Parse members and base classes first
     std::vector<DWARFDIE> 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)
       dwarf->ResolveType(die);
 
     if (type_is_objc_object_or_interface) {
       ConstString class_name(clang_type.GetTypeName());
       if (class_name) {
         dwarf->GetObjCMethods(class_name, [&](DWARFDIE method_die) {
           method_die.ResolveType();
           return true;
         });
 
         for (DelayedPropertyList::iterator pi = delayed_properties.begin(),
                                            pe = delayed_properties.end();
              pi != pe; ++pi)
           pi->Finalize();
       }
     }
 
-    // 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
       // 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)
if (die.GetDIENamesAndRanges(name, mangled, func_ranges, decl_file, decl_line,
     }
   }
   return nullptr;
 }
 
 void DWARFASTParserClang::ParseSingleMember(
     const DWARFDIE &die, const DWARFDIE &parent_die,
     const lldb_private::CompilerType &class_clang_type,
-    std::vector<int> &member_accessibilities,
     lldb::AccessType default_accessibility,
     DelayedPropertyList &delayed_properties,
     lldb_private::ClangASTImporter::LayoutInfo &layout_info,
     FieldInfo &last_field_info) {
   ModuleSP module_sp = parent_die.GetDWARF()->GetObjectFile()->GetModule();
   const dw_tag_t tag = die.Tag();
   // Get the parent byte size so we can verify any members will fit
   const uint64_t parent_byte_size =
if (!is_artificial) {
     const uint64_t character_width = 8;
     const uint64_t word_width = 32;
     if (tag == DW_TAG_member) {
       if (member_type) {
         CompilerType member_clang_type = member_type->GetLayoutCompilerType();
 
         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));
 
         if (bit_size > 0) {
           FieldInfo this_field_info;
           this_field_info.bit_offset = field_bit_offset;
           this_field_info.bit_size = bit_size;
if (prop_name != nullptr && member_type) {
         m_ast.SetMetadataAsUserID(ivar_decl, die.GetID());
     }
   }
 }
 
 bool DWARFASTParserClang::ParseChildMembers(
     const DWARFDIE &parent_die, CompilerType &class_clang_type,
     std::vector<std::unique_ptr<clang::CXXBaseSpecifier>> &base_classes,
-    std::vector<int> &member_accessibilities,
     std::vector<DWARFDIE> &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;
 
   FieldInfo last_field_info;
 
   ModuleSP module_sp = parent_die.GetDWARF()->GetObjectFile()->GetModule();
   TypeSystemClang *ast =
       llvm::dyn_cast_or_null<TypeSystemClang>(class_clang_type.GetTypeSystem());
   if (ast == nullptr)
     return false;
 
   for (DWARFDIE die = parent_die.GetFirstChild(); die.IsValid();
        die = die.GetSibling()) {
     dw_tag_t tag = die.Tag();
 
     switch (tag) {
     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:
       // Let the type parsing code handle this one for us.
       member_function_dies.push_back(die);
       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);
       if (num_attributes > 0) {
         DWARFFormValue encoding_form;
         AccessType accessibility = default_accessibility;
         bool is_virtual = false;
         bool is_base_of_class = true;

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h

public:
   static bool RecordHasFields(const clang::RecordDecl *record_decl);
 
   CompilerType CreateObjCClass(llvm::StringRef name,
                                clang::DeclContext *decl_ctx,
                                OptionalClangModuleID owning_module,
                                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
 
   // Namespace Declarations
 
   clang::NamespaceDecl *
   GetUniqueNamespaceDeclaration(const char *name, clang::DeclContext *decl_ctx,
                                 OptionalClangModuleID owning_module,

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

 
 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;
 }
 
-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::TagType>(clang_type);
-    if (tag_type) {
-      clang::TagDecl *tag_decl =
-          llvm::dyn_cast<clang::TagDecl>(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));
 }
 
 /// Aggressively desugar the provided type, skipping past various kinds of
 /// syntactic sugar and other constructs one typically wants to ignore.
 /// The \p mask argument allows one to skip certain kinds of simplifications,