[PATCH] D125779: [llvm-dva] 04 - Locations and ranges

Thu Jun 30 03:58:27 PDT 2022

CarlosAlbertoEnciso added inline comments.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVLocation.h:42
+  LVOperation &operator=(LVOperation const &) = delete;
+  ~LVOperation() = default;
+
----------------
psamolysov wrote:
> Either the class should be marked as `final` or the destructor should be virtual. 
Marked as `final`.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVLocation.h:83
+  bool hasAssociatedRange() const {
+    return (getIsClassOffset() || getIsDiscardedRange()) ? false : true;
+  }
----------------
psamolysov wrote:
> 
Good simplification. Changed.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVLocation.h:128
+  // Line numbers for locations.
+  LVLine *getLowerLine() const { return LowerLine; }
+  void setLowerLine(LVLine *Line) { LowerLine = Line; }
----------------
psamolysov wrote:
> Should this and a couple of const methods below return non-const pointers?
The `getLowerLine` and `getUpperLine` now return a `const` pointer.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVLocation.h:163-168
+  virtual void print(raw_ostream &OS, bool Full = true) const override;
+  virtual void printExtra(raw_ostream &OS, bool Full = true) const override;
+
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+  virtual void dump() const override { print(dbgs()); }
+#endif
----------------
psamolysov wrote:
> Either `virtual` or `override` is enough.
Removed `virtual`.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVRange.h:25
+
+class LVRangeEntry {
+  LVAddress Lower = 0;
----------------
psamolysov wrote:
> The class should be marked as `final` or have a virtual destructor (or protected destructor if you have no intent to access the derived from `LVRangeEntry` classes by pointers to the basic class).
Marked as `final`.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:120
+  void printActiveRanges(raw_ostream &OS, bool Full = true) const {
+    (const_cast<LVScope *>(this))->printActiveRanges(OS, Full);
+  }
----------------
psamolysov wrote:
> This cast of a const pointer to a non-const, so casting away constness may cause undefined behavior if you write/change something in the non-const function member. If you don't (it's not clear now because not all the involved functions are implemented yet), it could make sense to move the actual implementation of the function member into the `const` version and cast `this` to `const` (or use it as is, I believe it should compile too).
> 
> Also I saw this pattern a few times in the previous patch: https://reviews.llvm.org/D125778
The implementation of `printActiveRanges` it is not changed at later patches.

The `const` version is only required when it is called by other const functions:

```
void LVScopeNamespace::printExtra(raw_ostream &OS, bool Full) const {
  ...
  // Print any active ranges.
  if (Full) {
    printActiveRanges(OS, Full);
    ...
  }
}
```

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVLocation.cpp:323
+  //-------------------------------------------------------------------------
+  case 0:
+    Stream << "offset " << int(Operands[0]);
----------------
psamolysov wrote:
> Is there any enum in the `dwarf` namespace which is equals to zero? If not it could make sense to use a named constant instead of magic number `0`.
@probinson How do you feel in creating something like dwarf::DW_OP_null similar to the existing dwarf::DW_TAG_null.

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVLocation.cpp:622
+// Print location (formatted version).
+void LVLocation::print(LVLocations *Locations, raw_ostream &OS, bool Full) {
+  // Print the symbol coverage.
----------------
psamolysov wrote:
> 
Added the assertion.

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVLocation.cpp:624
+  // Print the symbol coverage.
+  if (options().getAttributeCoverage()) {
+    // The location entries are contained within a symbol. Get a location,
----------------
psamolysov wrote:
> 
Added the assertion. Small correction: `!Locations->empty()`

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVLocation.cpp:627
+    // to access basic information about indentation, parent, etc.
+    LVLocation *Location = Locations->front();
+    LVSymbol *Symbol = Location->getParentSymbol();
----------------
psamolysov wrote:
> There is not check that `Locations` is not empty.
Added the following checks to cover both cases: null ptr and/or empty.

```
  ...
  assert(Locations && "Locations must not be nullptr");
  // Print the symbol coverage.
  if (Locations && !Locations->empty() && options().getAttributeCoverage()) {
  ..
```

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVLocation.cpp:645
+  if (getReader().doPrintLocation(/*Location=*/nullptr))
+    for (const LVLocation *Location : *Locations)
+      Location->print(OS, Full);
----------------
psamolysov wrote:
> There is no check that `Locations` is not null.
Added the following check:

```
  if (Locations && getReader().doPrintLocation(/*Location=*/nullptr))
```

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVRange.cpp:27
+  Scope->setLevel(Level);
+  return std::make_tuple(Low, High, Scope);
+}
----------------
psamolysov wrote:
> I'm not sure who will be the owner of the heap-allocated `Scope` and who will delete it. Maybe a comment is required.
That is a good finding. 

```
using LVTuple = std::tuple<LVAddress, LVAddress, LVScope *>;
LVTuple getEntry(LVAddress Low, LVAddress High, LVLevel Level) {
  LVScope *Scope = new LVScope();
  Scope->setLevel(Level);
  return std::make_tuple(Low, High, Scope);
}
```
That code is leftovers. Removed all together.

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVRange.cpp:36
+  // Traverse the ranges and store them into the interval tree.
+  if (RangeEntries.size())
+    for (LVRangeEntry &RangeEntry : RangeEntries) {
----------------
psamolysov wrote:
> This check is not required, if `RageEntries` is empty, the loop below will just do no iterations.
Removed the check.

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVRange.cpp:74
+  // Just add the scope and range pair, in no particular order.
+  RangeEntries.emplace_back(LVRangeEntry(LowerAddress, UpperAddress, Scope));
+}
----------------
psamolysov wrote:
> This should create a `RangeEntry` in place.
Replaced.

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVRange.cpp:77
+
+void LVRange::addEntry(LVScope *Scope) {
+  // Traverse the ranges and update the ranges set only if the ranges
----------------
psamolysov wrote:
> 
Added assertion.

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:136-137
+void LVScope::addObject(LVLocation *Location) {
+  assert(!Location->getParent() && "Location already inserted");
+  assert(Location && "Invalid location.");
+  if (!Ranges)
----------------
psamolysov wrote:
> These two lines should be swapped.
Done.

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSort.cpp:59
+  if (LHS->getLowerAddress() == RHS->getLowerAddress())
+    return (LHS->getUpperAddress() < RHS->getUpperAddress());
+
----------------
psamolysov wrote:
> I'm not sure these wrapping `()` are required.
Removed the wrapping `()`.

================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSymbol.cpp:173
+
+void LVSymbol::getLocations(LVLocations &LocationList) {
+  if (!Locations)
----------------
psamolysov wrote:
> This method looks like it can be marked as `const` if you have no plan to add a call to `calculateCoverage()` or to another method that changes the object.
Marked the method as `const`.

================
Comment at: llvm/unittests/DebugInfo/LogicalView/LocationRangesTest.cpp:58
+    T *Element = new T();
+    EXPECT_NE(Element, nullptr);
+    return Element;
----------------
psamolysov wrote:
> A `std::bad_alloc` will be thrown if the test is out of memory. It makes sense to wrap the line in the `try-catch` block or use the non-throwing overload of the `new` operator.
Changed to use the non-throwing overload.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125779