[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/Core/LVSupport.h:221
+    if (!SecondMap)
+      return nullptr;
+
----------------
probinson wrote:
> I assume this is the reason for the requirement that ValueType be a pointer.
> The more idiomatic LLVM way to handle this would be to have `find` return `Optional<ValueType>` and then you can use `None` as the not-there indicator.
> 
> I'm fine with it as is, but you might think about revising this in a follow-up patch.
> 
Good point. Added to list for later refactor.

- Use `Optional<ValueType>`.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h:49
+class LVSymbolTable final {
+  using LVSymbolNames = std::map<std::string, LVSymbolTableEntry>;
+  LVSymbolNames SymbolNames;
----------------
probinson wrote:
> FYI, LLVM has a StringMap class that is advertised as more efficient than `std::map<std::string, ValueType>`.  Mainly it does fewer allocations because the key is not a `std::string`.
> Not saying you should change this now, but keep it in mind for a later refactor.
Good point. Added to list for later refactor.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h:152
+  LVBinaryReader &operator=(const LVBinaryReader &) = delete;
+  ~LVBinaryReader() {
+    // Delete the containers created by 'createInstructions'.
----------------
CarlosAlbertoEnciso wrote:
> probinson wrote:
> > probinson wrote:
> > > Should the `LVBinaryReader` destructor be virtual?
> > You might think about whether it makes sense to put the destructor into the .cpp file so the allocations and deallocations are at least somewhat near each other.
> You are correct. Marked `LVBinaryReader` destructor as `virtual`.
Moved destructor to .cpp file close to `createInstructions` and `getSectionRanges` where the allocations are done.



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:1429
+    return true;
+  } else {
+    if (0 < Entry.DirIdx && Entry.DirIdx <= Prologue.IncludeDirectories.size())
----------------
probinson wrote:
> LLVM style says do not use `else` after `return`.
Removed `else` after `return`.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:1435
+  }
+  return false;
+}
----------------
probinson wrote:
> I think this `return false` is unreachable.  Which means the function always returns true; maybe it should return `void` instead?
The problem is that both returned `true` values must be in the same scope as the `Directory` setting and there a missing `return false` for the case of DWARF >= 5.

```
bool DWARFDebugLine::LineTable::getDirectoryForEntry(
    const FileNameEntry &Entry, std::string &Directory) const {
  if (Prologue.getVersion() >= 5) {
    if (Entry.DirIdx < Prologue.IncludeDirectories.size()) {
      Directory =
          dwarf::toString(Prologue.IncludeDirectories[Entry.DirIdx], "");
      return true;
    }
    return false;
  }
  if (0 < Entry.DirIdx && Entry.DirIdx <= Prologue.IncludeDirectories.size()) {
    Directory =
        dwarf::toString(Prologue.IncludeDirectories[Entry.DirIdx - 1], "");
    return true;
  }
  return false;
}
```


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

https://reviews.llvm.org/D125783