[PATCH] D49173: [DebugInfo] Make children iterator bidirectional

David Blaikie via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jul 11 09:49:50 PDT 2018


dblaikie accepted this revision.
dblaikie added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:595-599
+  // Find the previous DIE whose depth is the same as the Die's depth.
+  for (size_t I = getDIEIndex(Die) - 1; I >= 0; --I) {
+    if (DieArray[I].getDepth() == Depth)
+      return DWARFDie(this, &DieArray[I]);
+  }
----------------
JDevlieghere wrote:
> dblaikie wrote:
> > Unlike get(Next)Sibling, this loop may need to check for "<= Depth" and then return DWARFDie() if it's the < case.
> > 
> > When going forwards, there's always a null die sibling to break this loop (well, I guess a null die is returned from getSibling - and then no caller tries to call getSibling on the null die) - because it has the same depth.
> > 
> > But going backwards there's no null die to break the loop - oh, well, kind of.
> > 
> > So if you've got this arrangement: Unit{A{B,NULL},C{D,NULL},NULL}
> > 
> > And at D you "get previous" then you'll return the NULL child of A... which is still a null child, and that works I suppose. But you could've stopped searching once you saw C - because you've stepped out of C's children list by going to a shallower depth than the depth of D who's sibling you were searching for.
> > 
> > 
> > 
> > Oh... and it's undefined to dereference a begin iterator anyway, so none of that really matters, I suppose? :)
> Indeed. I added the check at some point but looks like it didn't make it into the patch. Even if it doesn't matter for the iterator (I assume you mean dereferencing the end iterator?) the check is cheap so we might as well make the interface behave as you'd expect. 
Yeah - if it were just the iterator API I'd say maybe make it an assert - but since this is a "getPrevSibling" it makes sense to me that it's not UB to call it on the first child, and for it to be symmetric with "get(Next)Sibling" in terms of returning null on the first/last element.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:626
+      return DWARFDie(this, &DieArray[I]);
+  }
+  return DWARFDie();
----------------
Maybe assert that DieArray[I].getDepth() > Depth - to ensure it didn't some how jump out of the children?


https://reviews.llvm.org/D49173





More information about the llvm-commits mailing list