[PATCH] [dwarfdump] Pretty print location expressions and location lists

[Alexey Samsonov]

================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLoc.h:47
@@ -41,2 +46,3 @@
+private:
   typedef SmallVector<LocationList, 4> LocationLists;
 
----------------
Just move private members decl/def to the top of the class.

================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLoc.h:62
@@ -55,2 +61,3 @@
   /// specified address size to interpret the address ranges.
   void parse(DataExtractor data, unsigned AddressSize);
+
----------------
Same here - DataExtractor should now the address size. 

================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLoc.h:65
@@ -57,1 +64,3 @@
+  Optional<LocationList>
+  parseOneLocationList(DataExtractor Data, unsigned AddressSize, uint32_t *Offset);
 };
----------------
I think your DataExtractor should know AddressSize instead.

================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLoc.h:84
@@ -71,3 +83,3 @@
+private:
   typedef SmallVector<LocationList, 4> LocationLists;
   LocationLists Locations;
----------------
Ditto

================
Comment at: include/llvm/DebugInfo/DWARF/DWARFExpression.h:10
@@ +9,3 @@
+
+#ifndef LLVM_DEBUGINFO_DWARFEXPRESSION_H
+#define LLVM_DEBUGINFO_DWARFEXPRESSION_H
----------------
Please fix header guard.

================
Comment at: include/llvm/DebugInfo/DWARF/DWARFExpression.h:82
@@ +81,3 @@
+  public:
+    Description &getDescription() { return Desc; }
+    uint8_t getCode() { return Opcode; }
----------------
Do you actually use these methods anywhere?

================
Comment at: include/llvm/DebugInfo/DWARF/DWARFExpression.h:107
@@ +106,3 @@
+      Offset = Op.isError() ? Expression->Data.getData().size() : Op.EndOffset;
+      Op.Error = Offset >= Expression->Data.getData().size() ||
+        !Op.Extract(Expression->Data, Expression->Unit, Offset);
----------------
Why do you set Op.Error in iterator, but not in Op::Extract?

================
Comment at: include/llvm/DebugInfo/DWARFExpression.h:1
@@ +1,2 @@
+//===--- DWARFExpression.h - DWARF Expression handling ----------*- C++ -*-===//
+//
----------------
This file should be removed in favor of include/llvm/DebugInfo/DWARF/DWARFExpression.h, right?

================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:129
@@ +128,3 @@
+    // getDebugLoc().
+    if (getNumCompileUnits())
+      getDebugLocDWO()->dump(OS, getCompileUnitAtIndex(0));
----------------
This is slightly weird - what if there are no compile units, but non-empty .debug_loc_dwo?

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugInfoEntry.cpp:119
@@ +118,3 @@
+                       U->getContext().isLittleEndian(), 0);
+    DWARFExpression(Data, U).Print(OS);;
+    return;
----------------
extra semicolon

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugInfoEntry.cpp:130
@@ +129,3 @@
+    if (LocSection.Data.size()) {
+      DWARFDebugLoc DebugLoc(LocSection.Relocs);
+      DataExtractor Data(LocSection.Data, U->getContext().isLittleEndian(), 0);
----------------
dblaikie wrote:
> Could possibly abstract over these two differences using a function template helper?
> 
> Or maybe it'd suffice to put the if (LL)/else outside the if conditios so it can be shared (& could add an else to the main if (sec.data.size()) that returns early to make that work? Dunno how that'd look)
I actually think it's fine to leave this as is to keep the code straightforward.
We may shorten it, though
  if (auto LL = DebugLoc.parseOneLocationList) {
    LL->dump(OS, U, Indent);
  } else {
    OS << "error extracting location list.";
  }

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugInfoEntry.cpp:137
@@ +136,3 @@
+      else
+        OS << "error extracting location list.";
+    } else if (LocDWOSection.Data.size()) {
----------------
do you need to set indent here?

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:64
@@ +63,3 @@
+    // 1. A beginning address offset. ...
+    E.Begin = Data.getUnsigned(Offset, AddressSize);
+    if (AI != RelocMap.end())
----------------
getAddress() ?

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:91
@@ +90,3 @@
+    // A single location description describing the location of the object...
+    StringRef str = Data.getData().substr(*Offset, Bytes);
+    *Offset += Bytes;
----------------
Consider using
  E.Loc.resize(Bytes);
  if (!Data.getU8(Offset, E.Loc.data(), Bytes)) {
    llvm::errs() << "...."
  }

same for parsing U16 above
  uint16_t Bytes;
  if (!Data.getU16(Offset, &Bytes, 1)) {
    llvm::errs() << "...";
  }

================
Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:101
@@ -39,2 +100,3 @@
   uint32_t Offset = 0;
   while (data.isValidOffset(Offset+AddressSize-1)) {
+    if (auto LL = parseOneLocationList(data, AddressSize, &Offset))
----------------
isValidOffsetForAddress

================
Comment at: lib/DebugInfo/DWARF/DWARFExpression.cpp:110
@@ +109,3 @@
+static uint8_t getRefAddrSize(uint8_t AddrSize, uint16_t Version) {
+  return (Version == 2) ? AddrSize : 4;
+}
----------------
This is a copy of method from `DWARFFormValue.cpp`? Probably the code should be shared, so that there's one place to fix when we support DWARF64.

http://reviews.llvm.org/D6771

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/