This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Check for errors when reading data for extended opcode
ClosedPublic

Authored by jhenderson on May 29 2020, 6:45 AM.

Details

Summary

Previously, if an extended opcode was truncated, it would manifest as an "unexpected line op length error" which wasn't quite accurate. This change checks for errors any time data is read whilst parsing an extended opcode, and reports any errors detected.

Depends on D80796.

Diff Detail

Event Timeline

jhenderson created this revision.May 29 2020, 6:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 29 2020, 6:45 AM
aprantl added inline comments.May 29 2020, 11:46 AM
llvm/include/llvm/DebugInfo/DWARF/DWARFDataExtractor.h
74

I assume this never turns an error into a success?

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
877

This seems oddly stricter than before. Is the !Err condition necessary?

MaskRay added inline comments.May 30 2020, 12:42 PM
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
856

Does it help clarity using DataExtractor::Cursor and StringRef getCStrRef(Cursor &C) const?

labath added inline comments.Jun 1 2020, 1:14 AM
llvm/include/llvm/DebugInfo/DWARF/DWARFDataExtractor.h
74

Nope. The idea is that if the error is already set, then the function will be a no-op, and the error will remain in the erroneous (and unchecked) state.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
877

I'm guessing this is because in the erroneous state the SubOpcode and Len won't contain real data, but only zeroes, and so a better course of action is to print the error about reaching the end-of-file (contribution) message.

Though if that's the case, then it does bring up the question of why is this the only statement guarded by !Err -- this argument should apply to other print statements too...

jhenderson marked 2 inline comments as done.Jun 1 2020, 2:08 AM
jhenderson added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
856

Maybe. I'm not particularly familiar with the Cursor usage yet, so I'll spend some time experimenting with that to see if it makes sense here.

877

@labath is right in that if there's an error, the "extended opcode" won't be a known value (it'll be another zero), so printing its value doesn't make much sense. Although I guess printing the length might not be a terrible idea. I'll give that some thought.

Re. consistency: I ran out of time to finish uploading a series of patches. One of my later patches does the check for the othe opcodes to stop them printing their arguments. I'll get back to that in the next day or two, and it should make everything consistent. Although now you point that out, maybe I should move this bit into that patch. I'll take a look to see if it makes sense.

jhenderson updated this revision to Diff 267602.Jun 1 2020, 6:36 AM

Switch to using Cursor instead of an explicit offset and Error when parsing extended opcodes. Also removed suppression of output in event of error. A later patch will address this.

jhenderson marked 3 inline comments as done.Jun 1 2020, 6:37 AM
jhenderson planned changes to this revision.Jun 2 2020, 8:03 AM

The unit tests need fixing up. Not sure why there weren't failing for me locally before.

jhenderson updated this revision to Diff 268115.Jun 3 2020, 3:08 AM

Fixed the unit tests by adding an additional check which I'd previously dodged. Also added additional testing as unit testing, to cover all the cases which use the cursor.

labath accepted this revision.Jun 8 2020, 4:16 AM

Looks good to me.

llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
1330–1331 ↗(On Diff #268115)

Maybe OutputRef.split(LinePrefix).second

This revision is now accepted and ready to land.Jun 8 2020, 4:16 AM
jhenderson updated this revision to Diff 269196.Jun 8 2020, 6:24 AM

Rebased tests for recent changes to error messages. Also addressed @labath's review comments.

If there are no objections overnight, I'll put this in tomorrow UK time.

For reference, the pre merge bot tests are failing due to a change in the error message which I haven't rebased on yet. I'll fix that prior to committing.

This revision was automatically updated to reflect the committed changes.