[PATCH] D27995: Add the ability for DWARFDie objects to get the parent DWARFDie.

David Blaikie via llvm-commits llvm-commits at lists.llvm.org
Wed Dec 21 11:25:20 PST 2016


On Wed, Dec 21, 2016 at 8:47 AM Greg Clayton <clayborg at gmail.com> wrote:

>
> > On Dec 20, 2016, at 4:21 PM, David Blaikie <dblaikie at gmail.com> wrote:
> >
> > On Tue, Dec 20, 2016 at 3:53 PM Greg Clayton via Phabricator <
> reviews at reviews.llvm.org> wrote:
> > clayborg added a comment.
> >
> > I am not sure I would want to trim much more. Let me know if you see
> things I can trim. I could remove the "C2" die, but that is about it. Let
> me know if you see other stuff that can be trimmed.
> >
> >
> >
> > ================
> > Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:992
> > +    C2
> > +  };
> >
> > ----------------
> > The blocks you build with (classes in llvm/CodeGen) are different from
> the blocks that are in the DWARF parser API (llvm/DebugInfo/DWARF) so it
> really isn't practical to try and just create DIE objects since that isn't
> what our parser is using.
> >
> > But the code change you made isn't to the parser - so we don't need the
> parser to be involved in this test. We can unit test the DIE data
> structures directly by building some up and checking that they can walk
> their siblings/parents/children appropriately.
> >
> > So the only way to currently use the API is to actually encode bytes so
> we can decode them and use the DWARF parsing API.
> >
> > I'm thinking something possibly as simple as:
> >
> > DWARFDebugInfoEntry Entries[6];
> > Entries[0].setDepth(0);
> > Entries[1].setDepth(1);
> > Entries[2].setDepth(2);
> > Entries[3].setDepth(2);
> > Entries[4].setDepth(1);
> > Entries[5].setDepth(2);
> >
> > Check:
> > Entries[2].getSibling() == Entries[2]
> > Entries[3].getSibling() == NULL
> > Entries[1].getParent() == Entries[0]
> >
> > That sort of thing.
> >
> > Granted, this doesn't test the actual changes to parsing where the depth
> is tracked, so perhaps that's sufficient justification to test it all with
> the parser.
>
> Yeah, this is the main reason for testing it this way. It allows us to
> make changes to the DWARFDebugInfoEntry, DWARFUnit and DWARFDie class and
> verify things still behave correctly when it is parsed.
>
> >
> > (side note: not sure the "if depth == 1" special case in getParent is
> worth it - it should just fall out of the loop below quickly/easily enough)
> >
> > [It's a pity this change makes it impossible to navigate the
> parent/children of a DWARFDebugInfoEntry without the DWARFUnit around.
> Wonder if we can keep that & still be able to access the parent without
> overhead here...
>
> We can technically do this without the DWARFUnit, but then
> DWARFDebugInfoEntry is making assumptions on how it is stored in some other
> class. I can change this, but thought it was better design to let the class
> that stores the DWARFDebugInfoEntry objects decide how to find it parent
> and sibling.
>
> > Actually, can getParent not use the DWARFUnit? Since it's necessarily
> walking backwards, and necessarily must find a parent - it seems it
> wouldn't need access. It just takes a pointer and walks backwards in
> pointer decrements until it finds something with depth less than itself.
> That shouldn't require access to the DWARFUnit?
>
> I can implement this without DWARFUnit, but not that we have DWARFDie, it
> seemed a cleaner design to let the class that stores the
> DWARFDebugInfoEntry object figure out how to get the parent.
> >
> >
> > Next sibling is harder, since it could run off the end. Well, not with
> nulls present. Without them (as you're proposing) then it'd need access.
> Could potentially use an extra bit (eg: the high bit of depth) to store
> "I'm the last child".]
>
> I am fine with moving the logic back into DWARFDebugInfoEntry, but I still
> think it is cleaner to let DWARFUnit, the class that knows how the
> DWARFDebugInfoEntry objects are stored, figure out how the get the parent
> and sibling. No one uses DWARFDebugInfoEntry directly anymore except
> DWARFUnit and DWARFDie internals, all clients use DWARFDie. And having the
> DWARFUnit do the finding does allow it to correctly bounds check when
> getting the sibling in case we get incomplete DWARF.
>
> If I remove the nulls we can still put a single terminal entry on the
> entire list so we don't walk off the end. We won't need to do this in
> DWARFUnit gets the parent/siblings, but we would if we want to let
> DWARFDebugInfoEntry assume how its been stored. And we can change depth to
> use 24 bits, and store 8 bits of other data in there, including as you said
> "I am the last child". As we change these kinds of things, I would rather
> have complete tests that ensure we don't regress.
>

Fair points, all - it was somewhat awkward that the DWARFDebugInfoEntry
made assumptions about how it was stored.

That said, I am a bit confuesd by the use of getDIEAtIndex in the
implementation of getParent/getSibling - when the implementation of both
accesses the DieArray[I] directly. I suppose it's convenient to handle the
this + value wrapping, but seems small. The bounds checking in that
function seems strange - I'd expect it to be an assertion (like
std::vector::op[] and other array-like operations in C++ APIs).


