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.

source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h

 protected:
   SymbolFileDWARF *m_dwarf2Data;
   std::unique_ptr<SymbolFileDWARFDwo> m_dwo_symbol_file;
-  const DWARFAbbreviationDeclarationSet *m_abbrevs;
-  void *m_user_data;
+  const DWARFAbbreviationDeclarationSet *m_abbrevs = nullptr;
+  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

                                                         // points to the exact
                                                         // DW_TAG_subprogram
                                                         // DIEs
-  dw_addr_t m_base_addr;
-  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;
-  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
+  dw_addr_t m_base_addr = 0;
+  dw_offset_t m_offset = DW_INVALID_OFFSET;
+  dw_offset_t m_length = 0;
+  uint16_t m_version = 0;
+  uint8_t m_addr_size = GetDefaultAddressSize();
+  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 = false;
+  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();
 

source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp

 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;
+  SymbolFileDWARF *dwarf2Data = m_dwarf2Data;
+  this->~DWARFCompileUnit();
+  new (this) DWARFCompileUnit(dwarf2Data);
 }
 
 bool DWARFCompileUnit::Extract(const DWARFDataExtractor &debug_info,