DWZ 02/12: refactor: Unify+simplify DWARFCompileUnit ctor+Clear() into in-class initializers + Extract()

[jankratochvil]

Summary

It has no functionality effect but I find it much more simplified for further refactorizations and extensions in my patchset.

I was concerned about the worse performance of DWARFDebugInfo::Parse this way of allocating+destroying a CU for each iteration but I see it is now used only by DWARFDebugInfo::Dump so that is no longer a problem.

[labath]

You're using a fairly novel (to this codebase at least) simplification technique, so I think we should discuss that first. The way we have normally done these things is to just have the constructor call the Clear() function.

Personally, I am not really sure what to think about this approach. On one side, I kinda like it, but on the other one, it feels somewhat dodgy.

[clayborg]

Good change in the header file. I am not sure I like the destruct this object in place and replace with new version... If this is commonly done and acceptable form of C++ I would be ok with it, but I agree with Pavel, it seems a little bit off the books.

[jankratochvil]

In D40212#929716, @clayborg wrote:

Good change in the header file.

If you mean the in-class initializers they obviously cannot be used without the in-place construction+destruction as they would stay duplicate to the Clear() method.

OK if you are also uncomfortable with it I will keep Clear() as is and just call Clear() from the ctor to unify it at least a bit.

[clayborg]

In D40212#929740, @jankratochvil wrote:

In D40212#929716, @clayborg wrote:

Good change in the header file.

If you mean the in-class initializers they obviously cannot be used without the in-place construction+destruction as they would stay duplicate to the Clear() method.

OK if you are also uncomfortable with it I will keep Clear() as is and just call Clear() from the ctor to unify it at least a bit.

Or we can get rid of Clear(), make the constructor private and make DWARFCompileInit::Extract be static and return a shared pointer as suggested in your other patch. I doubt many compile units fail to construct so we can just avoid the Clear() call all together and keep the initialization in the header file. Let me know

[jankratochvil]

DWZ support

[jankratochvil]

refactor: Unify+simplify DWARFCompileUnit ctor+Clear() into in-class initializers + Extract()

It has no functionality effect but I find it much more simplified for further
refactorizations and extensions in my patchset.

I was concerned about the worse performance of DWARFDebugInfo::Parse this way
of allocating+destroying a CU for each iteration but I see it is now used only
by DWARFDebugInfo::Dump so that is no longer a problem.

Differential revision: https://reviews.llvm.org/D40212

[clayborg]

Won't hold up the checkin for the cleaner while loop, but feel free to fix that and checkin if it works.

source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp

248–249 ↗

