[PATCH] D102634: Calculate indexes of last child of each DWARF entry once during tryExtractDIEsIfNeeded.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:833-842
+  // We do not want access out of bounds when parsing corrupted debug data.
+  size_t I = getDIEIndex(Die);
+  if (I >= LastChildren.size())
+    return DWARFDie();
+  size_t LastChild = LastChildren[I];
+  if (!LastChild) {
+    // TODO Shouldn't Die->hasChildren() be false in that case?
----------------
simon.giesecke wrote:
> avl wrote:
> > avl wrote:
> > > clayborg wrote:
> > > > This is a side affect of how the DWARFDebugInfoEntry class is created. LLDB has a more efficient way of doing things as each DWARFDebugInfoEntry knows its sibling index. Since all of the DIEs are contained in an std::vector inside of the DWARFUnit, we can just store the parent index, the sibling index. See the file lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h.
> > > > 
> > > > 
> > > > ```
> > > >   uint32_t m_parent_idx; // How many to subtract from "this" to get the parent.
> > > >   uint32_t m_sibling_idx; // How many to add to "this" to get the sibling.
> > > > ```
> > > > 
> > > > The current DWARFDebugInfoEntry just contains a offset and a depth and an abbrev pointer. This info doesn't make it easy to navigate the DWARFDie objects sibling, and parent. If the LLVM DWARF parser adopts this parent index and sibling index, then navigation can happen much quicker and this function can simply get the sibling index and subtract 1 as that will always the the NULL tag that terminates the previous DIE child chain.
> > > > uint32_t m_parent_idx; // How many to subtract from "this" to get the parent.
> > > > uint32_t m_sibling_idx; // How many to add to "this" to get the sibling.
> > > > 
> > > > The current DWARFDebugInfoEntry just contains a offset and a depth and an abbrev pointer. This info doesn't make it easy to navigate the DWARFDie objects sibling, and parent. If the LLVM DWARF parser adopts this parent index and sibling index, then navigation can happen much quicker and this function can simply get the sibling index and subtract 1 as that will always the the NULL tag that terminates the previous DIE child chain.
> > > > 
> > > 
> > > If the LLVM DWARF parser adopts above parent index and sibling index solution then it would also be useful for dsymutil. Currently it creates links to parents by separate pass.
> > > 
> > > This is a side affect of how the DWARFDebugInfoEntry class is created. LLDB has a more efficient way of doing things as each DWARFDebugInfoEntry knows its sibling index. Since all of the DIEs are contained in an std::vector inside of the DWARFUnit, we can just store the parent index, the sibling index. See the file lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h.
> > > 
> > > 
> > > ```
> > >   uint32_t m_parent_idx; // How many to subtract from "this" to get the parent.
> > >   uint32_t m_sibling_idx; // How many to add to "this" to get the sibling.
> > > ```
> > > 
> > > The current DWARFDebugInfoEntry just contains a offset and a depth and an abbrev pointer. This info doesn't make it easy to navigate the DWARFDie objects sibling, and parent. If the LLVM DWARF parser adopts this parent index and sibling index, then navigation can happen much quicker and this function can simply get the sibling index and subtract 1 as that will always the the NULL tag that terminates the previous DIE child chain.
> > 
> > Since it was mentioned in this review - Is somebody going to implementing that idea(replacing "Depth" with two smaller fields)?
> > Since it was mentioned in this review - Is somebody going to implementing that idea(replacing "Depth" with two smaller fields)?
> 
> I would consider that when working on this again. However, I am not sure when I will be able to get back to this. If someone else wants to pick it up, that would be great.
> > Since it was mentioned in this review - Is somebody going to implementing that idea(replacing "Depth" with two smaller fields)?
> 
> I would consider that when working on this again. However, I am not sure when I will be able to get back to this. If someone else wants to pick it up, that would be great.

I am interested in picking up this. Will do this. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102634