[PATCH] D125783: [llvm-debuginfo-analyzer] 08 - ELF Reader

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/LVReaderHandler.h:40
+//
+// The supported binary formats are: ELF, MacOS and CodeView.
+class LVReaderHandler {
----------------
probinson wrote:
> 
Changed to `Mach-O`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:272
+
+  // Ensure a valid starting address for the public name's.
+  LVSectionAddresses::const_iterator Iter =
----------------
probinson wrote:
> 
Changed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:347
+
+  // There are cases where the section size is smaller that the [LowPC,HighPC]
+  // range; it causes to decode invalid addresses. The recorded size in the
----------------
probinson wrote:
> 
Changed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:348
+  // There are cases where the section size is smaller that the [LowPC,HighPC]
+  // range; it causes to decode invalid addresses. The recorded size in the
+  // logical scope is one less that the real size.
----------------
probinson wrote:
> 
Changed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:349
+  // range; it causes to decode invalid addresses. The recorded size in the
+  // logical scope is one less that the real size.
+  LLVM_DEBUG({
----------------
probinson wrote:
> 
Changed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:413
+      });
+      // The 'processLines()' function will move each created logical line
+      // to its enclosing logical scope, using the debug ranges information
----------------
probinson wrote:
> Maybe add a first line to the comment here, something like
> `// Here we add logical lines to the Instructions.  Later on,`
> and then the comment about 'processLines()' has better context.
Good point. Added your suggested comment.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:623
+
+  // This compilation unit does not have line records. Traverse its scopes
+  // and take any collected instruction lines as the working set in order
----------------
probinson wrote:
> 
Changed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:37
+
+  if (!options().getPrintSymbols()) {
+    switch (Tag) {
----------------
probinson wrote:
> Maybe a comment here?
Added

```
    // As the command line options did not specify a request to print
    // logical symbols (--print=symbols or --print=all or --print=elements),
    // skip its creation.
```


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:48
+    case dwarf::DW_TAG_GNU_call_site_parameter:
+      return CurrentSymbol;
+    default:
----------------
probinson wrote:
> Isn't `CurrentSymbol` currently `nullptr` ?
Yes. To simplify changed to 
```
return nullptr;
```



================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:593
+    // If the input DIE is for a compile unit, process its attributes in
+    // the case of split DWARF, to overrite any common attribute values.
+    if (SkeletonDie.isValid()) {
----------------
probinson wrote:
> overrite should be override? or overwrite?
Changed to `override`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:729
+    for (const DWARFDebugLine::Row &Row : Lines->Rows) {
+      // The 'processLines()' function will move each created logical line
+      // to its enclosing logical scope, using the debug ranges information
----------------
probinson wrote:
> Maybe add a first line to the comment here, something like
> `// Here we collect logical debug lines in CULines.  Later on,`
> and then the comment about 'processLines()' has better context.
Added your suggested comment.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:759
+std::string LVELFReader::getRegisterName(LVSmall Opcode, uint64_t Operands[2]) {
+  // The 'prettyPrintRegisterOp' function uses the DWARFUnit to access the
+  // offset and tag name. At this point we are operating on a logical view
----------------
probinson wrote:
> 
Changed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:760
+  // The 'prettyPrintRegisterOp' function uses the DWARFUnit to access the
+  // offset and tag name. At this point we are operating on a logical view
+  // item, with no access to the underlying DWARF data used by LLVM.
----------------
probinson wrote:
> 
Changed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:762
+  // item, with no access to the underlying DWARF data used by LLVM.
+  // It does not support DW_OP_regval_type.
+  if (Opcode == dwarf::DW_OP_regval_type)
----------------
probinson wrote:
> 
Changed.

```
  // The 'prettyPrintRegisterOp' function uses the DWARFUnit to support
  // DW_OP_regval_type. At this point we are operating on a logical view
  // item, with no access to the underlying DWARF data used by LLVM.
  // We do not support DW_OP_regval_type here.
```



================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:1215
+    // Mark the symbol as 'comdat' in any of the following cases:
+    // - Symbol has the SF_Weak or
+    // - Symbol section index different of the DotTextSectionIndex.
----------------
probinson wrote:
> 
Changed.


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

https://reviews.llvm.org/D125783