Page MenuHomePhabricator

[DWARFDebugLine] Use truncating data extractors for prologue parsing
Needs ReviewPublic

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

Details

Summary

This makes the code easier to reason about, as it will behave the same
way regardless of whether there is any more data coming after the
presumed end of the prologue.

Diff Detail

Event Timeline

labath created this revision.Apr 6 2020, 8:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2020, 8:03 AM
probinson added inline comments.Apr 6 2020, 8:26 AM
llvm/include/llvm/DebugInfo/DWARF/DWARFDebugLine.h
132

The truncating constructor takes a const DWARFDataExtractor& so I don't understand why this parameter had to change.

Some nice cleanup indeed - thanks! Please wait for @jhenderson to also approve, since he's been doing more work in this area. (never sure whether to approve or not in this case - not sure if the official "approval" might then remove it from other people's review queue, etc)

MaskRay added inline comments.Apr 6 2020, 12:10 PM
llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
457

At a first glance, 2 : 8 looks a bit magical. This may deserve a comment.

probinson added inline comments.Apr 6 2020, 1:33 PM
llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
458

The version check should be < rather than != here.

465

And here, Version < 5 please.

Some nice cleanup indeed - thanks! Please wait for @jhenderson to also approve, since he's been doing more work in this area. (never sure whether to approve or not in this case - not sure if the official "approval" might then remove it from other people's review queue, etc)

FWIW, in my case, I just use my emails as my review queue, so I don't mind others approving too.

llvm/test/tools/llvm-dwarfdump/X86/Inputs/debug_line_malformed.s
195

I wonder if it's worth a comment at the start here saying something to explain why the formatting of the following bytes is weird. Perhaps something like "The following bytes would be read as part of the prologue header, were data extraction not stopped once the data end is reached, hence they are formatted like a header".

Something similar applies for the other changes in this file.

324

I think this comment is missing a word somewhere. Probably "truncated"? Also missing a trailing full stop. Also "md5" -> "MD5".

llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test
115

This should probably show that the last opcode within range is read successfully in addition to showing the past-the-end one is recorded as zero.

143

Missing trailing full stop.

This may not have been clear from the test comments before, but this and the previous case were supposed to work in tandem, because the invalid MD5 form message was reported in both cases, but not the "not at end" message IIRC.

I feel like the input for this case probably should be updated to not have a malformed MD5 form. Is there any interaction any more to do with continuing to parse that these 2 cases now don't need to cover?

206

How straightforward would it be to provide the offset as extra context here, like we do with "unexpected end of data"?

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
470

I'm not sure where this error is coming from (I don't recall seeing it before), but it's got a spurious trailing full stop, which is inconsistent with the other error messages. It might want fixing separately I guess.

478–482

What happened to this error message?