[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();
   }
----------------
friss wrote:
> dblaikie wrote:
> > Where does this case come up? It looks like the calls to getCompileUnitAtIndex are protected by a getNumCompileUnits check first, no?
> Hmmm. I remember asking me the same question when I skimmed over the patch before I sent it out so I must left it intentionally... though I can;t find a good reason right now. Maybe I added the guards afterwards, dunno.
Fair enough - maybe just switch to an assert for now, then.

================
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;
----------------
friss wrote:
> dblaikie wrote:
> > 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)
> There would be no extra element in standard cases. But yes, if parseOneLocation errors out, you'd get an incomplete location in the list. (The normal exit of the loop should always be handled by the isValidOffset() condition).
Ah, fair - well, either way then, depending on how pedantic about bad inputs you want to be. (in some ways having the weird extra empty element might at least tell a user there's something weird, compared to silently skipping it)

================
Comment at: lib/DebugInfo/DWARFDebugLoc.cpp:132
@@ +131,3 @@
+  for (const Entry &E : Entries) {
+    OS << '\n';
+    OS.indent(Indent);
----------------
friss wrote:
> dblaikie wrote:
> > leading newline seems a surprising contract - but maybe that's convention here, I haven't looked.
> I agree. Only motivation is: putting the newline at the end requires a conditional, because we don't want the last line to have a newline.
Ah, fair enough

================
Comment at: lib/DebugInfo/DWARFExpression.cpp:242
@@ +241,3 @@
+  const char *Name = OperationEncodingString(Opcode);
+  assert(Name && "DW_OP has no name!");
+  OS << Name + 3;
----------------
friss wrote:
> dblaikie wrote:
> > 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))
> This one is one purpose. If we reach that point if the code, it means the Op has a description in the table, so it really should have a name in the support routine. No user input should be able to break that.
Makes sense.

Oh, btw - should we just put a pointer to the name in the ExpressionOps table?

http://reviews.llvm.org/D6771

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