This is an archive of the discontinued LLVM Phabricator instance.

[DWARFDebugLine] Use new DWARFDataExtractor::getInitialLength
ClosedPublic

Authored by labath on Feb 25 2020, 7:37 AM.

Details

Summary

The error messages change somewhat, but I believe the overall
informational value remains unchanged.

Diff Detail

Event Timeline

labath created this revision.Feb 25 2020, 7:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2020, 7:37 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
jhenderson added inline comments.Feb 25 2020, 8:27 AM
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
347–350

If I understand this correctly, this will now report cases where the unit length couldn't be read due to data running out. Should this change include some testing for that too?

Also, in that case, if I've followed the code correctly, the error message will be "parsing line table prologue at offset 0x00000000: unexpected end of data", which doesn't give particularly great precision as to where the unexpected end of data was. How about adding something like "parsing line table prologue at offset 0x00000000: unable to read unit_length: unexpected end of data"?

1218

Idle thought: If wonder if this should be unit tested (probably at a slightly higher level)? I could easily see a bug where a length equal to DW_LENGTH_lo_reserved in a DWARF64 file was not treated as valid.

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

This and the following case might want renaming slightly, since the offset is no longer at the end. Perhaps "ParserMarkedAsDoneFor..."?

labath planned changes to this revision.Feb 26 2020, 8:56 AM
labath marked 3 inline comments as done.

I haven't managed to get around to updating this patch today. I'll try to do it tomorrow.

llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
347–350

Yes, I think I can add a test for the "data running out" case.

I also believe you're right about the error message. I can add the extra string since it is convenient here, but in general I wouldn't want to subscribe to this level of detail in error messages. My goal is to make error handling to only take up a fraction of the function doing the parsing. Ideally I'd just carry the Error (or Cursor) object around, and only check it once or twice per function -- kind of like what the debug_names patch (D75119) does now. If I wanted report the exact point at which the data "ran out", I'd have to check for errors after every get() operation.

1218

Yes, I've noticed this too, and I am pretty sure there was a bug here. I'll see if I can construct some meanigful test here without creating a 4GB buffer.

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

will do

I had another thought about the "unexpected end of data" message: would it be possible to include the offset at which the data ended? That would still give sufficient context to the message without the need to check in lots of different places.

labath added a comment.EditedFeb 27 2020, 7:02 AM

I had another thought about the "unexpected end of data" message: would it be possible to include the offset at which the data ended? That would still give sufficient context to the message without the need to check in lots of different places.

Yes, that is definitely possible. In fact I was considering that myself but I wasn't sure whether this extra information would be useful.

Now that I think about, I think this is a good idea, particularly once we have truncated data extractors, as then the value will not just be the end of the section.

labath marked 5 inline comments as done.Feb 27 2020, 8:48 AM
labath added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp
347–350

Test case added. Instead of adding the error prefix, I've created D75265.

1218

The most interesting thing to test here would be to check that the parser correctly advances to the next unit for a contribution with length=DW_LENGTH_lo_reserved. However, that is not possible, since the very next thing it does after checking totalLengthIsValid() (which would have returned false before, but returns true now) is to check that the next unit offset fits into the data extractor.

However, I have created a dwarfdump test that ensures that we can dump a prologue of an incomplete contribution with this length (my other patch has introduced a bug where we would skip dumping of prologues with these lengths. That seems to be the next best thing.

labath updated this revision to Diff 246966.Feb 27 2020, 8:48 AM
labath marked 5 inline comments as done.
  • add more tests
jhenderson accepted this revision.Feb 28 2020, 2:16 AM

LGTM, thanks!

llvm/test/tools/llvm-dwarfdump/X86/debug_line_dwarf64_large_table.s
2–4

This isn't a hard policy or anything, but I often find it more readable for comments in lit tests to use '##' instead of just '#', as it helps them stand out from lit and FileCheck directives.

17

Nit: too many blank lines here.

This revision is now accepted and ready to land.Feb 28 2020, 2:16 AM
labath marked 2 inline comments as done.Mar 2 2020, 2:12 AM
This revision was automatically updated to reflect the committed changes.