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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:55
   explicit operator bool() const { return isValid(); }
-  const DWARFDebugInfoEntry *getDebugInfoEntry() const { return Die; }
+  const DWARFDebugInfoEntry *getDebugInfoEntry() const { return Die.data(); }
   DWARFUnit *getDwarfUnit() const { return U; }
----------------
This is probably better written as:

  return &Die[0]

Though I guess this returns a pointer because it can be/is called on invalid DWARFDies? (in which case the above code would be invalid - and maybe we should assert that this isn't called on invalid DWARFDies? & maybe have it return a reference instead of a pointer if that's possible)


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDie.h:115
+    if (isValid() && Die[0].hasChildren())
+      return DWARFDie(U, Die.drop_front(1));
     return DWARFDie();
----------------
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... 


================
Comment at: lib/DebugInfo/DWARF/DWARFDie.cpp:504
   if (isValid())
-    return U->getParent(Die);
+    return U->getParent(&Die.front());
   return DWARFDie();
----------------
Probably worth being consistent about using either Die.front() or Die[0] across this change. I'm not too fussed about which, really. I guess I lean slightly towards front(), though accept it's more verbose..


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:428
     if (DieArray[I].getDepth() == ParentDepth)
-      return DWARFDie(this, &DieArray[I]);
+      return DWARFDie(this, {DieArray.data() + I, DieArray.size() - I});
   }
----------------
I'd probably write DieArray.data() + I as &DieArray[I] - assuming I is in bounds?


https://reviews.llvm.org/D39185