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

[David Blaikie]

================
Comment at: lib/DebugInfo/DWARFContext.h:134
@@ -133,3 +133,3 @@
     parseCompileUnits();
-    return CUs[index].get();
+    return CUs.empty() ? nullptr : CUs[index].get();
   }
----------------
Where does this case come up? It looks like the calls to getCompileUnitAtIndex are protected by a getNumCompileUnits check first, no?

================
Comment at: lib/DebugInfo/DWARFDebugLoc.cpp:124
@@ -82,27 +123,3 @@
     Locations.resize(Locations.size() + 1);
-    LocationList &Loc = Locations.back();
-    Loc.Offset = Offset;
-    dwarf::LocationListEntry Kind;
-    while ((Kind = static_cast<dwarf::LocationListEntry>(
-                data.getU8(&Offset))) != dwarf::DW_LLE_end_of_list_entry) {
-
-      if (Kind != dwarf::DW_LLE_start_length_entry) {
-        llvm::errs() << "error: dumping support for LLE of kind " << (int)Kind
-                     << " not implemented\n";
-        return;
-      }
-
-      Entry E;
-
-      E.Start = data.getULEB128(&Offset);
-      E.Length = data.getU32(&Offset);
-
-      unsigned Bytes = data.getU16(&Offset);
-      // A single location description describing the location of the object...
-      StringRef str = data.getData().substr(Offset, Bytes);
-      Offset += Bytes;
-      E.Loc.resize(str.size());
-      std::copy(str.begin(), str.end(), E.Loc.begin());
-
-      Loc.Entries.push_back(std::move(E));
-    }
+    if (!parseOneLocationList(data, &Offset, Locations.back()))
+      return;
----------------
This would leave an extra element in the Locations container, wouldn't it? (Could be avoided with the return-by-value I was suggesting:

  if (Optional<LocationList> o = parseOneLocationList(...))
    Locations.push_back(std::move(*o));
  else
    return;

or similar)

================
Comment at: lib/DebugInfo/DWARFDebugLoc.cpp:132
@@ +131,3 @@
+  for (const Entry &E : Entries) {
+    OS << '\n';
+    OS.indent(Indent);
----------------
leading newline seems a surprising contract - but maybe that's convention here, I haven't looked.

================
Comment at: lib/DebugInfo/DWARFDebugLoc.h:63
@@ +62,3 @@
+
+  void parseOneLocationList(DataExtractor Data, unsigned AddressSize,
+                            uint32_t *Offset, LocationList &LL);
----------------
Would it make sense for this to return the LocationList, rather than use an out parameter?

================
Comment at: lib/DebugInfo/DWARFDebugLoc.h:90
@@ +89,3 @@
+
+  static bool parseOneLocationList(DataExtractor Data, uint32_t *Offset,
+                                   LocationList &LL);
----------------
Possibly return Optional<LocationList> here

================
Comment at: lib/DebugInfo/DWARFExpression.cpp:68
@@ +67,3 @@
+
+  class DescriptionArray {
+    std::vector<Description> Descriptions;
----------------
Not sure if this needs a class, rather than just a global const vector, and a function to construct and return it for initialization.

================
Comment at: lib/DebugInfo/DWARFExpression.cpp:242
@@ +241,3 @@
+  const char *Name = OperationEncodingString(Opcode);
+  assert(Name && "DW_OP has no name!");
+  OS << Name + 3;
----------------
If this is meant to be resilient to failure/corrupted files, you'd probably need this not to be an assertion. (again, not sure - but I assume if you guys want to ship this it should be pretty robust & corrupt files shouldn't just assert fail/UB (& should be tested))

================
Comment at: lib/DebugInfo/DWARFExpression.cpp:243
@@ +242,3 @@
+  assert(Name && "DW_OP has no name!");
+  OS << Name + 3;
+
----------------
If we're going to skip the DW_ prefix here, should we skip it elsewhere (tags, attributes, and forms)?

================
Comment at: lib/DebugInfo/DWARFExpression.cpp:255
@@ +254,3 @@
+    
+    switch (Size & ~Op::SignBit) {
+    case Op::Size1:
----------------
Breaking out this switch into a function might help readability

================
Comment at: lib/DebugInfo/DWARFExpression.cpp:300
@@ +299,3 @@
+    default:
+      assert(0 && "Unknown DWARFExpression Op size");
+    }
----------------
Favor llvm_unreachable over assert(0)

================
Comment at: test/DebugInfo/X86/dbg-value-const-byref.ll:25
@@ +24,3 @@
+; CHECK:     DW_AT_location [DW_FORM_data4]	(
+; CHECK-NEXT:   0x0000000000000001 - 0x000000000000000b: OP_consts +3
+; CHECK-NEXT:   0x000000000000000b - 0x0000000000000012: OP_consts +7
----------------
The prior version of this test didn't hardcode the address offsets - it'd be good to preserve that to make the test resilient to minor changes (like instruction selection, etc).

================
Comment at: test/DebugInfo/X86/debug-loc-offset.ll:50
@@ -49,1 +49,3 @@
+; do not check the location stored here, as it will be offseted by the
+; compile unit's low_pc. The real check is below in the debug_loc section.
 ; CHECK-NOT: DW_TAG
----------------
Not sure I understand the comment

http://reviews.llvm.org/D6771

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