Page MenuHomePhabricator

[DWARFDebugLine] Avoid dumping prologue members we did not parse
ClosedPublic

Authored by labath on Feb 24 2020, 5:02 AM.

Details

Summary

This patch if motivated by D74560, specifically the subthread about what
to print upon encountering reserved initial length values.

If the debug_line prologue has an unsupported version, we skip parsing
the rest of the data. If we encounter an reserved initial length field,
we don't even parse the version. However, we still print out all members
(with value 0) in the dump function.

This patch introduces early exits in the Prologue::dump function so that
we print only the fields that were parsed successfully. In case of an
unsupported version, we skip printing all subsequent prologue fields --
because we don't even know if this version has those field. In case of a
reserved unit length, we don't print anything -- if the very first field
of the prologue is invalid, it's hard to say if we even have a prologue
to begin with.

Note that the user will still be able to see the invalid/reserved
initial length value in the error message. I've modified (reordered)
debug_line_invalid.test to show that the error message comes straight
after the debug_line offset. I've also added some flush() calls to the
dumping code to ensure this is the case in all situations (without that,
the warnings could get out of sync if the output was not a terminal -- I
guess this is why std::iostreams have the tie() function).

Diff Detail

Event Timeline

labath created this revision.Feb 24 2020, 5:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2020, 5:02 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
dblaikie accepted this revision.Feb 24 2020, 11:06 AM

Sounds good to me

This revision is now accepted and ready to land.Feb 24 2020, 11:06 AM

Looks good in principle, just a couple of test suggestions from me.

llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test
44–46

For this and the two version checks, it's probably worth adding a CHECK-NOT immediately after the warning that shows the (rest of the) prologue is not printed.

Without it, I don't think you have any test coverage for the early exit, right?

71–72

For each of these other checks, it might be worth an extra CHECK line that shows that we do continue to print after the version field. What do you think? (Obviously, it's not necessary where there is already a file/include directory table check).

labath marked 3 inline comments as done.Feb 25 2020, 4:37 AM
labath added inline comments.
llvm/test/tools/llvm-dwarfdump/X86/debug_line_invalid.test
44–46

Indeed, thanks for pointing that out. I was too busy updating this test to realize that the new functionality was not actually tested.

labath updated this revision to Diff 246415.Feb 25 2020, 4:37 AM
labath marked an inline comment as done.

Add additional checks

jhenderson accepted this revision.Feb 25 2020, 5:35 AM

LGTM, with one nit.

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

Super nit: here and elsewhere, add a single space after NONFATAL: so that it's lined up the same as the other checks in this file (where the colon lines up with the second 'o' in 'prologue').

This revision was automatically updated to reflect the committed changes.

Looks like this broke tests on Windows: http://45.33.8.238/win/9151/step_11.txt

Can you take a look please?

Thanks. I've reverted that for now. I'll investigate off-line.