<div dir="ltr"><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div dir="ltr" class="gmail_msg">On Tue, Dec 20, 2016 at 3:53 PM Greg Clayton via Phabricator <<a href="mailto:reviews@reviews.llvm.org" class="gmail_msg" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br class="gmail_msg"></div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">clayborg added a comment.<br class="gmail_msg">
<br class="gmail_msg">
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.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:992<br class="gmail_msg">
+    C2<br class="gmail_msg">
+  };<br class="gmail_msg">
<br class="gmail_msg">
----------------<br class="gmail_msg">
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.</blockquote><div class="gmail_msg"><br class="gmail_msg"></div></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">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.</div></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"> </div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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.<br class="gmail_msg"></blockquote></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg"><br class="gmail_msg">I'm thinking something possibly as simple as:<br class="gmail_msg"><br class="gmail_msg">DWARFDebugInfoEntry Entries[6];<br class="gmail_msg">Entries[0].setDepth(0);<br class="gmail_msg">Entries[1].setDepth(1);</div><div class="gmail_msg">Entries[2].setDepth(2);</div><div class="gmail_msg">Entries[3].setDepth(2);<br class="gmail_msg">Entries[4].setDepth(1);<br class="gmail_msg">Entries[5].setDepth(2);<br class="gmail_msg"><br class="gmail_msg">Check:<br class="gmail_msg">Entries[2].getSibling() == Entries[2]</div><div class="gmail_msg">Entries[3].getSibling() == NULL</div><div class="gmail_msg">Entries[1].getParent() == Entries[0]<br><br>That sort of thing.<br><br>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.<br><br>(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)<br><br>[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... <br><br>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?<br><br>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".]<br class="gmail_msg"> </div></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1103<br class="gmail_msg">
+  EXPECT_EQ(C1.getParent().getOffset(), C.getOffset());<br class="gmail_msg">
+  EXPECT_EQ(C2.getParent().getOffset(), C.getOffset());<br class="gmail_msg">
+<br class="gmail_msg">
----------------<br class="gmail_msg">
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.</blockquote><div><br>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?<br> </div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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... </blockquote><div><br>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)<br> </div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.<br class="gmail_msg"></blockquote><div><br>What's the particular interesting thing about that?<br><br>I'd probably cut C2, though - you can cover that with B1/B2.<br> </div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1110<br class="gmail_msg">
+  EXPECT_FALSE(DefaultDie.getFirstChild().isValid());<br class="gmail_msg">
+  EXPECT_FALSE(DefaultDie.getSibling().isValid());<br class="gmail_msg">
+}<br class="gmail_msg">
----------------<br class="gmail_msg">
I can remove it,<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D27995" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D27995</a><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div></div></div>