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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
