[Lldb-commits] [PATCH] D63428: DWARF: Add "dwo_num" field to the DIERef class

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

I am concerned with increasing the size of DIERef objects. If we go to 12 bytes per DIERef, and we mostly store these in NameToDIE in "lldb_private::UniqueCStringMap<DIERef> m_map;" maps, which contain a std::vector of UniqueCStringMap::Entry objects which are:

  struct Entry {
    ConstString cstring;
    T value;
  };

Where T is DIERef. This will align to 24 bytes. Before we would be at 16 when DIERef was 8 bytes. I know we already exceeded the 8 byte size of DIERef with the added "section" field in DIERef with SVN revision 360872, so we already have 24 byte alignment, but the memory used by LLDB for large debug sessions will have increased significantly since a month ago.

If DIERef becomes the same size as DWARFDIE objects, we still need DIERef as when we index the DWARF we end up with DIERef objects and then we can unload the actual DWARF for the compile unit (so we can't switch over to using DWARFDIE since they have pointers to the loaded DWARF DIEs, but we will need to watch for code that might have been using a DIERef in lieu of DWARFDIE objects due to the DIERef size being smaller, but now that they are closer or will usually align to the same size, some clients might be able to switch over to use DWARFDIE objects now that there is really no size win.

So to address my concern with increase in DIERef size is wether we can use the "lldb::user_id_t" in the NameToDie map if we can always change a user_id_t into a DIERef and get back a size win in the process? Possibly look at anyone else storing DIERef objects and see if they can use lldb::user_id_t values too?



================
Comment at: source/Plugins/SymbolFile/DWARF/DIERef.h:59-62
+  unsigned m_dwo_num : 31;
   unsigned m_section : 1;
   dw_offset_t m_unit_offset;
   dw_offset_t m_die_offset;
----------------
I'm a bit concerned about increasing the size by 4 bytes here. All of our indexes are using maps of name to DIERef objects right? 


================
Comment at: source/Plugins/SymbolFile/DWARF/DIERef.h:64
 };
+static_assert(sizeof(DIERef) == 12, "");
 
----------------
Insert a message here? 


================
Comment at: source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp:245-246
 
-    DIERef ref(unit.GetDebugSection(), cu_offset, die.GetOffset());
+    DIERef ref(unit.GetSymbolFileDWARF().GetDwoNum(), unit.GetDebugSection(),
+               unit.GetOffset(), die.GetOffset());
     switch (tag) {
----------------
Make a DWARFDIE accessor function?:
```
DIERef DWARFDIE::GetDIERef() const;
```


================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1494
+    return DebugInfo()
+        ->GetUnitAtIndex(*die_ref.dwo_num())
+        ->GetDwoSymbolFile()
----------------
Can a DWO file be missing and ever return NULL here?


================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1495
+        ->GetUnitAtIndex(*die_ref.dwo_num())
+        ->GetDwoSymbolFile()
+        ->GetDIE(die_ref);
----------------
NULL can never be returned here?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63428/new/

https://reviews.llvm.org/D63428