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

How about turning this into an early return?

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

129

Since we pass by value we can remove the const?

[jankratochvil]

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

1073

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

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

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

lldb/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 GetName(DWARFDIE die, lldb_private::Stream &s);
 
-  static bool AppendTypeName(const DWARFUnit *cu, const dw_offset_t die_offset,
-                             lldb_private::Stream &s);
+  static bool AppendTypeName(DWARFDIE die, 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);

lldb/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);
+    GetName(abstract_die, s);
   } break;
 
   case DW_AT_type: {
     DWARFDIE type_die = form_value.Reference();
     s.PutCString(" ( ");
-    AppendTypeName(type_die.GetCU(), type_die.GetOffset(), s);
+    AppendTypeName(type_die, 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(),
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()) {
+// Get value of the DW_AT_name attribute for a debug information entry "die"
+// 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(DWARFDIE die, Stream &s) {
+  if (die) {
+    if (die.GetDIE()->IsNULL()) {
       s.PutCString("NULL");
       return true;
     } else {
-      const char *name =
-          die.GetAttributeValueAsString(cu, DW_AT_name, nullptr, true);
+      const char *name = die.GetDIE()->GetAttributeValueAsString(
+          die.GetCU(), 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()) {
+bool DWARFDebugInfoEntry::AppendTypeName(DWARFDIE die, Stream &s) {
+  if (die) {
+    if (die.GetDIE()->IsNULL()) {
       s.PutCString("NULL");
       return true;
     } else {
-      const char *name = die.GetPubname(cu);
+      const char *name = die.GetPubname();
       if (name)
         s.PutCString(name);
       else {
         bool result = true;
-        const DWARFAbbreviationDeclaration *abbrevDecl =
-            die.GetAbbreviationDeclarationPtr(cu, offset);
-
-        if (abbrevDecl == NULL)
-          return false;
-
-        switch (abbrevDecl->Tag()) {
+        switch (die.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 ");
if (die.GetDIE()->IsNULL()) {
         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);
-        }
+        if (DWARFDIE next_die = die.GetAttributeValueAsReferenceDIE(DW_AT_type))
+          result = AppendTypeName(next_die, s);
 
-        switch (abbrevDecl->Tag()) {
+        switch (die.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('*');