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

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:122
+  void printActiveRanges(raw_ostream &OS, bool Full = true) const {
+    (const_cast<LVScope *>(this))->printActiveRanges(OS, Full);
+  }
----------------
probinson wrote:
> I think I said something about this in a previous patch, that a non-const print function is counter-intuitive.  If it can be designed not to need a non-const version, that would be better.
Yes. You mentioned in the previous patch:

> Looks like the method shouldn't be declared const. Ordinarily you wouldn't think a print() method would be non-const, but obviously this one is.

Removed the `non-const` version. Changed `printActiveRanges` to be `const` as it does not modify any class members.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVRange.cpp:131
+                              const LVRangeEntry &rhs) -> bool {
+    return (lhs.lower() < rhs.lower());
+  };
----------------
probinson wrote:
> I see the caller is `stable_sort` so is there an ordering guarantee for multiple ranges with the same lower bound?  If so, where is that documented and enforced?
Very good point. Added code to handle that case:

```
  auto CompareRangeEntry = [](const LVRangeEntry &lhs,
                              const LVRangeEntry &rhs) -> bool {
    if (lhs.lower() < rhs.lower())
      return true;

    // If the lower address is the same, use the upper address value in
    // order to put first the smallest interval.
    if (lhs.lower() == rhs.lower())
      return lhs.upper() < rhs.upper();

    return false;
  };

```


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:712
+// Get all the locations associated with symbols.
+void LVScope::getLocations(LVLocations &LocationList,
+                           LVValidLocation ValidLocation, bool RecordInvalid) {
----------------
probinson wrote:
> "get" usually implies a method that is returning data of interest, but these are all doing something else.
It seems that is the same issue as with the `getRanges` method.

Added explanatory comments in `getLocations` to describe the functions performed by this method.

```
// Traverse the symbol location ranges and for each range:
// - Apply the 'ValidLocation' validation criteria.
// - Add any failed range to the 'LocationList'.
// - Calculate location coverage.
void LVScope::getLocations(LVLocations &LocationList,
                           LVValidLocation ValidLocation, bool RecordInvalid) {
  ...
}

```


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:762
+LVScope *LVScope::outermostParent(LVAddress Address) {
+  LVScope *Parent = this;
+  while (Parent) {
----------------
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;
}

```



================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:942
+    bool RecordInvalid = false;
+    getRanges(Locations, ValidLocation, RecordInvalid);
+  }
----------------
probinson wrote:
> It looks odd to call something named `getRanges` using local variables that are immediately thrown away.  This is essentially the same comment about the names of the `get` methods I made previously.
You are correct as the local `Locations` is  immediately thrown away.

What the `getRanges` method does is:
- traverse the scope ranges
- apply a validation criteria.
- add a failed range to the `Locations`
- calculate coverage.

It was hard to split `getRanges` between this patch and the `06-warning-internals` patch and avoid big changes.

In summary:
For this patch the only needed functionality in `getRanges` is to calculate the location coverage. As `RecordInvalid = false` the `getRanges` method will not update the local `Locations` variable.

Added explanatory comments in `getRanges` and `getLocations` to describe the functions performed by those methods.


```
// Traverse the scope ranges and for each range:
// - Apply the 'ValidLocation' validation criteria.
// - Add any failed range to the 'LocationList'.
// - Calculate location coverage.
void LVScope::getRanges(LVLocations &LocationList,
                        LVValidLocation ValidLocation, bool RecordInvalid) {
  ...
}

```


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

https://reviews.llvm.org/D125779