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

[Greg Clayton via llvm-commits]

> 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.

> ================
> 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.

>  
> 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
> 
> 
>