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

[Frederic Riss]

Thanks for taking the time to go though this! I'll respin within a couple of days. Some comments inline:


================
Comment at: lib/DebugInfo/DWARFContext.h:134
@@ -133,3 +133,3 @@
     parseCompileUnits();
-    return CUs[index].get();
+    return CUs.empty() ? nullptr : CUs[index].get();
   }
----------------
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.

================
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;
----------------
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).

================
Comment at: lib/DebugInfo/DWARFDebugLoc.cpp:132
@@ +131,3 @@
+  for (const Entry &E : Entries) {
+    OS << '\n';
+    OS.indent(Indent);
----------------
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.

================
Comment at: lib/DebugInfo/DWARFExpression.cpp:242
@@ +241,3 @@
+  const char *Name = OperationEncodingString(Opcode);
+  assert(Name && "DW_OP has no name!");
+  OS << Name + 3;
----------------
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.

================
Comment at: lib/DebugInfo/DWARFExpression.cpp:243
@@ +242,3 @@
+  assert(Name && "DW_OP has no name!");
+  OS << Name + 3;
+
----------------
dblaikie wrote:
> If we're going to skip the DW_ prefix here, should we skip it elsewhere (tags, attributes, and forms)?
I did it here and not at the other spots, because the DW_OPs might be multiple on a line. I have no strong feeling about stripping the other spots too.

http://reviews.llvm.org/D6771

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