Simplify `GetName`+`AppendTypeName` by `DWARFDIE`

[jankratochvil]

Summary

In D61502#1503247 @clayborg suggested that DWARFUnit *+dw_offset_t can be now replaced by DWARFDIE.

[JDevlieghere]

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

1073 ↗

(On Diff #200549)

How about turning this into an early return?

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

129 ↗

(On Diff #200549)

Since we pass by value we can remove the const?

[jankratochvil]

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

1073 ↗

(On Diff #200549)

This deep nesting is a general coding style in LLDB so I try to keep it. Yes, I would like to (and I use elsewhere) early returns to keep the nesting low. I will submit another patch for it when you say it is OK to clean it up.

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

129 ↗

(On Diff #200549)

There was already const dw_offset_t but OK, I will remove it as it is probably a LLVM rule.

[clayborg]

The whole goal of the DWARFDebugInfoEntry::GetName() and DWARFDebugInfoEntry::AppendTypeName() we might be needing to dump the name of a DIE in DWARF that was not parsed yet. Like if a DIE has a forward reference to a DIE and we are trying to dump a DWARFDebugInfoEntry when things go wrong. If this is not needed then just move these functions into DWARFDIE.

[clayborg]

We really shouldn't have any DWARFDIE references inside DWARFDebugInfoEntry.h. I noticed there are a lot now which is wrong.

[clayborg]

In D62211#1510903, @clayborg wrote:

We really shouldn't have any DWARFDIE references inside DWARFDebugInfoEntry.h. I noticed there are a lot now which is wrong.

I take this back. Many uses are good inside of DWARFDebugInfoEntry as I ok'ed patches when I looked at them. Initially this file was intended to be the level below DWARFDIE, but that has changed, and for good reason: make sure we never use the wrong DWARFUnit with a DWARFDebugInfoEntry. But my initial comment about moving the GetName and AppendTypeName still stand. If we aren't using these for forward references when we don't have all of the DWARFDebugInfoEntry objects parsed already, then they should be moved to DWARFDIE and removed from here.

[labath]

Isn't all of this dead code? These functions seem to be only called from DWARFDebugInfoEntry::Dump, and I can find no callers of the Dump method.

If that's the case, what I'd do is delete all of this stuff, and then if we ever need to dump debug info entries again, implement a llvm-style dump method on the DWARFDIE class. This will move us closer to aligning lldb and llvm versions of these two classes.

In D62211#1510974, @clayborg wrote:

In D62211#1510903, @clayborg wrote:

We really shouldn't have any DWARFDIE references inside DWARFDebugInfoEntry.h. I noticed there are a lot now which is wrong.

I take this back. Many uses are good inside of DWARFDebugInfoEntry as I ok'ed patches when I looked at them. Initially this file was intended to be the level below DWARFDIE, but that has changed, and for good reason: make sure we never use the wrong DWARFUnit with a DWARFDebugInfoEntry. But my initial comment about moving the GetName and AppendTypeName still stand. If we aren't using these for forward references when we don't have all of the DWARFDebugInfoEntry objects parsed already, then they should be moved to DWARFDIE and removed from here.

I think it is still possible (and desirable) to maintain (or restore) the "DWARFDIE >> DWARFDebugInfoEntry" invariant, while still making sure that users don't have to remember which entry goes with which compile unit. The way to achieve that is by making that nobody (outside the DWARFDIE class, and maybe some other low-level stuff) deals with DWARFDebugInfoEntries directly). That's pretty much what was done in the llvm's version of the parser where DWARFDebugInfoEntry is an extremely dumb class with just 5 simple accessor functions, and all of the interesting stuff happens in DWARFDie.

[jankratochvil]

I have moved it to DWARFDIE.cpp. I agree that it is probably all irrelevant after moving to the LLVM DWARF/ part. I have also removed return type as (1) it was wrong in one case and (2) no existing caller used the return type. I also refactored the deep nesting noted by @JDevlieghere.
I can also remove all the dumping code instead upon request, @labath.

[JDevlieghere]

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

1073 ↗

(On Diff #200549)

The deep nesting is definitely a bug and not a feature. We've been removing it where it makes sense.

[JDevlieghere]

^ Feel free to address that in a follow up commit

source/Plugins/SymbolFile/DWARF/DWARFDIE.h

public:
 
   // Accessing information about a DIE
   const char *GetMangledName() const;
 
   const char *GetPubname() const;
 
   const char *GetQualifiedName(std::string &storage) const;
 
+  using DWARFBaseDIE::GetName;
+  void GetName(lldb_private::Stream &s) const;
+
+  void AppendTypeName(lldb_private::Stream &s) const;
+
   lldb_private::Type *ResolveType() const;
 
   // Resolve a type by UID using this DIE's DWARF file
   lldb_private::Type *ResolveTypeUID(const DIERef &die_ref) const;
 
   // Functions for obtaining DIE relations and references
 
   DWARFDIE

source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp

 
 const char *DWARFDIE::GetQualifiedName(std::string &storage) const {
   if (IsValid())
     return m_die->GetQualifiedName(m_cu, storage);
   else
     return nullptr;
 }
 
+// GetName
+//
+// Get value of the DW_AT_name attribute and place that value into the supplied
+// stream object. If the DIE is a NULL object "NULL" is placed into the stream,
+// and if no DW_AT_name attribute exists for the DIE then nothing is printed.
+void DWARFDIE::GetName(Stream &s) const {
+  if (!IsValid())
+    return;
+  if (GetDIE()->IsNULL()) {
+    s.PutCString("NULL");
+    return;
+  }
+  const char *name = GetDIE()->GetAttributeValueAsString(GetCU(), DW_AT_name, nullptr, true);
+  if (!name)
+    return;
+  s.PutCString(name);
+}
+
+// AppendTypeName
+//
+// Follows the type name definition down through all needed tags to end up with
+// a fully qualified type name and dump the results to the supplied stream.
+// This is used to show the name of types given a type identifier.
+void DWARFDIE::AppendTypeName(Stream &s) const {
+  if (!IsValid())
+    return;
+  if (GetDIE()->IsNULL()) {
+    s.PutCString("NULL");
+    return;
+  }
+  if (const char *name = GetPubname()) {
+    s.PutCString(name);
+    return;
+  }
+  switch (Tag()) {
+  case DW_TAG_array_type:
+    break; // print out a "[]" after printing the full type of the element
+           // below
+  case DW_TAG_base_type:
+    s.PutCString("base ");
+    break;
+  case DW_TAG_class_type:
+    s.PutCString("class ");
+    break;
+  case DW_TAG_const_type:
+    s.PutCString("const ");
+    break;
+  case DW_TAG_enumeration_type:
+    s.PutCString("enum ");
+    break;
+  case DW_TAG_file_type:
+    s.PutCString("file ");
+    break;
+  case DW_TAG_interface_type:
+    s.PutCString("interface ");
+    break;
+  case DW_TAG_packed_type:
+    s.PutCString("packed ");
+    break;
+  case DW_TAG_pointer_type:
+    break; // print out a '*' after printing the full type below
+  case DW_TAG_ptr_to_member_type:
+    break; // print out a '*' after printing the full type below
+  case DW_TAG_reference_type:
+    break; // print out a '&' after printing the full type below
+  case DW_TAG_restrict_type:
+    s.PutCString("restrict ");
+    break;
+  case DW_TAG_set_type:
+    s.PutCString("set ");
+    break;
+  case DW_TAG_shared_type:
+    s.PutCString("shared ");
+    break;
+  case DW_TAG_string_type:
+    s.PutCString("string ");
+    break;
+  case DW_TAG_structure_type:
+    s.PutCString("struct ");
+    break;
+  case DW_TAG_subrange_type:
+    s.PutCString("subrange ");
+    break;
+  case DW_TAG_subroutine_type:
+    s.PutCString("function ");
+    break;
+  case DW_TAG_thrown_type:
+    s.PutCString("thrown ");
+    break;
+  case DW_TAG_union_type:
+    s.PutCString("union ");
+    break;
+  case DW_TAG_unspecified_type:
+    s.PutCString("unspecified ");
+    break;
+  case DW_TAG_volatile_type:
+    s.PutCString("volatile ");
+    break;
+  default:
+    return;
+  }
+
+  // Follow the DW_AT_type if possible
+  if (DWARFDIE next_die = GetAttributeValueAsReferenceDIE(DW_AT_type))
+    next_die.AppendTypeName(s);
+
+  switch (Tag()) {
+  case DW_TAG_array_type:
+    s.PutCString("[]");
+    break;
+  case DW_TAG_pointer_type:
+    s.PutChar('*');
+    break;
+  case DW_TAG_ptr_to_member_type:
+    s.PutChar('*');
+    break;
+  case DW_TAG_reference_type:
+    s.PutChar('&');
+    break;
+  default:
+    break;
+  }
+}
+
 lldb_private::Type *DWARFDIE::ResolveType() const {
   if (IsValid())
     return GetDWARF()->ResolveType(*this, true);
   else
     return nullptr;
 }
 
 lldb_private::Type *DWARFDIE::ResolveTypeUID(const DIERef &die_ref) const {

source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h

public:
 
   const char *GetName(const DWARFUnit *cu) const;
 
   const char *GetMangledName(const DWARFUnit *cu,
                              bool substitute_name_allowed = true) const;
 
   const char *GetPubname(const DWARFUnit *cu) const;
 
-  static bool GetName(const DWARFUnit *cu, const dw_offset_t die_offset,
-                      lldb_private::Stream &s);
-
-  static bool AppendTypeName(const DWARFUnit *cu, const dw_offset_t die_offset,
-                             lldb_private::Stream &s);
-
   const char *GetQualifiedName(DWARFUnit *cu, std::string &storage) const;
 
   const char *GetQualifiedName(DWARFUnit *cu, const DWARFAttributes &attributes,
                                std::string &storage) const;
 
   static bool OffsetLessThan(const DWARFDebugInfoEntry &a,
                              const DWARFDebugInfoEntry &b);
 

source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp

case DW_AT_data_member_location: {
     }
   } break;
 
   case DW_AT_abstract_origin:
   case DW_AT_specification: {
     DWARFDIE abstract_die = form_value.Reference();
     form_value.Dump(s);
     //  *ostrm_ptr << HEX32 << abstract_die.GetOffset() << " ( ";
-    GetName(abstract_die.GetCU(), abstract_die.GetOffset(), s);
+    abstract_die.GetName(s);
   } break;
 
   case DW_AT_type: {
     DWARFDIE type_die = form_value.Reference();
     s.PutCString(" ( ");
-    AppendTypeName(type_die.GetCU(), type_die.GetOffset(), s);
+    type_die.AppendTypeName(s);
     s.PutCString(" )");
   } break;
 
   case DW_AT_ranges: {
     lldb::offset_t ranges_offset =
         GetRangesOffset(dwarf2Data->DebugRanges(), form_value);
     dw_addr_t base_addr = cu ? cu->GetBaseAddress() : 0;
     DWARFDebugRanges::Dump(s, dwarf2Data->get_debug_ranges_data(),
const char *DWARFDebugInfoEntry::GetPubname(const DWARFUnit *cu) const {
   name = GetAttributeValueAsString(cu, DW_AT_linkage_name, nullptr, true);
   if (name)
     return name;
 
   name = GetAttributeValueAsString(cu, DW_AT_name, nullptr, true);
   return name;
 }
 
-// GetName
-//
-// Get value of the DW_AT_name attribute for a debug information entry that
-// exists at offset "die_offset" and place that value into the supplied stream
-// object. If the DIE is a NULL object "NULL" is placed into the stream, and if
-// no DW_AT_name attribute exists for the DIE then nothing is printed.
-bool DWARFDebugInfoEntry::GetName(const DWARFUnit *cu,
-                                  const dw_offset_t die_offset, Stream &s) {
-  if (cu == NULL) {
-    s.PutCString("NULL");
-    return false;
-  }
-
-  DWARFDebugInfoEntry die;
-  lldb::offset_t offset = die_offset;
-  if (die.Extract(cu, &offset)) {
-    if (die.IsNULL()) {
-      s.PutCString("NULL");
-      return true;
-    } else {
-      const char *name =
-          die.GetAttributeValueAsString(cu, DW_AT_name, nullptr, true);
-      if (name) {
-        s.PutCString(name);
-        return true;
-      }
-    }
-  }
-  return false;
-}
-
-// AppendTypeName
-//
-// Follows the type name definition down through all needed tags to end up with
-// a fully qualified type name and dump the results to the supplied stream.
-// This is used to show the name of types given a type identifier.
-bool DWARFDebugInfoEntry::AppendTypeName(const DWARFUnit *cu,
-                                         const dw_offset_t die_offset,
-                                         Stream &s) {
-  if (cu == NULL) {
-    s.PutCString("NULL");
-    return false;
-  }
-
-  DWARFDebugInfoEntry die;
-  lldb::offset_t offset = die_offset;
-  if (die.Extract(cu, &offset)) {
-    if (die.IsNULL()) {
-      s.PutCString("NULL");
-      return true;
-    } else {
-      const char *name = die.GetPubname(cu);
-      if (name)
-        s.PutCString(name);
-      else {
-        bool result = true;
-        const DWARFAbbreviationDeclaration *abbrevDecl =
-            die.GetAbbreviationDeclarationPtr(cu, offset);
-
-        if (abbrevDecl == NULL)
-          return false;
-
-        switch (abbrevDecl->Tag()) {
-        case DW_TAG_array_type:
-          break; // print out a "[]" after printing the full type of the element
-                 // below
-        case DW_TAG_base_type:
-          s.PutCString("base ");
-          break;
-        case DW_TAG_class_type:
-          s.PutCString("class ");
-          break;
-        case DW_TAG_const_type:
-          s.PutCString("const ");
-          break;
-        case DW_TAG_enumeration_type:
-          s.PutCString("enum ");
-          break;
-        case DW_TAG_file_type:
-          s.PutCString("file ");
-          break;
-        case DW_TAG_interface_type:
-          s.PutCString("interface ");
-          break;
-        case DW_TAG_packed_type:
-          s.PutCString("packed ");
-          break;
-        case DW_TAG_pointer_type:
-          break; // print out a '*' after printing the full type below
-        case DW_TAG_ptr_to_member_type:
-          break; // print out a '*' after printing the full type below
-        case DW_TAG_reference_type:
-          break; // print out a '&' after printing the full type below
-        case DW_TAG_restrict_type:
-          s.PutCString("restrict ");
-          break;
-        case DW_TAG_set_type:
-          s.PutCString("set ");
-          break;
-        case DW_TAG_shared_type:
-          s.PutCString("shared ");
-          break;
-        case DW_TAG_string_type:
-          s.PutCString("string ");
-          break;
-        case DW_TAG_structure_type:
-          s.PutCString("struct ");
-          break;
-        case DW_TAG_subrange_type:
-          s.PutCString("subrange ");
-          break;
-        case DW_TAG_subroutine_type:
-          s.PutCString("function ");
-          break;
-        case DW_TAG_thrown_type:
-          s.PutCString("thrown ");
-          break;
-        case DW_TAG_union_type:
-          s.PutCString("union ");
-          break;
-        case DW_TAG_unspecified_type:
-          s.PutCString("unspecified ");
-          break;
-        case DW_TAG_volatile_type:
-          s.PutCString("volatile ");
-          break;
-        default:
-          return false;
-        }
-
-        // Follow the DW_AT_type if possible
-        DWARFFormValue form_value;
-        if (die.GetAttributeValue(cu, DW_AT_type, form_value)) {
-          DWARFDIE next_die = form_value.Reference();
-          result = AppendTypeName(next_die.GetCU(), next_die.GetOffset(), s);
-        }
-
-        switch (abbrevDecl->Tag()) {
-        case DW_TAG_array_type:
-          s.PutCString("[]");
-          break;
-        case DW_TAG_pointer_type:
-          s.PutChar('*');
-          break;
-        case DW_TAG_ptr_to_member_type:
-          s.PutChar('*');
-          break;
-        case DW_TAG_reference_type:
-          s.PutChar('&');
-          break;
-        default:
-          break;
-        }
-        return result;
-      }
-    }
-  }
-  return false;
-}
-
 // BuildAddressRangeTable
 void DWARFDebugInfoEntry::BuildAddressRangeTable(
     const DWARFUnit *cu, DWARFDebugAranges *debug_aranges) const {
   if (m_tag) {
     if (m_tag == DW_TAG_subprogram) {
       dw_addr_t lo_pc = LLDB_INVALID_ADDRESS;
       dw_addr_t hi_pc = LLDB_INVALID_ADDRESS;
       if (GetAttributeAddressRange(cu, lo_pc, hi_pc, LLDB_INVALID_ADDRESS)) {