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

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere requested changes to this revision.
JDevlieghere added a comment.
This revision now requires changes to proceed.

Thanks for working on this, Rafael!

Halfway through the review I remembered that we already have a `DWARFExpression` class that has some logic for parsing DWARF Expressions and Operations. 
Could you have a look at merging your implementation? Depending on how big a change that is, it would be great if you could split that off into a separate code review.



================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:84
+  /// returns. Otherwise, an error occurred.
+  void parse(DataExtractor Data, uint32_t *Offset, uint32_t EndOffset);
+
----------------
Let's return an `llvm::error` here instead of having unreachables in the code. The caller can then decide what to do with it.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:89
+  /// than EndOffset indicates an error.
+  void parseDWARFExpression(Instruction &Ins, DataExtractor Data,
+                            uint32_t *Offset, uint32_t EndOffset);
----------------
Probably the previous comment applies here too.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugFrame.h:282
   // True if this is parsing an eh_frame section.
-  bool IsEH;
+  const bool IsEH;
+  // Not zero for sane pointer values coming out of eh_frame
----------------
Can we merge the private parts of this class? I'd just move this to the bottom. 


================
Comment at: lib/DebugInfo/DWARF/DWARFDataExtractor.cpp:30
+
+uint64_t DWARFDataExtractor::getEncodedPointer(uint32_t *Offset,
+                                               uint8_t Encoding,
----------------
This should probably be an optional too?


Repository:
  rL LLVM

https://reviews.llvm.org/D43313