[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/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;
----------------
probinson wrote:
> 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.
Added at the top of the file:
```
// This implements the LVELFReader class.
// It supports ELF and Mach-O formats.
```


================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:1185
+    }
+    section_iterator Section = IsSTAB ? Obj.section_end() : *IterOrErr;
+    if (Section == Obj.section_end())
----------------
probinson wrote:
> 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.
Reprased the comment to:
```
    // In the case of a Mach-O STAB symbol, get its section only if
    // the STAB symbol's section field refers to a valid section index.
    // Otherwise the symbol may error trying to load a section that
    // does not exist.
```
As the method is used by ELF and Mach-O
```
if (Section == Obj.section_end())
      continue;
```
cover both cases.


================
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();
----------------
CarlosAlbertoEnciso wrote:
> probinson wrote:
> > 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`.
> Changed.
You are correct. The tool does not work for relocatable files compiled with `-ffunction-sections`.
I just tried that option and the tool generates incorrect instructions `LVLineAssembler`.
Added that support for a later patch.


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

https://reviews.llvm.org/D125783