[Lldb-commits] [PATCH] D40466: DWZ 03/12: DWARFCompileUnit split to DWARFCompileUnitData

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added inline comments.


================
Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h:216
 
+  DWARFCompileUnitData *m_data;
+
----------------
jankratochvil wrote:
> clayborg wrote:
> > jankratochvil wrote:
> > > clayborg wrote:
> > > > Is there a reason this is a member variable that I am not seeing? Seems we could have this class inherit from DWARFCompileUnitData. I am guessing this will be needed for a future patch?
> > > Yes, future patch D40474 contains a new constructor so multiple `DWARFCompileUnit` then point to single `DWARFCompileUnitData`.  Sure that happens only in the case ot `DW_TAG_partial_unit` (one `DWARFCompileUnit` is read from a file while other `DWARFCompileUnit` are remapped instances with unique offset as used from units which did use `DW_TAG_imported_unit` for them).
> > > `DWARFCompileUnit(DWARFCompileUnitData *data, DWARFCompileUnit *main_cu);`
> > > 
> > We should just have an instance of this in this class and add a virtual function to retrieve it. Then we make a DWARFPartialUnit that subclasses this and has its own extra member variable and can use either one when it makes sense. Not a fan of just having a dangling pointer with no clear ownership. There is no "delete m_data" anywhere?
> The `DWARFCompileUnitData *m_data` content is being held in: `std::forward_list<DWARFCompileUnitData> SymbolFileDWARF::m_compile_unit_data_list` as I hope/believe a `DWARFCompileUnit` is never deleted until whole `SymbolFileDWARF` is deleted.  I did not use `std::shared_ptr<DWARFCompileUnitData> DWARFCompileUnit::m_data` as `shared_ptr` is both memory and lock-instruction-performance expensive.
> 
> Then we make a DWARFPartialUnit that subclasses this

We cannot because DWARFCompileUnit is constructed by `DWARFDebugInfo::ParseCompileUnitHeadersIfNeeded`->`DWARFCompileUnit::Extract` and it currently does not know whether it is `DW_TAG_compile_unit` or `DW_TAG_partial_unit`. But then `DWARFCompileUnit::Extract` could read even the very first DIE and save it for later use. Then `DWARFCompileUnit::ExtractDIEsIfNeeded` would not need the `bool cu_die_only` parameter and altogether I could also drop my D40471. Do you agree?
(This whole DWARF units parsing across the file is not too efficient anyway, there should be some index in use; and I would prefer the DWARF-5 one over the Apple one.)



https://reviews.llvm.org/D40466