[lldb][NFCI] Refactor out attribute parsing from DWARFASTParserClang::ParseSingleMember

[teemperor]

Summary

D68422 introduced ParsedDWARFTypeAttributes which encapsulated attribute parsing and
storage into its own small struct. This patch is doing the same for the member type attribute
parsing. One utility class is parsing normal member attributes and the other is parsing the
dedicated Objective-C property attributes.

Right now the patch just makes the ParseSingleMember function a bit shorter, but the bigger
benefit is that we can now split up the function into Objective-C property parsing and parsing
of normal members (struct/class members and Objective-C ivars). The only shared code between
those two parsing logic is the normal member attribute parsing.

[labath]

seems reasonable to me

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

if (func_range.GetBaseAddress().IsValid()) {
           return func_sp.get();
         }
       }
     }
   }
   return nullptr;
 }
 
-void DWARFASTParserClang::ParseSingleMember(
-    const DWARFDIE &die, const DWARFDIE &parent_die,
-    const lldb_private::CompilerType &class_clang_type,
-    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 =
-      parent_die.GetAttributeValueAsUnsigned(DW_AT_byte_size, UINT64_MAX);
-  const uint64_t parent_bit_size =
-      parent_byte_size == UINT64_MAX ? UINT64_MAX : parent_byte_size * 8;
-
-  DWARFAttributes attributes;
-  const size_t num_attributes = die.GetAttributes(attributes);
-  if (num_attributes == 0)
-    return;
-
+namespace {
+/// Parsed form of all attributes that are relevant for parsing type members.
+struct MemberAttributes {
+  explicit MemberAttributes(const DWARFDIE &die, const DWARFDIE &parent_die,
+                            ModuleSP module_sp);
   const char *name = nullptr;
+  /// Indicates how many bits into the word (according to the host endianness)
+  /// the low-order bit of the field starts. Can be negative.
+  int64_t bit_offset = 0;
+  /// Indicates the size of the field in bits.
+  size_t bit_size = 0;
+  uint64_t data_bit_offset = UINT64_MAX;
+  AccessType accessibility = eAccessNone;
+  llvm::Optional<uint64_t> byte_size;
+  DWARFFormValue encoding_form;
+  /// Indicates the byte offset of the word from the base address of the
+  /// structure.
+  uint32_t member_byte_offset;
+  bool is_artificial = false;
+  /// On DW_TAG_members, this means the member is static.
+  bool is_external = false;
+};
+
+/// Parsed form of all attributes that are relevant for parsing Objective-C
+/// properties.
+struct PropertyAttributes {
+  explicit PropertyAttributes(const DWARFDIE &die);
   const char *prop_name = nullptr;
   const char *prop_getter_name = nullptr;
   const char *prop_setter_name = nullptr;
+  /// \see clang::ObjCPropertyAttribute
   uint32_t prop_attributes = 0;
+};
+} // namespace
 
-  bool is_artificial = false;
-  DWARFFormValue encoding_form;
-  AccessType accessibility = eAccessNone;
-  uint32_t member_byte_offset =
-      (parent_die.Tag() == DW_TAG_union_type) ? 0 : UINT32_MAX;
-  llvm::Optional<uint64_t> byte_size;
-  int64_t bit_offset = 0;
-  uint64_t data_bit_offset = UINT64_MAX;
-  size_t bit_size = 0;
-  bool is_external =
-      false; // On DW_TAG_members, this means the member is static
-  uint32_t i;
-  for (i = 0; i < num_attributes && !is_artificial; ++i) {
+MemberAttributes::MemberAttributes(const DWARFDIE &die,
+                                   const DWARFDIE &parent_die,
+                                   ModuleSP module_sp) {
+  member_byte_offset = (parent_die.Tag() == DW_TAG_union_type) ? 0 : UINT32_MAX;
+
+  DWARFAttributes attributes;
+  const size_t num_attributes = die.GetAttributes(attributes);
+  for (std::size_t i = 0; i < num_attributes; ++i) {
     const dw_attr_t attr = attributes.AttributeAtIndex(i);
     DWARFFormValue form_value;
     if (attributes.ExtractFormValueAtIndex(i, form_value)) {
-      // DW_AT_data_member_location indicates the byte offset of the
-      // word from the base address of the structure.
-      //
-      // DW_AT_bit_offset indicates how many bits into the word
-      // (according to the host endianness) the low-order bit of the
-      // field starts.  AT_bit_offset can be negative.
-      //
-      // DW_AT_bit_size indicates the size of the field in bits.
       switch (attr) {
       case DW_AT_name:
         name = form_value.AsCString();
         break;
       case DW_AT_type:
         encoding_form = form_value;
         break;
       case DW_AT_bit_offset:
if (attributes.ExtractFormValueAtIndex(i, form_value)) {
         break;
 
       case DW_AT_accessibility:
         accessibility = DW_ACCESS_to_AccessType(form_value.Unsigned());
         break;
       case DW_AT_artificial:
         is_artificial = form_value.Boolean();
         break;
+      case DW_AT_external:
+        is_external = form_value.Boolean();
+        break;
+      default:
+        break;
+      }
+    }
+  }
+
+  // Clang has a DWARF generation bug where sometimes it represents
+  // fields that are references with bad byte size and bit size/offset
+  // information such as:
+  //
+  //  DW_AT_byte_size( 0x00 )
+  //  DW_AT_bit_size( 0x40 )
+  //  DW_AT_bit_offset( 0xffffffffffffffc0 )
+  //
+  // So check the bit offset to make sure it is sane, and if the values
+  // are not sane, remove them. If we don't do this then we will end up
+  // with a crash if we try to use this type in an expression when clang
+  // becomes unhappy with its recycled debug info.
+  if (byte_size.getValueOr(0) == 0 && bit_offset < 0) {
+    bit_size = 0;
+    bit_offset = 0;
+  }
+}
+
+PropertyAttributes::PropertyAttributes(const DWARFDIE &die) {
+
+  DWARFAttributes attributes;
+  const size_t num_attributes = die.GetAttributes(attributes);
+  for (size_t i = 0; i < num_attributes; ++i) {
+    const dw_attr_t attr = attributes.AttributeAtIndex(i);
+    DWARFFormValue form_value;
+    if (attributes.ExtractFormValueAtIndex(i, form_value)) {
+      switch (attr) {
       case DW_AT_APPLE_property_name:
         prop_name = form_value.AsCString();
         break;
       case DW_AT_APPLE_property_getter:
         prop_getter_name = form_value.AsCString();
         break;
       case DW_AT_APPLE_property_setter:
         prop_setter_name = form_value.AsCString();
         break;
       case DW_AT_APPLE_property_attribute:
         prop_attributes = form_value.Unsigned();
         break;
-      case DW_AT_external:
-        is_external = form_value.Boolean();
-        break;
-
       default:
-      case DW_AT_declaration:
-      case DW_AT_description:
-      case DW_AT_mutable:
-      case DW_AT_visibility:
-      case DW_AT_sibling:
         break;
       }
     }
   }
 
-  if (prop_name) {
-    ConstString fixed_setter;
+  if (!prop_name)
+    return;
 
-    // Check if the property getter/setter were provided as full names.
-    // We want basenames, so we extract them.
-
-    if (prop_getter_name && prop_getter_name[0] == '-') {
-      ObjCLanguage::MethodName prop_getter_method(prop_getter_name, true);
-      prop_getter_name = prop_getter_method.GetSelector().GetCString();
-    }
 
-    if (prop_setter_name && prop_setter_name[0] == '-') {
-      ObjCLanguage::MethodName prop_setter_method(prop_setter_name, true);
-      prop_setter_name = prop_setter_method.GetSelector().GetCString();
-    }
 
-    // If the names haven't been provided, they need to be filled in.
-
-    if (!prop_getter_name) {
-      prop_getter_name = prop_name;
-    }
-    if (!prop_setter_name && prop_name[0] &&
-        !(prop_attributes & DW_APPLE_PROPERTY_readonly)) {
-      StreamString ss;
+  if (!prop_getter_name)
 
-      ss.Printf("set%c%s:", toupper(prop_name[0]), &prop_name[1]);
 
-      fixed_setter.SetString(ss.GetString());
-      prop_setter_name = fixed_setter.GetCString();
-    }
-  }
 
-  // Clang has a DWARF generation bug where sometimes it represents
-  // fields that are references with bad byte size and bit size/offset
-  // information such as:
-  //
-  //  DW_AT_byte_size( 0x00 )
-  //  DW_AT_bit_size( 0x40 )
-  //  DW_AT_bit_offset( 0xffffffffffffffc0 )
-  //
-  // So check the bit offset to make sure it is sane, and if the values
-  // are not sane, remove them. If we don't do this then we will end up
-  // with a crash if we try to use this type in an expression when clang
-  // becomes unhappy with its recycled debug info.
+void DWARFASTParserClang::ParseSingleMember(
+    const DWARFDIE &die, const DWARFDIE &parent_die,
+    const lldb_private::CompilerType &class_clang_type,
+    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 =
+      parent_die.GetAttributeValueAsUnsigned(DW_AT_byte_size, UINT64_MAX);
+  const uint64_t parent_bit_size =
+      parent_byte_size == UINT64_MAX ? UINT64_MAX : parent_byte_size * 8;
 
-  if (byte_size.getValueOr(0) == 0 && bit_offset < 0) {
-    bit_size = 0;
-    bit_offset = 0;
-  }
+  // FIXME: Remove the workarounds below and make this const.
+  MemberAttributes attrs(die, parent_die, module_sp);
+  // Skip artificial members such as vtable pointers.
+  // FIXME: This check should verify that this is indeed an artificial member
+  // we are supposed to ignore.
+  if (attrs.is_artificial)
+    return;
+
+  const PropertyAttributes propAttrs(die);
 
   const bool class_is_objc_object_or_interface =
       TypeSystemClang::IsObjCObjectOrInterfaceType(class_clang_type);
 
   // FIXME: Make Clang ignore Objective-C accessibility for expressions
   if (class_is_objc_object_or_interface)
-    accessibility = eAccessNone;
+    attrs.accessibility = eAccessNone;
 
   // Handle static members
-  if (is_external && member_byte_offset == UINT32_MAX) {
-    Type *var_type = die.ResolveTypeUID(encoding_form.Reference());
+  if (attrs.is_external && attrs.member_byte_offset == UINT32_MAX) {
+    Type *var_type = die.ResolveTypeUID(attrs.encoding_form.Reference());
 
     if (var_type) {
-      if (accessibility == eAccessNone)
-        accessibility = eAccessPublic;
+      if (attrs.accessibility == eAccessNone)
+        attrs.accessibility = eAccessPublic;
       TypeSystemClang::AddVariableToRecordType(
-          class_clang_type, name, var_type->GetForwardCompilerType(),
-          accessibility);
+          class_clang_type, attrs.name, var_type->GetForwardCompilerType(),
+          attrs.accessibility);
     }
     return;
   }
 
-  if (is_artificial)
-    return;
-
-  Type *member_type = die.ResolveTypeUID(encoding_form.Reference());
+  Type *member_type = die.ResolveTypeUID(attrs.encoding_form.Reference());
   if (!member_type) {
     // FIXME: This should not silently fail.
-    if (prop_name)
+    if (propAttrs.prop_name)
       return;
-    if (name)
+    if (attrs.name)
       module_sp->ReportError(
           "0x%8.8" PRIx64 ": DW_TAG_member '%s' refers to type 0x%8.8x"
           " which was unable to be parsed",
-          die.GetID(), name, encoding_form.Reference().GetOffset());
+          die.GetID(), attrs.name, attrs.encoding_form.Reference().GetOffset());
     else
       module_sp->ReportError(
           "0x%8.8" PRIx64 ": DW_TAG_member refers to type 0x%8.8x"
           " which was unable to be parsed",
-          die.GetID(), encoding_form.Reference().GetOffset());
+          die.GetID(), attrs.encoding_form.Reference().GetOffset());
     return;
   }
 
   clang::FieldDecl *field_decl = nullptr;
   const uint64_t character_width = 8;
   const uint64_t word_width = 32;
   if (tag == DW_TAG_member) {
     CompilerType member_clang_type = member_type->GetLayoutCompilerType();
 
-    if (accessibility == eAccessNone)
-      accessibility = default_accessibility;
+    if (attrs.accessibility == eAccessNone)
+      attrs.accessibility = default_accessibility;
 
-    uint64_t field_bit_offset =
-        (member_byte_offset == UINT32_MAX ? 0 : (member_byte_offset * 8));
+    uint64_t field_bit_offset = (attrs.member_byte_offset == UINT32_MAX
+                                     ? 0
+                                     : (attrs.member_byte_offset * 8));
 
-    if (bit_size > 0) {
+    if (attrs.bit_size > 0) {
       FieldInfo this_field_info;
       this_field_info.bit_offset = field_bit_offset;
-      this_field_info.bit_size = bit_size;
+      this_field_info.bit_size = attrs.bit_size;
 
-      if (data_bit_offset != UINT64_MAX) {
-        this_field_info.bit_offset = data_bit_offset;
+      if (attrs.data_bit_offset != UINT64_MAX) {
+        this_field_info.bit_offset = attrs.data_bit_offset;
       } else {
-        if (!byte_size)
-          byte_size = member_type->GetByteSize(nullptr);
+        if (!attrs.byte_size)
+          attrs.byte_size = member_type->GetByteSize(nullptr);
 
         ObjectFile *objfile = die.GetDWARF()->GetObjectFile();
         if (objfile->GetByteOrder() == eByteOrderLittle) {
-          this_field_info.bit_offset += byte_size.getValueOr(0) * 8;
-          this_field_info.bit_offset -= (bit_offset + bit_size);
+          this_field_info.bit_offset += attrs.byte_size.getValueOr(0) * 8;
+          this_field_info.bit_offset -= (attrs.bit_offset + attrs.bit_size);
         } else {
-          this_field_info.bit_offset += bit_offset;
+          this_field_info.bit_offset += attrs.bit_offset;
         }
       }
 
       // The ObjC runtime knows the byte offset but we still need to provide
       // the bit-offset in the layout. It just means something different then
       // what it does in C and C++. So we skip this check for ObjC types.
       //
       // We also skip this for fields of a union since they will all have a
       // zero offset.
       if (!TypeSystemClang::IsObjCObjectOrInterfaceType(class_clang_type) &&
           !(parent_die.Tag() == DW_TAG_union_type &&
             this_field_info.bit_offset == 0) &&
           ((this_field_info.bit_offset >= parent_bit_size) ||
            (last_field_info.IsBitfield() &&
             !last_field_info.NextBitfieldOffsetIsValid(
                 this_field_info.bit_offset)))) {
         ObjectFile *objfile = die.GetDWARF()->GetObjectFile();
         objfile->GetModule()->ReportWarning(
             "0x%8.8" PRIx64 ": %s bitfield named \"%s\" has invalid "
             "bit offset (0x%8.8" PRIx64
             ") member will be ignored. Please file a bug against the "
             "compiler and include the preprocessed output for %s\n",
-            die.GetID(), DW_TAG_value_to_name(tag), name,
+            die.GetID(), DW_TAG_value_to_name(tag), attrs.name,
             this_field_info.bit_offset, GetUnitName(parent_die).c_str());
         return;
       }
 
       // Update the field bit offset we will report for layout
       field_bit_offset = this_field_info.bit_offset;
 
       // Objective-C has invalid DW_AT_bit_offset values in older
if (attrs.bit_size > 0) {
         }
 
         if (unnamed_field_info) {
           clang::FieldDecl *unnamed_bitfield_decl =
               TypeSystemClang::AddFieldToRecordType(
                   class_clang_type, llvm::StringRef(),
                   m_ast.GetBuiltinTypeForEncodingAndBitSize(eEncodingSint,
                                                             word_width),
-                  accessibility, unnamed_field_info->bit_size);
+                  attrs.accessibility, unnamed_field_info->bit_size);
 
           layout_info.field_offsets.insert(std::make_pair(
               unnamed_bitfield_decl, unnamed_field_info->bit_offset));
         }
       }
 
       last_field_info = this_field_info;
       last_field_info.SetIsBitfield(true);
if (!member_clang_type.IsCompleteType())
 
       if (member_clang_type.IsArrayType(&member_array_element_type,
                                         &member_array_size,
                                         &member_array_is_incomplete) &&
           !member_array_is_incomplete) {
         uint64_t parent_byte_size =
             parent_die.GetAttributeValueAsUnsigned(DW_AT_byte_size, UINT64_MAX);
 
-        if (member_byte_offset >= parent_byte_size) {
+        if (attrs.member_byte_offset >= parent_byte_size) {
           if (member_array_size != 1 &&
               (member_array_size != 0 ||
-               member_byte_offset > parent_byte_size)) {
+               attrs.member_byte_offset > parent_byte_size)) {
             module_sp->ReportError(
                 "0x%8.8" PRIx64 ": DW_TAG_member '%s' refers to type 0x%8.8x"
                 " which extends beyond the bounds of 0x%8.8" PRIx64,
-                die.GetID(), name, encoding_form.Reference().GetOffset(),
+                die.GetID(), attrs.name,
+                attrs.encoding_form.Reference().GetOffset(),
                 parent_die.GetID());
           }
 
           member_clang_type =
               m_ast.CreateArrayType(member_array_element_type, 0, false);
         }
       }
     }
 
     RequireCompleteType(member_clang_type);
 
     field_decl = TypeSystemClang::AddFieldToRecordType(
-        class_clang_type, name, member_clang_type, accessibility, bit_size);
+        class_clang_type, attrs.name, member_clang_type, attrs.accessibility,
+        attrs.bit_size);
 
     m_ast.SetMetadataAsUserID(field_decl, die.GetID());
 
     layout_info.field_offsets.insert(
         std::make_pair(field_decl, field_bit_offset));
   }
 
-  if (prop_name != nullptr) {
+  if (propAttrs.prop_name != nullptr) {
     clang::ObjCIvarDecl *ivar_decl = nullptr;
 
     if (field_decl) {
       ivar_decl = clang::dyn_cast<clang::ObjCIvarDecl>(field_decl);
       assert(ivar_decl != nullptr);
     }
 
     ClangASTMetadata metadata;
     metadata.SetUserID(die.GetID());
     delayed_properties.push_back(DelayedAddObjCClassProperty(
-        class_clang_type, prop_name, member_type->GetLayoutCompilerType(),
-        ivar_decl, prop_setter_name, prop_getter_name, prop_attributes,
-        &metadata));
+        class_clang_type, propAttrs.prop_name,
+        member_type->GetLayoutCompilerType(), ivar_decl,
+        propAttrs.prop_setter_name, propAttrs.prop_getter_name,
+        propAttrs.prop_attributes, &metadata));
 
     if (ivar_decl)
       m_ast.SetMetadataAsUserID(ivar_decl, die.GetID());
   }
 }
 
 bool DWARFASTParserClang::ParseChildMembers(
     const DWARFDIE &parent_die, CompilerType &class_clang_type,