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

[Paul Robinson via Phabricator via llvm-commits]

probinson added a comment.

I'm only partway through, but I didn't want to leave these pending in my browser window overnight.  I'll finish this tomorrow.



================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h:173
 
+bool prettyPrintRegisterOp(DWARFUnit *U, raw_ostream &OS,
+                           DIDumpOptions DumpOpts, uint8_t Opcode,
----------------
This declaration puts the function into the `llvm` namespace which is inappropriate.
I would rather see this become a static public method inside DWARFExpression.



================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h:570
   /// parseDWO - Parses .dwo file for current compile unit. Returns true if
   /// it was actually constructed.
+  bool parseDWO(StringRef AlternativeLocation = {});
----------------
Say something about the parameter here.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:403
+  // the low-pc and high-pc for those scopes that are marked as public, in
+  // order to support DWARF and CodeView.
+  LVPublicNames PublicNames;
----------------
DWARF v5 added .debug_names, does the tool support that section?


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:221
+    if (!SecondMap)
+      return nullptr;
+
----------------
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.



================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:261
+// file or a precompiled header file) has been moved from its original
+// location. That is the case when running regression tests.
+inline std::string createAlternativePath(StringRef To, StringRef From) {
----------------
Should specify that `To` is required to have forward slashes but `From` is not.  Or, do a conversion on `To` as well as `From`.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/LVReaderHandler.h:40
+//
+// The supported binary formats are: ELF, MacOS and CodeView.
+class LVReaderHandler {
----------------



================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h:49
+class LVSymbolTable final {
+  using LVSymbolNames = std::map<std::string, LVSymbolTableEntry>;
+  LVSymbolNames SymbolNames;
----------------
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.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h:81
+  // The virtual address refers to the address where the section is loaded.
+  using LVSectionAddresses = std::map<LVSectionIndex, object::SectionRef>;
+  LVSectionAddresses SectionAddresses;
----------------
So, `LVSectionIndex` isn't actually an (array) index or other small identifier, it's an address?


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h:152
+  LVBinaryReader &operator=(const LVBinaryReader &) = delete;
+  ~LVBinaryReader() {
+    // Delete the containers created by 'createInstructions'.
----------------
Should the `LVBinaryReader` destructor be virtual?


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h:152
+  LVBinaryReader &operator=(const LVBinaryReader &) = delete;
+  ~LVBinaryReader() {
+    // Delete the containers created by 'createInstructions'.
----------------
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.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVELFReader.h:38
+  // call 'prettyPrintRegisterOp'.
+  DIDumpOptions DumpOpts;
+
----------------
This should be a local variable in the place where prettyPrintRegisterOp is called; it doesn't need to be a class member.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVELFReader.h:111
+
+  // Get the location information for DW_AT_data_member_location.
+  void getLocationMember(dwarf::Attribute Attr, const DWARFFormValue &FormValue,
----------------
It seems strange to have "get" methods that don't return anything.  Are there better names?


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


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:1435
+  }
+  return false;
+}
----------------
I think this `return false` is unreachable.  Which means the function always returns true; maybe it should return `void` instead?


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFExpression.cpp:231
                                   const uint64_t Operands[2],
                                   const MCRegisterInfo *MRI, bool isEH) {
   if (!MRI)
----------------
Parameters should be re-aligned to conform to formatting standard.


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



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



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



================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVELFReader.cpp:764
+  if (Opcode == dwarf::DW_OP_regval_type)
+    return {};
+  if ((Opcode >= dwarf::DW_OP_breg0 && Opcode <= dwarf::DW_OP_breg31) ||
----------------
The `if` below is already filtering out the opcode, so this `if` can be removed.


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

https://reviews.llvm.org/D125783