[PATCH] [DebugInfo] Add a getParent method to DWARFDebugInformationEntryMinimal.

[Frédéric Riss]

> On Jan 17, 2015, at 2:39 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Fri, Jan 16, 2015 at 4:16 PM, Frederic Riss <friss at apple.com <mailto:friss at apple.com>> wrote:
> Hi samsonov, dblaikie,
> 
> The DIE representation of libDebugInfo is designed to be minimal
> (as the class name implies). The information that's there allows
> to walk the DIE tree down, but you can't get back to the root
> from an arbitrary node.
> 
> This patch adds this ability with the goal of using it in
> llvm-dsymutil. I could also use this information in dwarfdump
> itself to easily support dumping the full DIE hierarchy of a
> selected DIE when I finally implement a search option.
> 
> The added information definitely has a cost: the size of
> the structure is increased by 8 bytes on 64 bits archs
> due to padding and this combined to the added logic makes
> the raw parsing between 5 and 10% slower.
> 
> Just for llvm-dsymutil, this is optional. I can gather the
> parent chains in a separate path and store them, but I think
> that the performance/aded functionality benefit is worth
> it. WDYT?
> 
> I doubt the performance of dwarfdump is terribly important - but I would imagine the performance of dsymutil will be/is, so it might be worth looking at the alternatives that don't increase the memory cost of all DIEs?

Yes speed is more important for dsymutil than dwarfdump. I was more concerned about other clients like llvm-symbolizer that might also parse the DIE tree. I fail to see the connection between that fact and the conclusion you draw though. If we don’t store the parent in libDebugInfo, I will have to store it in dsymutil anyway (as I already said, not such a big deal).

> (I imagine a search query that responds with a path, rather than a node, would be a reasonable API, for example - but I dunno)

To be more precise about the use I envisioned for dwarfdump itself: the accelerator tables provide a mapping from names to DIE offset, allowing to do fast names lookups. Let’s say I implement a —search=<name> option to dwarfdump using those tables. If you just want to print the target DIE then libDebugInfo handles that just fine. If you want to print the whole path that leads to that DIE, you have to do some additional work that storing the parent would avoid.

Fred

>  
> 
> http://reviews.llvm.org/D7025 <http://reviews.llvm.org/D7025>
> 
> Files:
>   include/llvm/DebugInfo/DWARFDebugInfoEntry.h
>   lib/DebugInfo/DWARFUnit.cpp
> 
> Index: include/llvm/DebugInfo/DWARFDebugInfoEntry.h
> ===================================================================
> --- include/llvm/DebugInfo/DWARFDebugInfoEntry.h
> +++ include/llvm/DebugInfo/DWARFDebugInfoEntry.h
> @@ -30,13 +30,16 @@
>    /// Offset within the .debug_info of the start of this entry.
>    uint32_t Offset;
> 
> +  /// How many to substract to "this" to get the parent.
> +  uint32_t ParentIdx;
> +
>    /// How many to add to "this" to get the sibling.
>    uint32_t SiblingIdx;
> 
>    const DWARFAbbreviationDeclaration *AbbrevDecl;
>  public:
>    DWARFDebugInfoEntryMinimal()
> -    : Offset(0), SiblingIdx(0), AbbrevDecl(nullptr) {}
> +    : Offset(0), ParentIdx(0), SiblingIdx(0), AbbrevDecl(nullptr) {}
> 
>    void dump(raw_ostream &OS, DWARFUnit *u, unsigned recurseDepth,
>              unsigned indent = 0) const;
> @@ -66,6 +69,11 @@
>      return SiblingIdx > 0 ? this + SiblingIdx : nullptr;
>    }
> 
> +  // Returns the parent DIE or null if this is the root DIE.
> +  const DWARFDebugInfoEntryMinimal *getParent() const {
> +    return ParentIdx ? this - ParentIdx : nullptr;
> +  }
> +
>    // We know we are kept in a vector of contiguous entries, so we know
>    // we don't need to store our child pointer, if we have a child it will
>    // be the next entry in the list...
> @@ -82,6 +90,15 @@
>        SiblingIdx = 0;
>    }
> 
> +  void setParent(const DWARFDebugInfoEntryMinimal *Parent) {
> +    if (Parent) {
> +      // We know we are kept in a vector of contiguous entries, so we know
> +      // our parent will be soewhere before "this".
> +      ParentIdx = this - Parent;
> +    } else
> +      ParentIdx = 0;
> +  }
> +
>    const DWARFAbbreviationDeclaration *getAbbreviationDeclarationPtr() const {
>      return AbbrevDecl;
>    }
> Index: lib/DebugInfo/DWARFUnit.cpp
> ===================================================================
> --- lib/DebugInfo/DWARFUnit.cpp
> +++ lib/DebugInfo/DWARFUnit.cpp
> @@ -143,12 +143,18 @@
>    if (DieArray.size() <= 1)
>      return;
> 
> -  std::vector<DWARFDebugInfoEntryMinimal *> ParentChain;
> +  // Make sure the vector is never empty so that we don't have to
> +  // check for emptyness inside the loop bellow.
> +  std::vector<DWARFDebugInfoEntryMinimal *> ParentChain(1,&DieArray[0]);
>    DWARFDebugInfoEntryMinimal *SiblingChain = nullptr;
>    for (auto &DIE : DieArray) {
>      if (SiblingChain) {
>        SiblingChain->setSibling(&DIE);
>      }
> +    // Note that this will call setParent with itself on the root
> +    // node. This will result in a null offset which is handled as 'no
> +    // parent'.
> +    DIE.setParent(ParentChain.back());
>      if (const DWARFAbbreviationDeclaration *AbbrDecl =
>              DIE.getAbbreviationDeclarationPtr()) {
>        // Normal DIE.
> @@ -165,7 +171,7 @@
>      }
>    }
>    assert(SiblingChain == nullptr || SiblingChain == &DieArray[0]);
> -  assert(ParentChain.empty());
> +  assert(ParentChain.size() == 1 && ParentChain[0] == &DieArray[0]);
>  }
> 
>  void DWARFUnit::extractDIEsToVector(
> 
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/ <http://reviews.llvm.org/settings/panel/emailpreferences/>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150119/2b9c047b/attachment.html>