[PATCH] D125778: [llvm-debuginfo-analyzer] 03 - Logical elements

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVLine.h:84
+  void print(raw_ostream &OS, bool Full = true) const override;
+  void printExtra(raw_ostream &OS, bool Full = true) const override{};
+
----------------
probinson wrote:
> 
Changed.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h:155
+  // The parent of this object (nullptr if the root scope). For locations,
+  // the parent it is a symbol object; otherwise is a scope object.
+  union {
----------------
probinson wrote:
> 
Changed.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:108
+  virtual void printSizes(raw_ostream &OS) const {};
+  virtual void printSummary(raw_ostream &OS) const {};
+
----------------
probinson wrote:
> No `;` after the `{}` 
Changed.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:236
+  void printExtra(raw_ostream &OS, bool Full = true) const override;
+  virtual void printMatchedElements(raw_ostream &OS, bool UseMatchedElements){};
+
----------------
probinson wrote:
> 
Changed.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:518
+
+// Class to represent a DWARF subroutine function.
+class LVScopeFunctionType final : public LVScopeFunction {
----------------
probinson wrote:
> Did you mean "subroutine type"?
Yes. Changed to `// Class to represent a DWARF subroutine type.`


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:552
+
+class LVScopeRoot final : public LVScope {
+  size_t FileFormatNameIndex = 0;
----------------
probinson wrote:
> Needs comment saying what does LVScopeRoot represent.
Added `// Class to represent the binary file being analyzed.`



================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:39
+    for (auto *Item : *this)
+      delete Item;
+  }
----------------
probinson wrote:
> This appears to be assuming that the vector elements are pointers?  And the memory for the pointed-to objects is owned by the vector?  I'd think it would be simpler to have a SmallVector of objects, not pointers-to-objects, in which case SmallVector takes care of destroying them for you.
> (Or, use a SmallVector of smart pointers.)
> 
> If you really want a SmallVector of normal pointers, then the template needs to guarantee that T is a pointer type, somehow.  But this all feels rather fragile.
The `LVAutoSmallVector` is used to record the created logical elements (`LVScope`, `LVSymbol`, etc) which currently are created by `new` and to do the automatic destroy.

Once the patches are accepted, one the tasks is to use Small pointers across all patches and `LVAutoSmallVector` will be replaced with SmallVector of smart pointers.

Adding the check to guarantee that `T` is a pointer type.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:51
+  void set(T Idx) {
+    assert(static_cast<unsigned>(Idx) < static_cast<unsigned>(T::LastEntry) &&
+           "Invalid property");
----------------
probinson wrote:
> I think the `static_cast<unsigned>` should be unnecessary? because Idx has type T, therefore `Idx < T::LastEntry` should work correctly.
> Same comment in the other methods.
> 
> Actually, you could defer the bounds checking to SmallBitVector's `operator[]` and remove a lot of this code.  For example
> ```
> void set(T Idx) {
>   Bits[static_cast<unsigned>(Idx)] = 1;
> }
> ```
Nice code reduction. Changed to use `operator[]`.



================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:64
+    assert(static_cast<unsigned>(Idx) < static_cast<unsigned>(T::LastEntry) &&
+           "Invalid property");
+    return Bits[static_cast<unsigned>(Idx)];
----------------
probinson wrote:
> This assert is redundant, `SmallBitVector::operator[]` does the same assertion.
Nice code reduction. Changed to use `operator[]` for the `reset` method as well.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSymbol.h:88
+    assert((!Element || (Element && static_cast<LVSymbol *>(Element))) &&
+           "Invalid element");
+    setReference(static_cast<LVSymbol *>(Element));
----------------
probinson wrote:
> This assertion looks a little strange.  I think it's trying to verify that Element is either null, or is-a LVSymbol.  In that case the assert would be
> `assert((!Element || dyn_cast<LVSymbol *>(Element)) && "Invalid element");`
Good point.
I think the correct form is:
`assert((!Element || dyn_cast<LVSymbol>(Element)) && "Invalid element");`

Changed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVElement.cpp:28
+             : nullptr;
+}
+
----------------
probinson wrote:
> If the LVElement hierarchy is set up for LLVM-style casting, then this could be just `return dyn_cast<LVType *>(ElementType);`
I see your point. But in this case we are not relying on the LVElement hierarchy.

```
class LVElement {
  PROPERTY(Property, IsType);
  // Type of this element.
  LVElement *ElementType = nullptr;
};

```
We are returning the `ElementType` that can be a `LVType` or a `LVScope` and it is determined by the `PROPERTY IsType/IsScope`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVElement.cpp:33
+             ? static_cast<LVScope *>(ElementType)
+             : nullptr;
+}
----------------
probinson wrote:
> If the LVElement hierarchy is set up for LLVM-style casting, then this could be just `return dyn_cast<LVScope *>(ElementType);`
Same as above comments.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVElement.cpp:97
+void LVElement::setFilename(StringRef Filename) {
+  // Get index for the flatted out filename.
+  FilenameIndex = getStringIndex(Filename);
----------------
probinson wrote:
> 
Changed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVLine.cpp:40
+const char *LVLine::kind() const {
+  static const char *Kind = KindUndefined;
+  if (getIsLineDebug())
----------------
probinson wrote:
> `Kind` doesn't need to be `static` here.
Removed `static`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:56
+const char *LVScope::kind() const {
+  static const char *Kind = KindUndefined;
+  if (getIsArray())
----------------
probinson wrote:
> `Kind` doesn't need to be `static` here.
Removed `static`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:411
+StringRef LVScope::resolveReferencesChain() {
+  // If the scope have a DW_AT_specification or DW_AT_abstract_origin,
+  // follow the chain to resolve the name from those references.
----------------
probinson wrote:
> 
Changed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:638
+  while (Parent) {
+    // Terminates if the 'SetFunction' have been already executed.
+    if ((Parent->*GetFunction)())
----------------
probinson wrote:
> 
Changed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:652
+    while (Parent) {
+      // Terminates if the 'SetFunction' have been already executed.
+      if ((Parent->*GetFunction)())
----------------
probinson wrote:
> 
Changed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:930
+    float Percentage =
+        rint((float(Size) / CUContributionSize) * 100.0 * 100.0) / 100.0;
+    OS << format("%10d (%6.2f%%) : ", Size, Percentage);
----------------
probinson wrote:
> Did this solve a real problem?  I would expect this expression to be folded to
> `rint((float(Size) / CUContributionSize) * 100.0)`
> but maybe my notion of floating-point constant folding is too naive.
> 
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/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1086
+  const_cast<LVScopeCompileUnit *>(this)->Found.reset();
+  const_cast<LVScopeCompileUnit *>(this)->Printed.reset();
+
----------------
probinson wrote:
> 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.
All the `print` methods for the logical elements are marked as `const`. Only this one requires to modify some class members (stats).


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:1145
+        if (Scope->getHasReferenceAbstract() && !Scope->getAddedMissing())
+          // Add missing elements at the function nested scopes.
+          Scope->addMissingElements(Scope->getReference());
----------------
probinson wrote:
> This looks like it just repeats the comment from a few lines above.
Removed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSupport.cpp:23
+
+// Transforms '\\' into '/' (Platform independent).
+std::string llvm::logicalview::transformPath(StringRef Path) {
----------------
probinson wrote:
> There should be existing functions in Support for this.
The only similar function in `Support` is `Path.cpp::native`

But we need additional changes to the given `Path`.
Added the comments for further clarification:

```
// Perform the following transformations to the given 'Path':
// - all characters to lowercase.
// - '\\' into '/' (Platform independent).
// - '//' into '/' 
```



================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSymbol.cpp:35
+const char *LVSymbol::kind() const {
+  static const char *Kind = KindUndefined;
+  if (getIsCallSiteParameter())
----------------
probinson wrote:
> `Kind` doesn't need to be `static` here.
Removed `static`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp:48
+const char *LVType::kind() const {
+  static const char *Kind = KindUndefined;
+  if (getIsBase())
----------------
probinson wrote:
> `Kind` doesn't need to be `static` here.
Removed `static`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp:90
+  // any reference tags, such as DW_AT_specification, DW_AT_abstract_origin,
+  // DW_AT_extension.
+
----------------
probinson wrote:
> This comment might belong in another patch? It doesn't seem to describe anything in this method.
The comment is intended to explain that a `LVType` does not have references to any DW_AT_specification, DW_AT_abstract_origin, DW_AT_extension, etc.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp:134
+  // In the case of unnamed types, try to generate a name for it, using
+  // the parents name and the line information. Ignore the template
+  if (!isNamed() && !getIsTemplateParam())
----------------
probinson wrote:
> Ignore the template parameters. (?)
Correct. Changed to `Ignore the template parameters.`


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp:273
+      } else
+        Name.append(std::string(ArgType->getName()));
+    } else {
----------------
probinson wrote:
> Even though it's a one-line else clause, please put `{}` around it so that the next line doesn't look like incorrect indentation.
Added `{}`.


================
Comment at: llvm/unittests/DebugInfo/LogicalView/LogicalElementsTest.cpp:65
+    T *Element = new (std::nothrow) T();
+    EXPECT_NE(Element, nullptr);
+    return Element;
----------------
probinson wrote:
> I think `ASSERT_NE` here would be better; bail out of the test in a controlled way if the `new` returns nullptr.  Otherwise you'll just get a segfault somewhere later that will be harder to diagnose.
`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.




================
Comment at: llvm/unittests/DebugInfo/LogicalView/LogicalElementsTest.cpp:111
+  Root = getScopesRoot();
+  EXPECT_NE(Root, nullptr);
+
----------------
probinson wrote:
> I think `ASSERT_NE` here too.
`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/D125778/new/

https://reviews.llvm.org/D125778