[PATCH] D28040: Don't store the NULL DIEs in the DIE array in the DWARFUnit.

[Greg Clayton via Phabricator via llvm-commits]

clayborg added a comment.

Converting email only comments to be here with responses.



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugInfoEntry.h:38
+  /// If non-zero this DIE is the last child in a sibling chain.
+  uint32_t IsLastChild : 1;
 
----------------
dblaike commented in email:

> I think you might be misreading the DWARF (& it looks like the DWARF is a bit confusing, as well).
> 
> For starters - in both your examples, the byte_size of the struct is 4 bytes. So it seems storage is shared, or at least that using the same type doesn't improve/reduce the size of the struct - so on that basis I'd probably suggest using the bool type.
> 
> Beyond that, though. The AT_data_member_location is +3, as you say - not +4, so it is still using the top (or bottom - the counting here is a bit weird imho, but seems to work out) byte of the previous 4 byte value.
> 
> Also - we have /lots/ of similar mixed type bitfields in llvm, I think (have a grep around - it looks like it to me, at least) - so if this isn't doing the right thing I'd be /really/ surprised. 

I did misread the DWARF, it is correct. I will switch this to bool.


================
Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:399
+          Offset += 1;
+        }
       } else {
----------------
dblaike said:

> How would we succeed to print the NULL if we aren't printing children? There would be no 'last child' & so we wouldn't print the NULL in that case either.

So if you tell the CU DIE to print itself and have a recurse depth of 1 for DWARF like:

```
0x0: CU
0x1:  A
0x2    A1
0x3:   NULL
0x4:  B
0x5:   B1 
0x6:   NULL
0x7:  NULL
```
you would expect to see:

```
0x0: CU
0x1:  A
0x4:  B
0x7:  NULL
```
> You're right though, we wouldn't print it in 392, but perhaps we should change the condition here from "if (Die->isLastChild())" to "if (Die->hasChildren())" ?
> 

I think the misunderstanding here is that the NULL is actually terminating the current DIEs sibling chain, not the children's sibling chain. If we use "if (Die->hasChildren())" then we would print a NULL after every Die that has children, and since the NULL is for the current DIEs sibling, not the children, that would do the wrong thing:

```
0x0: CU
0x1:  A
0x2:  NULL // these would be incorrect
0x4:  B
0x5:  NULL // these would be incorrect
0x7:  NULL
```
> Also, a test case for an empty child list would be good.

Agreed, I can add it.


================
Comment at: unittests/DebugInfo/DWARF/DWARFDebugInfoTest.cpp:1043
   EXPECT_EQ(B.getParent(), CUDie);
-  EXPECT_EQ(Null.getParent(), CUDie);
+  EXPECT_EQ(C.getParent(), CUDie);
 
----------------
dblaike said:

> Just to be clear - I'm never suggesting there are test cases we "can regress on", just asking about whether the test cases pull their weight - whether they exercise sufficiently distinct/interesting codepaths that make them important tests compared to the existing coverage.
>
> It looks like C is perhaps a superset - it demonstrates we can go down into children, and back up. But, yes, good point that A and B are boundary/corner cases that are probably reasonable to test for separately.
> 
> Please add comments describing what makes each of these paths interesting. (eg: "// verify an immediate parent", "// verify skipping preceding siblings", "// verify skipping preceding niblings" (luls: https://en.wiktionary.org/wiki/nibling ) - those probably aren't the perfect comments, but a rough idea)

Will do.




https://reviews.llvm.org/D28040