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

[Pavel Labath via Phabricator via lldb-commits]

labath marked 7 inline comments as done.
labath added a comment.

I've been thinking about the DIERef class a lot while doing this, and the more I thought about it, the more I became convinced that forcing everything to go through DIERefs is not a good idea. It is kind of useful to have it as a sort of exchange currency between various components, but that means it forces an particular access pattern for the debug info, one which may not be always optimal. For example, there's no reason why the manual index would need to use offsets of anything. Offsets require binary search, but the manual index has already gone through the debug info once, so it can easily remember the indexes of everything. Indexes don't need binary search, and they can be stored more compactly. Even for the debug_names index, which does use offsets, the DIERef representation is not the ideal form. That's because it uses unit-relative die indexes while DIERef stores the absolute index. This means it still has to look up the DWO unit in order to correctly construct an absolute die offset.

So, I was thinking that instead of the indexes handing vectors of DIERef, they could provide an iterator api that would return DWARFDIEs directly. This way each index would be completely free to use whatever internal representation it wants. All it would need to do is to be able to convert it to a DWARFDIE eventually. The reason I've put putting it off for now is that the iterators, when combined with the polymorphism of the index class can get a bit messy, and so I wasn't sure if this is worth that effort. However, I agree that the size of the manual index is important, so I am going to post a dumbed-down version of this idea, which uses a compact representation internally, but then still converts it to an array of DIERefs.



================
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;
----------------
clayborg wrote:
> 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? 
I would say only the manual index is affected by this. All the indexes *return* results as a DIERef, but I don't think that's a problem because they don't store them this way internally.


================
Comment at: source/Plugins/SymbolFile/DWARF/DIERef.h:64
 };
+static_assert(sizeof(DIERef) == 12, "");
 
----------------
clayborg wrote:
> Insert a message here? 
What would you have me say? I can't think of anything that wouldn't already be obvious from the assert condition. I think that's one of the reasons why c++>=14 makes the message optional..


================
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) {
----------------
clayborg wrote:
> Make a DWARFDIE accessor function?:
> ```
> DIERef DWARFDIE::GetDIERef() const;
> ```
Actually, there is one already, I just forgot to use it.


================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1494
+    return DebugInfo()
+        ->GetUnitAtIndex(*die_ref.dwo_num())
+        ->GetDwoSymbolFile()
----------------
clayborg wrote:
> Can a DWO file be missing and ever return NULL here?
Given that the DWO numbers are completely made up and handed out by SymbolFileDWARFDwos, the only way you can get a valid dwo number is if you already had a SymbolFileDWARFDwo to give it to you. That means we can assume that the unit at the given index will be a skeleton unit and that it will contain a valid dwo symbol file.


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

https://reviews.llvm.org/D63428