[PATCH] D39294: [llvm-dwarfdump] - Teach verifier to report broken DWARF expressions.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:384
+    ++NumErrors;
+    error() << TitleMsg;
+    Die.dump(OS, 0, DumpOpts);
----------------
aprantl wrote:
> perhaps also add the `<< ":\n"` ?
Added `\n`, can't add `:\n` because have to handle reporting for `DW_AT_stmt_list` below.


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:405
       if (*SectionOffset >= DObj.getLineSection().Data.size()) {
         ++NumErrors;
         error() << "DW_AT_stmt_list offset is beyond .debug_line "
----------------
JDevlieghere wrote:
> Can we use ReportError here too?
Done.


================
Comment at: lib/DebugInfo/DWARF/DWARFVerifier.cpp:424
+    DWARFUnit *U = Die.getDwarfUnit();
+    DataExtractor Data(StringRef((const char *)Expr->data(), Expr->size()),
+                       DCtx.isLittleEndian(), 0);
----------------
JDevlieghere wrote:
> I don't think we can avoid the cast, but let's use reinterpret_cast here.  
Done.


================
Comment at: test/tools/llvm-dwarfdump/X86/verify_broken_exprloc.s:12
+# VERIFY:      DW_TAG_GNU_call_site_parameter
+# VERIFY-NEXT:   DW_AT_location    (decoding error.)
+
----------------
aprantl wrote:
> where does the `.` after error come from? it looks out of place. Perhaps `<decoding error>` would be better?
`.` comes from https://github.com/llvm-mirror/llvm/blob/master/lib/DebugInfo/DWARF/DWARFExpression.cpp#L224.

It does not seem this message was ever tested before, so I removed the dot. 

`<decoding error>` looks better for me, but I would not change `()` to `<>` 
in this patch as it comes from some different place and probably should break a lot of tests, 
so looks should be done as separate change (if should).


https://reviews.llvm.org/D39294