Page MenuHomePhabricator

[DWARFDebugLine] Check for (EOF) errors when parsing v5 content descriptors
ClosedPublic

Authored by labath on Apr 6 2020, 8:00 AM.

Details

Summary

Without that we could be silently reading zeroes, as that's the default
DataExtractor behavior. The entire parse would still most likely fail,
but it would do that with a seemingly unrelated/nonsensical error
message.

Diff Detail

Event Timeline

labath created this revision.Apr 6 2020, 8:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2020, 8:00 AM
dblaikie accepted this revision.Apr 6 2020, 10:59 AM

Looks good - thanks!

This revision is now accepted and ready to land.Apr 6 2020, 10:59 AM

I feel like this is slightly undertested: at the moment, you only have one new test case, but there are two other places where a warning can be detected, one of which causes a different code path too (the early out from the loop). I think all three need testing.

llvm/test/tools/llvm-dwarfdump/X86/debug_line_short_prologue.s
2

This is probably fine as is, but did you consider extending debug_line_invalid.s (and its corresponding input file) or the gtest unit tests to test this?

37

Could you add a comment please, saying why this entry is considered too short?

labath updated this revision to Diff 256276.Apr 9 2020, 6:32 AM
labath marked 2 inline comments as done.
  • add more test cases
llvm/test/tools/llvm-dwarfdump/X86/debug_line_short_prologue.s
2

debug_line_invalid.s won't cut it because this needs to be at the end of the section (at least until the truncation patch lands -- after that I was planning on deleting it as it will be equivalent to one of the tests there -- however, given the new test cases I've added now, it can probably stay).

The dwarf generator in the unittests would suffer from a similar problem -- I could get it to emit a shorter prologue length (though I would have to manually compute what that length would be), but it would still try to emit the prologue in entirety.

37

I've done something (maybe?) even better. One of the new cases I've added shows how the prologue would look like if it was complete.

jhenderson accepted this revision.Apr 14 2020, 12:10 AM

LGTM, with one request.

llvm/test/tools/llvm-dwarfdump/X86/debug_line_short_prologue.s
2–3

Something I'm trying to encourage is using "##" for comment markers in lit tests using assembly/YAML etc, to distinguish from the RUN and CHECK lines. I find it makes them stand out better. Would you mind changing this here?

37

Thanks. I didn't know that logic was even possible in assembly!

labath marked an inline comment as done.Apr 14 2020, 7:03 AM
labath added inline comments.
llvm/test/tools/llvm-dwarfdump/X86/debug_line_short_prologue.s
2–3

Yeah, I think that's a good convention -- I just keep forgetting about that.

This revision was automatically updated to reflect the committed changes.