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

Wed Sep 21 09:55:36 PDT 2022

probinson added a comment.

Done with code files, still need to do tests.  But my laptop is about to have a forced reboot so I am submitting these comments now.

================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVELFReader.h:43
+  bool RangesDataAvailable = false;
+  DWARFDie DummyDie;
+  LVAddress CUBaseAddress = 0;
----------------
DummyDie is a placeholder used in only two places. It should be a local variable in those places, not a class member.

================
Comment at: llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:94
+                             "Binary object format in '%s' is not supported.",
+                             TheFilename.c_str());
+  }
----------------
`StringRef` has a `str()` method that returns `std::string` so you can eliminate the local variable `TheFilename`.

================
Comment at: llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:107
+  if (BuffOrErr.getError()) {
+    std::string TheFilename(ConvertedPath);
+    return createStringError(errc::bad_file_descriptor,
----------------
ConvertedPath is already a std::string, no need to make a copy.

================
Comment at: llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:154
+
+  return Error::success();
+}
----------------
Unexpected file type is success?

================
Comment at: llvm/lib/DebugInfo/LogicalView/LVReaderHandler.cpp:172
+  for (LVReader *Reader : TheReaders)
+    if (options().getPrintExecute())
+      if (Error Err = Reader->doPrint())
----------------
The `for` and first `if` can be swapped.

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

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

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

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

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

================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:512
+  // equal or greater that the line address and less than the address of the
+  // next line record.
+  LLVM_DEBUG({
----------------

================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:512
+  // equal or greater that the line address and less than the address of the
+  // next line record.
+  LLVM_DEBUG({
----------------
probinson wrote:
> 
Have you thought about using `std::merge` to do this merging of two sorted containers?  If that doesn't work, maybe refactor the three nested loops modeled on the example merge code sketched out [[ https://en.cppreference.com/w/cpp/algorithm/merge | here ]]

================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:601
+          // instruction line address is greater than the last source line.
+          TraverseLines = false;
+          DebugLines->push_back(InstructionLine);
----------------
Is `TraverseLines` really needed? If it is set to false only when you reach `DebugLines->end()` it seems like an unnecessary complication.

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

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

================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:48
+    case dwarf::DW_TAG_GNU_call_site_parameter:
+      return CurrentSymbol;
+    default:
----------------
Isn't `CurrentSymbol` currently `nullptr` ?

================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:440
+    if (RangesDataAvailable && options().getGeneralCollectRanges()) {
+      auto GetRanges = [&](const DWARFFormValue &FormValue,
+                           DWARFUnit *U) -> Expected<DWARFAddressRangesVector> {
----------------
FormValue and U are parameters, the lambda is using nothing else.

================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:461
+        // This seems to be a tombstone for empty ranges.
+        if (Range.LowPC == 1 && Range.HighPC == 1)
+          continue;
----------------
Any time lowpc == highpc it is an empty range.  Linkers might fill in 1 for code that gets stripped, or they might fill in something else.

================
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()) {
----------------
overrite should be override? or overwrite?

================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:692
+    if (options().getPrintSizes() && Upper)
+      CompileUnit->addSize(Scope, Lower, Upper);
+  }
----------------
It looks like Upper will not include the size of the last child?

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

================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:795
+                             "Could not create DWARF information: %s",
+                             TheFilename.c_str());
+  }
----------------
If `getFilename()` returns a StringRef, you can do `getFilename().str().c_str()` and then don't need TheFilename.

================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:862
+    // same, do not perform any adjustment.
+    auto DeductIncrementFileIndex = [&]() -> bool {
+      if (CU->getVersion() < 5)
----------------
"Deduct" means "subtract" which is not what you mean here.

================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:958
+// Get the location information for the associated attribute.
+void LVELFReader::getLocationList(dwarf::Attribute Attr,
+                                  const DWARFFormValue &FormValue,
----------------
This method doesn't "get" information in the usual sense (return it to the caller).
Maybe `processLocationList` ?  Consistent with the lambda names in the implementation.

================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:1049
+
+void LVELFReader::getLocationMember(dwarf::Attribute Attr,
+                                    const DWARFFormValue &FormValue,
----------------
Maybe `processMemberLocation` ?

================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:1169
+    // the symbol may error trying to load a section that does not exist.
+    const MachOObjectFile *MachO = dyn_cast<const MachOObjectFile>(&Obj);
+    bool IsSTAB = false;
----------------
LVELFReader is used for MachO?  I guess the differences are minimal because they both use DWARF.  But probably should mention this in the comment at the top of the file.

================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:1185
+    }
+    section_iterator Section = IsSTAB ? Obj.section_end() : *IterOrErr;
+    if (Section == Obj.section_end())
----------------
Is there a missing TODO here?  The comment above talks about "unless you know" which implies there are cases where we could know.  But this line is basically
`if (IsSTAB) continue;`
which could have been sooner, and simplify this bit 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.
----------------

================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:1216
+    // - Symbol has the SF_Weak or
+    // - Symbol section index different of the DotTextSectionIndex.
+    LVSectionIndex SectionIndex = Section->getIndex();
----------------
This implies that the tool can handle only linked executables?  Not relocatable objects?  maybe I knew that...  but this requirement means it won't work for relocatable files compiled with `-ffunction-sections`.

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

https://reviews.llvm.org/D125783