This is an archive of the discontinued LLVM Phabricator instance.

[llvm-dwarfdump] - Teach verifier to report broken DWARF expressions.
ClosedPublic

Authored by grimar on Oct 25 2017, 9:22 AM.

Details

Summary

Patch improves next things:

  1. Fixes assert/crash in getOpDesc when giving it a invalid expression op code.
  2. DWARFExpression::print() called DWARFExpression::Operation::getEndOffset() which returned and used uninitialized field EndOffset. Patch fixes that.
  3. Teaches verifier to verify DW_AT_location and error out on broken expressions.

Diff Detail

Event Timeline

grimar created this revision.Oct 25 2017, 9:22 AM
grimar retitled this revision from [llvm-dwarfdump] - Fix issues that might happen when object with broken DWARF expressions is parsed. to [llvm-dwarfdump] - Teach verifier to report broken DWARF expressions..Oct 25 2017, 9:35 AM
aprantl accepted this revision.Oct 25 2017, 1:51 PM

Thanks!

lib/DebugInfo/DWARF/DWARFVerifier.cpp
384

perhaps also add the << ":\n" ?

test/tools/llvm-dwarfdump/X86/verify_broken_exprloc.s
12

where does the . after error come from? it looks out of place. Perhaps <decoding error> would be better?

This revision is now accepted and ready to land.Oct 25 2017, 1:51 PM
JDevlieghere added inline comments.Oct 25 2017, 1:52 PM
lib/DebugInfo/DWARF/DWARFVerifier.cpp
405

Can we use ReportError here too?

JDevlieghere added inline comments.Oct 25 2017, 2:00 PM
lib/DebugInfo/DWARF/DWARFVerifier.cpp
424

I don't think we can avoid the cast, but let's use reinterpret_cast here.

grimar updated this revision to Diff 120382.Oct 26 2017, 3:32 AM
  • Addressed review comments.
  • Minor cosmetic change.
lib/DebugInfo/DWARF/DWARFVerifier.cpp
384

Added \n, can't add :\n because have to handle reporting for DW_AT_stmt_list below.

405

Done.

424

Done.

test/tools/llvm-dwarfdump/X86/verify_broken_exprloc.s
12

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

JDevlieghere accepted this revision.Oct 26 2017, 9:08 AM

Thanks, George! LGTM

Thanks!

test/tools/llvm-dwarfdump/X86/verify_broken_exprloc.s
12

This is obviously not super important, but I thought of
DW_AT_location (<decoding error>)
The <> is meant to "escape" the error message and make it look different from an actual value.

Adrian, Jonas, thank you for review !

test/tools/llvm-dwarfdump/X86/verify_broken_exprloc.s
12

Ah, there is no problems to do that in this patch then I believe,
I'll change to (<decoding error>) before commit.

This revision was automatically updated to reflect the committed changes.