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

Pavel Samolysov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu May 26 08:08:09 PDT 2022


psamolysov added a comment.

Good job! Here is a series of comments, please have a look.



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


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



================
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; }
----------------
Should this and a couple of const methods below return non-const pointers?


================
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
----------------
Either `virtual` or `override` is enough.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVRange.h:25
+
+class LVRangeEntry {
+  LVAddress Lower = 0;
----------------
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).


================
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);
+  }
----------------
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


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVLocation.cpp:323
+  //-------------------------------------------------------------------------
+  case 0:
+    Stream << "offset " << int(Operands[0]);
----------------
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`.


================
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.
----------------



================
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,
----------------



================
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();
----------------
There is not check that `Locations` is not empty.


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


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


================
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) {
----------------
This check is not required, if `RageEntries` is empty, the loop below will just do no iterations.


================
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));
+}
----------------
This should create a `RangeEntry` in place.


================
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
----------------



================
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)
----------------
These two lines should be swapped.


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


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSymbol.cpp:173
+
+void LVSymbol::getLocations(LVLocations &LocationList) {
+  if (!Locations)
----------------
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.


================
Comment at: llvm/unittests/DebugInfo/LogicalView/LocationRangesTest.cpp:94
+  Element->setType(Type);
+  EXPECT_STREQ(Element->getName().data(), Name.data());
+  EXPECT_EQ(Element->getOffset(), Offset);
----------------
There is an overload of the `operator==` for two `StringRef`s, so there is no needs in `EXPECT_STREQ`, moreover the `data()` method of `StringRef` doesn't guarantee that the returned string will be null terminated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125779



More information about the llvm-commits mailing list