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

[David Blaikie via llvm-commits]

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.

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

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?

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




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


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)


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.




================
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/fb93851a/attachment.html>