[PATCH] D125784: [llvm-debuginfo-analyzer] 09 - CodeView Reader

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSupport.cpp:112
+    if (InTemplate && !AngleCount)
+      InTemplate = false;
+  }
----------------
probinson wrote:
> I'd eliminate the `InTemplate` variable and check `AngleCount` instead, but up to you.
That is a good suggestion. Eliminated `InTemplate` and `CharsSeen`.

`warning: variable 'CharsSeen' set but not used [-Wunused-but-set-variable]`


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:1622
+    if (DemangledSymbol.find("scalar deleting dtor") != std::string::npos)
+      Function->setIsArtificial();
+  }
----------------
zequanwu wrote:
> Clang generated global ctor and dtor names containing the substrings: `dynamic initializer for` and `dynamic atexit destructor for`: https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGDebugInfo.cpp#L2254-L2259
Thanks for the information. Adding an extra check for `dynamic atexit destructor for`.
```
    // We don't have a way to see if the symbol is compiler generated. Use
    // the linkage name, to detect `scalar deleting destructor' functions.
    std::string DemangledSymbol = demangle(std::string(LinkageName));
    if (DemangledSymbol.find("scalar deleting dtor") != std::string::npos) {
      Function->setIsArtificial();
    } else {
      // Clang generates global ctor and dtor names containing the substrings:
      // 'dynamic initializer for' and 'dynamic atexit destructor for'.
      if (DemangledSymbol.find("dynamic atexit destructor for") !=
          std::string::npos)
        Function->setIsArtificial();
    }
```



================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:1900
+                : getSizeInBytesForTypeIndex(TIElementType))
+      PrevSubrange->setCount(PrevSubrange->getCount() / Size);
+
----------------
zequanwu wrote:
> Might worth checking `Size != 0`, I have seen that element size being 0, might be missing in pdb.
`Size` is already checked in the `if` condition.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.cpp:1400
+    LVScope *AbstractFunction = new LVScopeFunction();
+    if (AbstractFunction) {
+      AbstractFunction->setIsSubprogram();
----------------
probinson wrote:
> Usually `new` is assumed to return non-null, why check it just in this one place?
That is leftovers from another change. Removed the `non-null` check.


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

https://reviews.llvm.org/D125784