[PATCH] D39185: [llvm-dwarfdump] - Fix array out of bounds access crash.

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Thanks everyone for comments ! I plan to address them tomorrow, few quick answers are below.



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:45
   DWARFUnit *U = nullptr;
-  const DWARFDebugInfoEntry *Die = nullptr;
+  ArrayRef<DWARFDebugInfoEntry> Die;
 
----------------
aprantl wrote:
> Can you add a comment explaining why the Die member is an array and what the bounds of the array are representing? I assume that the last member of the array is the last children of the DIE?
No, last member is the las DIE of unit, just like in original code.
I'll revisit this place. May be we can just  have array of children here.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:115
+    if (isValid() && Die[0].hasChildren())
+      return DWARFDie(U, Die.drop_front(1));
     return DWARFDie();
----------------
dblaikie wrote:
> This is probably incorrect - in the sense that it'll produce a DWARFDie that has a longer array that contains more than its own children.
> 
> Imagine this tree of DIEs:
> 
>   A
>     B
>     C
> 
> If you ask A for its first child, you'd get a DWARFDie containing {B, C} and then if you asked B for its first child, you could end up with C - even though C is a sibling of B, not a child.
> 
> I guess that bug already existed, but still... 
> you'd get a DWARFDie containing {B, C} and then if you asked B for its first child, you could end up with C

It should not happen. Because Die[0].hasChildren() takes information about children from abbreviation table,
so for that case it will not find DW_CHILDREN_yes there and will return empty DWARFDie as expected.

I'll try to improve this place though.


================
Comment at: tools/llvm-dwarfdump/llvm-dwarfdump.cpp:284
+  for (const auto &CU : CUs) {
+    for (unsigned I = 0, E = CU->getNumDIEs(); I != E; ++I) {
+      DWARFDie Die = CU->getDIEAtIndex(I);
----------------
aprantl wrote:
> why does this break the iterator?
It does not break it.
Problem is that CU->dies() iterates over array of DWARFDebugInfoEntry. 
Previous code just used pointer as argument:

```
DWARFDie Die = {CU.get(), &Entry};
```
But now I need to pass ArrayRef of them to DWARFDie constructor, I need to find array size
or take DWARFDie that is already exist.


https://reviews.llvm.org/D39185