[PATCH] D110363: [DWARF][NFC] add ParentIdx and SiblingIdx to DWARFDebugInfoEntry for faster navigation.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D110363#3028027 <https://reviews.llvm.org/D110363#3028027>, @clayborg wrote:

> In D110363#3027286 <https://reviews.llvm.org/D110363#3027286>, @avl wrote:
>
>> After some thinking, it looks like this idea "So the DWARFUnit class no longer needs to be involved in the getting the parent, sibling, child stuff" may not be suitable for dies navigation. The reason for this is that DWARFDebugInfoEntry does not know the size of DWARFDebugInfoEntry vector. That makes it impossible to assert and/or check whether new pointers to the DWARFDebugInfoEntry-es are valid. i.e.
>>
>> I propose to use current solution when DWARFUnit does dies navigation and properly validates elements. What do you think?
>
> The assertions can't be done, that is true, but the theory is if we parse all of the DIEs correctly, they should all have valid values for the ParentIdx and SiblingIdx up front and we should have nothing to worry about. We should create the data correctly one time and then trust it. We have been using this in the LLDB parser for many years and it is quite stable and efficient.

Yep, +1 to that. If these offsets were based on parsed input, then there'd be a possibility of them being incorrect/invalid/out of range, but they aren't - they'd be computed based on the hierarchy created in memory by libDebugInfo.

That said, I do find it a bit unfortunate to have objects that have an implicit requirement on how they're allocated - knowing they're in an array and walking around that array to find other things. But that ship's probably solidly sailed on these APIs and might as well do more of it? (or would it be reasonable to shift these sort of APIs up into DWARFDie and access the array via the DWARFUnit? Though maybe there's observable performance cost of that? I'd be a bit surprised)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110363