Code cleanup: Remove DWARFDebugInfoEntry::m_empty_children

[jankratochvil]

Summary

As @aprantl suggested I have merged DWARFDebugInfoEntry's m_empty_children into m_has_children.
m_empty_children was implemented by rL144983. I do not see why @clayborg made it a separate flag, from some point of view it is sure cleaner but technically I do not see its purpose.
I have checked all calls of HasChildren() that it should not matter to them. The code even wants to know if there are any children - it does not matter how the children presence is coded in the binary.
The only problematic may be operator== (and !=) although that one is currently only used by my lldbassert() in my bugfixed D53255.
This patch sure has no regressions on Fedora 28 x86_64.
I do not need/request this patch, just that @aprantl asked about it.

[clayborg]

m_has_children was used to represent what was in the DWARF in the byte that follows the DW_TAG. m_empty_children was used for DIEs that said they had children but actually only contain a single NULL tag. It is fine to not differentiate between the two.

source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h

281–282

We might want to move m_has_children where m_empty_children used to be and give a bit back to m_abbr_idx by increasing DIE_ABBR_IDX_BITSIZE by 1. We have a few bits to spare in m_sibling_idx since it is the number of dies to advance by to get to the sibling. Since DIEs at the very least are a few bytes, having a 2GB advance instead of 4GB is plenty.

282–284

This comment is wrong. The DWARF encodes if a DIE has children in the .debug_info, but we may override this value if the DIE only contains an NULL terminating DIE.

It should read something like:

// If it is zero, then the DIE doesn't have children, or the 
// DWARF claimed it had children but the DIE only contained 
// a single NULL terminating child.

[jankratochvil]

source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h

282–284

I cannot say it was wrong but I agree your rewording is much better.

[clayborg]

See inlined comment.

source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h

281–282

Might be worth changing these to be uint16_t, removing DIE_ABBR_IDX_BITSIZE and changing any code that was using that to make sure that the value is <= UINT16_MAX:

uint16_t m_abbr_idx;
uint16_t m_tag;

[jankratochvil]

Done the conversion of bitfield to uint16_t.

[jankratochvil]

Also changed assert()->lldbassert() for m_abbr_idx 16-bit overflow as that could be a tough bug to catch if it ever happens.

[clayborg]

Looks good. Nice cleanup.

[jankratochvil]

In D53321#1266912, @clayborg wrote:

Looks good. Nice cleanup.

Thanks for the cleanups and fixes.

source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h

 
 typedef std::multimap<uint32_t, const DWARFDebugInfoEntry *> UInt32ToDIEMMap;
 typedef UInt32ToDIEMMap::iterator UInt32ToDIEMMapIter;
 typedef UInt32ToDIEMMap::const_iterator UInt32ToDIEMMapConstIter;
 
 class DWARFDeclContext;
 
 #define DIE_SIBLING_IDX_BITSIZE 31
-#define DIE_ABBR_IDX_BITSIZE 15
 
 class DWARFDebugInfoEntry {
 public:
   typedef std::vector<DWARFDebugInfoEntry> collection;
   typedef collection::iterator iterator;
   typedef collection::const_iterator const_iterator;
 
   typedef std::vector<dw_offset_t> offset_collection;
   typedef offset_collection::iterator offset_collection_iterator;
   typedef offset_collection::const_iterator offset_collection_const_iterator;
 
   DWARFDebugInfoEntry()
       : m_offset(DW_INVALID_OFFSET), m_parent_idx(0), m_sibling_idx(0),
-        m_empty_children(false), m_abbr_idx(0), m_has_children(false),
-        m_tag(0) {}
+        m_has_children(false), m_abbr_idx(0), m_tag(0) {}
 
   explicit operator bool() const { return m_offset != DW_INVALID_OFFSET; }
   bool operator==(const DWARFDebugInfoEntry &rhs) const;
   bool operator!=(const DWARFDebugInfoEntry &rhs) const;
 
   void BuildAddressRangeTable(SymbolFileDWARF *dwarf2Data,
                               const DWARFUnit *cu,
                               DWARFDebugAranges *debug_aranges) const;
public:
   }
   const DWARFDebugInfoEntry *GetSibling() const {
     return m_sibling_idx > 0 ? this + m_sibling_idx : NULL;
   }
   // We know we are kept in a vector of contiguous entries, so we know
   // we don't need to store our child pointer, if we have a child it will
   // be the next entry in the list...
   DWARFDebugInfoEntry *GetFirstChild() {
-    return (HasChildren() && !m_empty_children) ? this + 1 : NULL;
+    return HasChildren() ? this + 1 : NULL;
   }
   const DWARFDebugInfoEntry *GetFirstChild() const {
-    return (HasChildren() && !m_empty_children) ? this + 1 : NULL;
+    return HasChildren() ? this + 1 : NULL;
   }
 
   void GetDeclContextDIEs(DWARFUnit *cu,
                           DWARFDIECollection &decl_context_dies) const;
 
   void GetDWARFDeclContext(SymbolFileDWARF *dwarf2Data, DWARFUnit *cu,
                            DWARFDeclContext &dwarf_decl_ctx) const;
 
void SetSibling(DWARFDebugInfoEntry *sibling) {
     } else
       m_sibling_idx = 0;
   }
 
   void SetSiblingIndex(uint32_t idx) { m_sibling_idx = idx; }
 
   void SetParentIndex(uint32_t idx) { m_parent_idx = idx; }
 