(On Diff #124290)

Might be cleaner to do:

DWARFCompileUnitSP cu;
while ((cu = DWARFCompileUnit::Extract(dwarf2Data, &offset))) {

[jankratochvil]

source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp

248–249 ↗

(On Diff #124290)

I did not know LLVM style can be this brief, that is sure better, implemented.

lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h

 #include "DWARFDIE.h"
 #include "DWARFDebugInfoEntry.h"
 #include "lldb/lldb-enumerations.h"
 
 class NameToDIE;
 class SymbolFileDWARF;
 class SymbolFileDWARFDwo;
 
+typedef std::shared_ptr<DWARFCompileUnit> DWARFCompileUnitSP;
+
 class DWARFCompileUnit {
 public:
   enum Producer {
     eProducerInvalid = 0,
     eProducerClang,
     eProducerGCC,
     eProducerLLVMGCC,
     eProcucerOther
   };
 
-  DWARFCompileUnit(SymbolFileDWARF *dwarf2Data);
+  static DWARFCompileUnitSP Extract(SymbolFileDWARF *dwarf2Data,
+      lldb::offset_t *offset_ptr);
   ~DWARFCompileUnit();
 
-  bool Extract(const lldb_private::DWARFDataExtractor &debug_info,
-               lldb::offset_t *offset_ptr);
   size_t ExtractDIEsIfNeeded(bool cu_die_only);
   DWARFDIE LookupAddress(const dw_addr_t address);
   size_t AppendDIEsWithTag(const dw_tag_t tag,
                            DWARFDIECollection &matching_dies,
                            uint32_t depth = UINT32_MAX) const;
-  void Clear();
   bool Verify(lldb_private::Stream *s) const;
   void Dump(lldb_private::Stream *s) const;
   // Offset of the initial length field.
   dw_offset_t GetOffset() const { return m_offset; }
   lldb::user_id_t GetID() const;
   // Size in bytes of the initial length + compile unit header.
   uint32_t Size() const { return m_is_dwarf64 ? 23 : 11; }
   bool ContainsDIEOffset(dw_offset_t die_offset) const {
public:
   }
 
   dw_offset_t GetBaseObjOffset() const { return m_base_obj_offset; }
 
 protected:
   SymbolFileDWARF *m_dwarf2Data;
   std::unique_ptr<SymbolFileDWARFDwo> m_dwo_symbol_file;
   const DWARFAbbreviationDeclarationSet *m_abbrevs;
-  void *m_user_data;
+  void *m_user_data = nullptr;
   DWARFDebugInfoEntry::collection
       m_die_array; // The compile unit debug information entry item
   std::unique_ptr<DWARFDebugAranges> m_func_aranges_ap; // A table similar to
                                                         // the .debug_aranges
                                                         // table, but this one
                                                         // points to the exact
                                                         // DW_TAG_subprogram
                                                         // DIEs
-  dw_addr_t m_base_addr;
+  dw_addr_t m_base_addr = 0;
   // Offset of the initial length field.
   dw_offset_t m_offset;
   dw_offset_t m_length;
   uint16_t m_version;
   uint8_t m_addr_size;
-  Producer m_producer;
-  uint32_t m_producer_version_major;
-  uint32_t m_producer_version_minor;
-  uint32_t m_producer_version_update;
-  lldb::LanguageType m_language_type;
+  Producer m_producer = eProducerInvalid;
+  uint32_t m_producer_version_major = 0;
+  uint32_t m_producer_version_minor = 0;
+  uint32_t m_producer_version_update = 0;
+  lldb::LanguageType m_language_type = lldb::eLanguageTypeUnknown;
   bool m_is_dwarf64;
-  lldb_private::LazyBool m_is_optimized;
-  dw_addr_t m_addr_base;         // Value of DW_AT_addr_base
-  dw_addr_t m_ranges_base;       // Value of DW_AT_ranges_base
-  dw_offset_t m_base_obj_offset; // If this is a dwo compile unit this is the
-                                 // offset of the base compile unit in the main
-                                 // object file
+  lldb_private::LazyBool m_is_optimized = lldb_private::eLazyBoolCalculate;
+  dw_addr_t m_addr_base = 0;     // Value of DW_AT_addr_base
+  dw_addr_t m_ranges_base = 0;   // Value of DW_AT_ranges_base
+  // If this is a dwo compile unit this is the offset of the base compile unit
+  // in the main object file
+  dw_offset_t m_base_obj_offset = DW_INVALID_OFFSET;
 
   void ParseProducerInfo();
 
   static void
   IndexPrivate(DWARFCompileUnit *dwarf_cu, const lldb::LanguageType cu_language,
                const DWARFFormValue::FixedFormSizes &fixed_form_sizes,
                const dw_offset_t cu_offset, NameToDIE &func_basenames,
                NameToDIE &func_fullnames, NameToDIE &func_methods,
                NameToDIE &func_selectors, NameToDIE &objc_class_selectors,
                NameToDIE &globals, NameToDIE &types, NameToDIE &namespaces);
 
 private:
+  DWARFCompileUnit(SymbolFileDWARF *dwarf2Data);
+
   const DWARFDebugInfoEntry *GetCompileUnitDIEPtrOnly() {
     ExtractDIEsIfNeeded(true);
     if (m_die_array.empty())
       return NULL;
     return &m_die_array[0];
   }
 
   const DWARFDebugInfoEntry *DIEPtr() {

lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp

 
 using namespace lldb;
 using namespace lldb_private;
 using namespace std;
 
 extern int g_verbose;
 
 DWARFCompileUnit::DWARFCompileUnit(SymbolFileDWARF *dwarf2Data)
-    : m_dwarf2Data(dwarf2Data), m_abbrevs(NULL), m_user_data(NULL),
-      m_die_array(), m_func_aranges_ap(), m_base_addr(0),
-      m_offset(DW_INVALID_OFFSET), m_length(0), m_version(0),
-      m_addr_size(DWARFCompileUnit::GetDefaultAddressSize()),
-      m_producer(eProducerInvalid), m_producer_version_major(0),
-      m_producer_version_minor(0), m_producer_version_update(0),
-      m_language_type(eLanguageTypeUnknown), m_is_dwarf64(false),
-      m_is_optimized(eLazyBoolCalculate), m_addr_base(0),
-      m_ranges_base(0), m_base_obj_offset(DW_INVALID_OFFSET) {}
+    : m_dwarf2Data(dwarf2Data) {}
 
 DWARFCompileUnit::~DWARFCompileUnit() {}
 
-void DWARFCompileUnit::Clear() {
-  m_offset = DW_INVALID_OFFSET;
-  m_length = 0;
-  m_version = 0;
-  m_abbrevs = NULL;
-  m_addr_size = DWARFCompileUnit::GetDefaultAddressSize();
-  m_base_addr = 0;
-  m_die_array.clear();
-  m_func_aranges_ap.reset();
-  m_user_data = NULL;
-  m_producer = eProducerInvalid;
-  m_language_type = eLanguageTypeUnknown;
-  m_is_dwarf64 = false;
-  m_is_optimized = eLazyBoolCalculate;
-  m_addr_base = 0;
-  m_base_obj_offset = DW_INVALID_OFFSET;
-}
-
-bool DWARFCompileUnit::Extract(const DWARFDataExtractor &debug_info,
-                               lldb::offset_t *offset_ptr) {
-  Clear();
+DWARFCompileUnitSP DWARFCompileUnit::Extract(SymbolFileDWARF *dwarf2Data,
+  DWARFCompileUnitSP cu_sp(new DWARFCompileUnit(dwarf2Data));
+  // Out of memory?
+  if (cu_sp.get() == NULL)
+    return nullptr;
 
-  m_offset = *offset_ptr;
+  const DWARFDataExtractor &debug_info = dwarf2Data->get_debug_info_data();
+
+  cu_sp->m_offset = *offset_ptr;
 
   if (debug_info.ValidOffset(*offset_ptr)) {
     dw_offset_t abbr_offset;
-    const DWARFDebugAbbrev *abbr = m_dwarf2Data->DebugAbbrev();
-    m_length = debug_info.GetDWARFInitialLength(offset_ptr);
-    m_is_dwarf64 = debug_info.IsDWARF64();
-    m_version = debug_info.GetU16(offset_ptr);
+    const DWARFDebugAbbrev *abbr = dwarf2Data->DebugAbbrev();
+    cu_sp->m_length = debug_info.GetDWARFInitialLength(offset_ptr);
+    cu_sp->m_is_dwarf64 = debug_info.IsDWARF64();
+    cu_sp->m_version = debug_info.GetU16(offset_ptr);
     abbr_offset = debug_info.GetDWARFOffset(offset_ptr);
-    m_addr_size = debug_info.GetU8(offset_ptr);
+    cu_sp->m_addr_size = debug_info.GetU8(offset_ptr);
 
-    bool length_OK = debug_info.ValidOffset(GetNextCompileUnitOffset() - 1);
-    bool version_OK = SymbolFileDWARF::SupportedVersion(m_version);
+    bool length_OK =
+        debug_info.ValidOffset(cu_sp->GetNextCompileUnitOffset() - 1);
+    bool version_OK = SymbolFileDWARF::SupportedVersion(cu_sp->m_version);
     bool abbr_offset_OK =
-        m_dwarf2Data->get_debug_abbrev_data().ValidOffset(abbr_offset);
-    bool addr_size_OK = ((m_addr_size == 4) || (m_addr_size == 8));
+        dwarf2Data->get_debug_abbrev_data().ValidOffset(abbr_offset);
+    bool addr_size_OK = (cu_sp->m_addr_size == 4) || (cu_sp->m_addr_size == 8);
 
     if (length_OK && version_OK && addr_size_OK && abbr_offset_OK &&
         abbr != NULL) {
-      m_abbrevs = abbr->GetAbbreviationDeclarationSet(abbr_offset);
-      return true;
+      cu_sp->m_abbrevs = abbr->GetAbbreviationDeclarationSet(abbr_offset);
+      return cu_sp;
     }
 
     // reset the offset to where we tried to parse from if anything went wrong
-    *offset_ptr = m_offset;
+    *offset_ptr = cu_sp->m_offset;
   }
 
-  return false;
+  return nullptr;
 }
 
 void DWARFCompileUnit::ClearDIEs(bool keep_compile_unit_die) {
   if (m_die_array.size() > 1) {
     // std::vectors never get any smaller when resized to a smaller size,
     // or when clear() or erase() are called, the size will report that it
     // is smaller, but the memory allocated remains intact (call capacity()
     // to see this). So we need to create a temporary vector and swap the

lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h

enum {
     eDumpFlag_ShowForm = (1 << 1), // Show the DW_form type
     eDumpFlag_ShowAncestors =
         (1 << 2) // Show all parent DIEs when dumping single DIEs
   };
 
   DWARFDebugAranges &GetCompileUnitAranges();
 
 protected:
-  typedef std::shared_ptr<DWARFCompileUnit> DWARFCompileUnitSP;
-
   static bool OffsetLessThanCompileUnitOffset(dw_offset_t offset,
                                               const DWARFCompileUnitSP &cu_sp);
 
   typedef std::vector<DWARFCompileUnitSP> CompileUnitColl;
 
   //----------------------------------------------------------------------
   // Member variables
   //----------------------------------------------------------------------

lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp

DWARFDebugAranges &DWARFDebugInfo::GetCompileUnitAranges() {
   }
   return *m_cu_aranges_ap.get();
 }
 
 void DWARFDebugInfo::ParseCompileUnitHeadersIfNeeded() {
   if (m_compile_units.empty()) {
     if (m_dwarf2Data != NULL) {
       lldb::offset_t offset = 0;
-      const DWARFDataExtractor &debug_info_data =
-          m_dwarf2Data->get_debug_info_data();
-      while (debug_info_data.ValidOffset(offset)) {
-        DWARFCompileUnitSP cu_sp(new DWARFCompileUnit(m_dwarf2Data));
-        // Out of memory?
+      for (;;) {
+        DWARFCompileUnitSP cu_sp =
+            DWARFCompileUnit::Extract(m_dwarf2Data, &offset);
         if (cu_sp.get() == NULL)
           break;
 
-        if (cu_sp->Extract(debug_info_data, &offset) == false)
-          break;
-
         m_compile_units.push_back(cu_sp);
 
         offset = cu_sp->GetNextCompileUnitOffset();
       }
     }
   }
 }
 

 // for different tasks such as parsing the file contents into a
 // structured data, dumping, verifying and much more.
 //----------------------------------------------------------------------
 void DWARFDebugInfo::Parse(SymbolFileDWARF *dwarf2Data, Callback callback,
                            void *userData) {
   if (dwarf2Data) {
     lldb::offset_t offset = 0;
     uint32_t depth = 0;
-    DWARFCompileUnitSP cu(new DWARFCompileUnit(dwarf2Data));
-    if (cu.get() == NULL)
-      return;
     DWARFDebugInfoEntry die;
 
-    while (cu->Extract(dwarf2Data->get_debug_info_data(), &offset)) {
+    DWARFCompileUnitSP cu;
+    while ((cu = DWARFCompileUnit::Extract(dwarf2Data, &offset))) {
       const dw_offset_t next_cu_offset = cu->GetNextCompileUnitOffset();
 
       depth = 0;
       // Call the callback function with no DIE pointer for the compile unit
       // and get the offset that we are to continue to parse from
       offset = callback(dwarf2Data, cu.get(), NULL, offset, depth, userData);
 
       // Make sure we are within our compile unit
while ((cu = DWARFCompileUnit::Extract(dwarf2Data, &offset))) {
       }
 
       // Make sure the offset returned is valid, and if not stop parsing.
       // Returning DW_INVALID_OFFSET from this callback is a good way to end
       // all parsing
       if (!dwarf2Data->get_debug_info_data().ValidOffset(offset))
         break;
 
-      // See if during the callback anyone retained a copy of the compile
-      // unit other than ourselves and if so, let whomever did own the object
-      // and create a new one for our own use!
-      if (!cu.unique())
-        cu.reset(new DWARFCompileUnit(dwarf2Data));
-
       // Make sure we start on a proper
       offset = next_cu_offset;
     }
   }
 }
 
 typedef struct DumpInfo {
   DumpInfo(Stream *init_strm, uint32_t off, uint32_t depth)