[PATCH] D125779: [llvm-debuginfo-analyzer] 04 - Locations and ranges

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

Please update to the most recent patch; some changes are not in the review yet.



================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:762
+LVScope *LVScope::outermostParent(LVAddress Address) {
+  LVScope *Parent = this;
+  while (Parent) {
----------------
CarlosAlbertoEnciso wrote:
> CarlosAlbertoEnciso wrote:
> > probinson wrote:
> > > Looks like if Ranges is null, this ends up returning null.  So, cheaper to do
> > > ```
> > > if (!Ranges)
> > >   return nullptr;
> > > ```
> > > and then remove the `if (Ranges)` from inside the while loop.
> > I think you found an issue with the code.
> > The `Ranges` is not updated when we move scopes. We must use the `Ranges` for the new scope and check if the given `Address` is contained in the specific set.
> > 
> > ```
> > LVScope *LVScope::outermostParent(LVAddress Address) {
> >   LVScope *Parent = this;
> >   while (Parent) {
> >     const LVLocations *ParentRanges = Parent->getRanges();
> >     if (ParentRanges)
> >       for (const LVLocation *Location : *ParentRanges)
> >         if (Location->getLowerAddress() <= Address)
> >           return Parent;
> >     Parent = Parent->getParentScope();
> >   }
> >   return Parent;
> > }
> > 
> > ```
> > 
> The internal regression suite detected 2 cases where the location coverage changed from:
> `0.00 -> 19.05` and `0.00 -> 23.53`.
Please make sure the unittest has coverage for this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125779/new/

https://reviews.llvm.org/D125779