>
> > ================
> > Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1103
> > +  EXPECT_EQ(C1.getParent().getOffset(), C.getOffset());
> > +  EXPECT_EQ(C2.getParent().getOffset(), C.getOffset());
> > +
> > ----------------
> > I believe this is close to the bare bones as we can get it. We are
> testing two DIEs, B and C that have children and making sure all functions
> get the right children and siblings.
> >
> > It's this part "making sure all functions get the right children and
> siblings" that I'm wondering about - does each of those checks carry its
> weight, or are they redundantly testing the same basic codepaths? Are there
> bugs that are likely to be exposed by each of them that wouldn't be exposed
> by any others?
>
> We are really testing the parser here to make sure it correctly parses the
> debug info and sets up the data needed for us to correctly navigate the DIE
> relations. You asked for my to test:
>
> A
>   A1
> B
>   B1
>
> And we need to ensure everyone has the correctly parent/sibling/child to
> ensure we don't regress.
>

But we don't need to check /everyone/ to have pretty good confidence we
won't regress (indeed: there are infinitely many trees we could create and
test - which we aren't, so we're picking particular relationships that give
us pretty good confidence that we won't regress).

Specifically, for example - we wanted to add the C subtree to ensure that
walking from B2 to its next sibling would produce null, not C1. So that's
the only thing I'd expect to test there - I wouldn't expect to test other
properties of the C subtree (I wouldn't expect to test B's sibling is C, or
any of C's children, for example).


>
> >
> > I can cut out the whole C tree, but then that won't test asking B2 to
> get its sibling and making sure we don't return C1 as its sibling...
> >
> > Right - it might be that the input data structure is already minimal,
> what I'm wondering about is whether we need to check as many properties of
> the them - it seems we only have a few interesting properties to test. (in
> the same way that we don't do an exact match for the output in lit tests in
> LLVM - we test a few important properties that ensure the functionality
> works)
>
> I like that we are fully testing this simple test case to ensure our
> parser doesn't regress, even with large changes in how data is stored.
>
> > What would you cut from this? I can cut the B2 and C2, bit would rather
> not, as I want two consecutive DIEs with no children as a test.
> >
> > What's the particular interesting thing about that?
> >
> > I'd probably cut C2, though - you can cover that with B1/B2.
>
> I can cut C2.
>
> >
> >
> >
> > ================
> > Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1110
> > +  EXPECT_FALSE(DefaultDie.getFirstChild().isValid());
> > +  EXPECT_FALSE(DefaultDie.getSibling().isValid());
> > +}
> > ----------------
> > I can remove it,
> >
> >
> > https://reviews.llvm.org/D27995
> >
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161221/f4dc2cb6/attachment.html>


More information about the llvm-commits mailing list