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

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
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({
----------------
CarlosAlbertoEnciso wrote:
> probinson wrote:
> > 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 ]]
> Changed.
Thanks for mentioning `std::merge` and a possible model to refactor this method.

If is OK with you, I will differ the factoring for a later patch.
These are the cases to be considered during refactoring.
- Merge instructions for objects with no comdat functions.
- Merge instructions for objects with comdat functions.
- Merge instructions for a fully linked binary.
- Merge instructions when `-ffunction-sections`


================
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);
----------------
probinson wrote:
> Is `TraverseLines` really needed? If it is set to false only when you reach `DebugLines->end()` it seems like an unnecessary complication.
```
    for (LVLine *InstructionLine : InstructionLines) {
      if (TraverseLines) {
        while (Iter != DebugLines->end()) {
          if (InstructionAddress < DebugAddress) {
            ...
          }
        }
        if (Iter == DebugLines->end()) {
          TraverseLines = false;
          DebugLines->push_back(InstructionLine);
        }
      } else {
        DebugLines->push_back(InstructionLine);
      }
    }
```
We are traversing the instructions and comparing the current `InstructionAddress` and `DebugAddress`. When we reach the end of the current `DebugLines`, we should stop any further comparison.
`TraverseLines` will indicate that the remaining `Instructions` must be added to the `DebugLines`.


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

https://reviews.llvm.org/D125783