[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/LVRange.h:48
+// Class to represent a list of range addresses associated with a
+// scope; the addresses are stored in ascending order.
+using LVRangeEntries = std::vector<LVRangeEntry>;
----------------
probinson wrote:
> Can they overlap?
As they are recorded using the IntervalTree, they can overlap.
Added to the comment:


```
// Class to represent a list of range addresses associated with a
// scope; the addresses are stored in ascending order and can overlap.
```


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:224
+  // Calculate coverage factor.
+  void CalculateCoverageFactor() {
+    float CoveragePercentage = 0;
----------------
probinson wrote:
> Why have both `calculateCoverage` and `CalculateCoverageFactor` ?  They appear to do the same thing.
Removed `CalculateCoverageFactor`. Left over from some renaming.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:183
+// Return the original CodeView enum value.
+inline uint16_t updateOperationCode(uint8_t Code) { return 0x1100 | Code; }
+
----------------
probinson wrote:
> `updateOperationCode` implies modifying something, which this function does not do.  `getCVOperationCode` maybe?
Changed to `getCodeViewOperationCode`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVLocation.cpp:623
+void LVLocation::print(LVLocations *Locations, raw_ostream &OS, bool Full) {
+  assert(Locations && "Locations must not be nullptr");
+  // Print the symbol coverage.
----------------
probinson wrote:
> This is okay, but when a pointer parameter must not be null, it's better practice to make it a reference parameter instead.  Then tests for non-null go away.
I see your point. Unfortunately `Locations` is a pointer and can be null.

The caller is already checking for a null pointer.

```
void LVSymbol::printExtra(raw_ostream &OS, bool Full) const {
  ...
    if (Locations)
      // Print location information.
      LVLocation::print(Locations, OS, Full);
  ...
}

```
Change to 
```
void LVSymbol::printExtra(raw_ostream &OS, bool Full) const {
  ...
  // Print location information.
  LVLocation::print(Locations, OS, Full);
  ...
}
```
```
// Print location (formatted version).
void LVLocation::print(LVLocations *Locations, raw_ostream &OS, bool Full) {
  if (!Locations || Locations->empty())
    return;

  // Print the symbol coverage.
  if (options().getAttributeCoverage()) {
    ...
  }

  // Print the symbol location, including the missing entries.
  if (getReader().doPrintLocation(/*Location=*/nullptr))
    ...
}
```
The assertion is not needed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVLocation.cpp:626
+  if (Locations && !Locations->empty() && options().getAttributeCoverage()) {
+    assert(!Locations->empty() && "Locations must have at least one element");
+    // The location entries are contained within a symbol. Get a location,
----------------
probinson wrote:
> The `if` above already checked `!Locations->empty()` so this assertion is redundant.
Removed assertion.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVRange.cpp:26
+
+  LLVM_DEBUG({ dbgs() << "\nRanges Tree:\n"; });
+
----------------
probinson wrote:
> The same heading is here and below where the entire resulting tree is printed. Maybe these should be different?  "Range Tree entries" here, and leave "Ranges Tree" for when the whole tree is printed.
Good point. Changed to your suggested heading.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSymbol.cpp:190
+      // lower address.
+      // The symbol can have a set of non-continuos locations. We are using
+      // only the first location entry to get the outermost parent.
----------------
probinson wrote:
> 
Changed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSymbol.cpp:203
+        CoverageParent
+            ? rint((double(CoverageFactor) / CoverageParent) * 100.0 * 100.0) /
+                  100.0
----------------
probinson wrote:
> Same comment as the other place that made this same calculation, in a previous patch.
> It solved the issue of differences in the percentage when analyzing the same binary on Windows and Linux. For some cases:
> 
> ```
> Expect value:
> Totals by lexical level:
> [001]:        106 (100.00%)
> [002]:         71 ( 66.99%)    <---
> [003]:         20 ( 18.87%)
> ```
> 
> ```
> Actual value
> Totals by lexical level:
> [001]:        106 (100.00%)
> [002]:         71 ( 66.98%)    <---
> [003]:         20 ( 18.87%)
> ```
> 




================
Comment at: llvm/unittests/DebugInfo/LogicalView/LocationRangesTest.cpp:58
+    T *Element = new (std::nothrow) T();
+    EXPECT_NE(Element, nullptr);
+    return Element;
----------------
probinson wrote:
> Use `ASSERT_NE` so the test will abort if the allocation fails.
`ASSERT_NE` can be used only inside functions that don't return a value.
 
See https://google.github.io/googletest/advanced.html#assertion-placement

> The one constraint is that assertions that generate a fatal failure (FAIL* and ASSERT_*) can only be used in void-returning functions.




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

https://reviews.llvm.org/D125779