[PATCH] D43313: [DebugInfo] Support parsing DWARF expressions

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere requested changes to this revision.
JDevlieghere added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/llvm/BinaryFormat/Dwarf.h:534
+uintptr_t readULEB128(const uint8_t *&Data);
+uintptr_t readSLEB128(const uint8_t *&Data);
+
----------------
I'm not convinced that (1) we need these functions and (2) that they belong here. Can you look into using their counterparts in the DataExtractor? 


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:68
+
+  using InstrList      = std::vector<Instruction>;
+  using iterator       = InstrList::iterator;
----------------
Please run clang-format over the new/modified code. 


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:100
+  std::vector<Instruction> Instructions;
+  uint64_t CodeAlignmentFactor;
+  int64_t DataAlignmentFactor;
----------------
I believe this and the next line can be const.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:108
+  }
+
+  void addInstruction(uint8_t Opcode, uint64_t Operand1) {
----------------
Let's add a comment to the next two functions as well to keep things consistent. 


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:125
+  }
+
+  void addExprOperation(Instruction &Ins, uint8_t Opcode, uint64_t Operand1) {
----------------
Same here.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:231
+private:
+  /// The following fields are defined in section 6.4.1 of the DWARF standard v4
+  uint8_t Version;
----------------
I know this code was moved, but it would be nice if you could check which of these can be made const since you're touching these lines. 


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:275
+  /// The following fields are defined in section 6.4.1 of the DWARF standard v3
+  uint64_t LinkedCIEOffset;
+  uint64_t InitialLocation;
----------------
Same as previous comment.


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:323
+    OS << " " << OperationEncodingString(Opcode);
+    if (ExprOp.Ops.size() == 0)
+      continue;
----------------
`::empty()`?


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:407
+    for (unsigned i = 0; i < IndentLevel; ++i)
+      OS << "  ";
+    OS << CallFrameString(Opcode) << ":";
----------------
I think you can call `::indent(2*IndentLevel)`


================
Comment at: lib/DebugInfo/DWARF/DWARFDebugFrame.cpp:599
+      uint64_t LSDAAddress = 0;
+      auto *Cie = CIEs[IsEH ? (StartStructureOffset - CIEPointer) : CIEPointer];
 
----------------
Please don't use auto when you can't deduct the type form the RHS


Repository:
  rL LLVM

https://reviews.llvm.org/D43313