-  bool GetEmptyChildren() const { return m_empty_children; }
-
-  void SetEmptyChildren(bool b) { m_empty_children = b; }
-
   static void
   DumpDIECollection(lldb_private::Stream &strm,
                     DWARFDebugInfoEntry::collection &die_collection);
 
 protected:
   dw_offset_t
       m_offset; // Offset within the .debug_info of the start of this entry
   uint32_t m_parent_idx; // How many to subtract from "this" to get the parent.
                          // If zero this die has no parent
   uint32_t m_sibling_idx : 31, // How many to add to "this" to get the sibling.
-      m_empty_children : 1;    // If a DIE says it had children, yet it just
-                               // contained a NULL tag, this will be set.
-  uint32_t m_abbr_idx : DIE_ABBR_IDX_BITSIZE,
-                        m_has_children : 1, // Set to 1 if this DIE has children
-                        m_tag : 16; // A copy of the DW_TAG value so we don't
-                                    // have to go through the compile unit
-                                    // abbrev table
+      // If it is zero, then the DIE doesn't have children, or the
+      // DWARF claimed it had children but the DIE only contained
+      // a single NULL terminating child.
+      m_has_children : 1;
+  uint16_t m_abbr_idx;
+  uint16_t m_tag; // A copy of the DW_TAG value so we don't have to go through
+                  // the compile unit abbrev table
 };
 
 #endif // SymbolFileDWARF_DWARFDebugInfoEntry_h_

source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp

 
 bool DWARFDebugInfoEntry::FastExtract(
     const DWARFDataExtractor &debug_info_data, const DWARFUnit *cu,
     const DWARFFormValue::FixedFormSizes &fixed_form_sizes,
     lldb::offset_t *offset_ptr) {
   m_offset = *offset_ptr;
   m_parent_idx = 0;
   m_sibling_idx = 0;
-  m_empty_children = false;
   const uint64_t abbr_idx = debug_info_data.GetULEB128(offset_ptr);
-  assert(abbr_idx < (1 << DIE_ABBR_IDX_BITSIZE));
+  lldbassert(abbr_idx <= UINT16_MAX);
   m_abbr_idx = abbr_idx;
 
   // assert (fixed_form_sizes);  // For best performance this should be
   // specified!
 
   if (m_abbr_idx) {
     lldb::offset_t offset = *offset_ptr;
 
bool DWARFDebugInfoEntry::Extract(SymbolFileDWARF *dwarf2Data,
   lldb::offset_t offset = *offset_ptr;
   //  if (offset >= cu_end_offset)
   //      Log::Status("DIE at offset 0x%8.8x is beyond the end of the current
   //      compile unit (0x%8.8x)", m_offset, cu_end_offset);
   if ((offset < cu_end_offset) && debug_info_data.ValidOffset(offset)) {
     m_offset = offset;
 
     const uint64_t abbr_idx = debug_info_data.GetULEB128(&offset);
-    assert(abbr_idx < (1 << DIE_ABBR_IDX_BITSIZE));
+    lldbassert(abbr_idx <= UINT16_MAX);
     m_abbr_idx = abbr_idx;
     if (abbr_idx) {
       const DWARFAbbreviationDeclaration *abbrevDecl =
           cu->GetAbbreviations()->GetAbbreviationDeclaration(abbr_idx);
 
       if (abbrevDecl) {
         m_tag = abbrevDecl->Tag();
         m_has_children = abbrevDecl->HasChildren();
strm.Printf("%.8x: %.8x %.8x %.8x 0x%4.4x %s%s\n", die_ref.GetOffset(),
                 DW_TAG_value_to_name(die_ref.Tag()),
                 die_ref.HasChildren() ? " *" : "");
   }
 }
 
 bool DWARFDebugInfoEntry::operator==(const DWARFDebugInfoEntry &rhs) const {
   return m_offset == rhs.m_offset && m_parent_idx == rhs.m_parent_idx &&
          m_sibling_idx == rhs.m_sibling_idx &&
-         m_empty_children == rhs.m_empty_children &&
          m_abbr_idx == rhs.m_abbr_idx && m_has_children == rhs.m_has_children &&
          m_tag == rhs.m_tag;
 }
 
 bool DWARFDebugInfoEntry::operator!=(const DWARFDebugInfoEntry &rhs) const {
   return !(*this == rhs);
 }

source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp

while (offset < next_cu_offset &&
     } else {
       if (null_die) {
         if (prev_die_had_children) {
           // This will only happen if a DIE says is has children but all it
           // contains is a NULL tag. Since we are removing the NULL DIEs from
           // the list (saves up to 25% in C++ code), we need a way to let the
           // DIE know that it actually doesn't have children.
           if (!m_die_array.empty())
-            m_die_array.back().SetEmptyChildren(true);
+            m_die_array.back().SetHasChildren(false);
         }
       } else {
         die.SetParentIndex(m_die_array.size() - die_index_stack[depth - 1]);
 
         if (die_index_stack.back())
           m_die_array[die_index_stack.back()].SetSiblingIndex(
               m_die_array.size() - die_index_stack.back());
 
if (null_die) {
       }
       prev_die_had_children = die_has_children;
     }
   }
 
   if (!m_die_array.empty()) {
     if (m_first_die) {
       // Only needed for the assertion.
-      m_first_die.SetEmptyChildren(m_die_array.front().GetEmptyChildren());
+      m_first_die.SetHasChildren(m_die_array.front().HasChildren());
       lldbassert(m_first_die == m_die_array.front());
     }
     m_first_die = m_die_array.front();
   }
 
   m_die_array.shrink_to_fit();
 
   ExtractDIEsEndCheck(offset);