This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Prevent crash when .debug_line line_range is zero
ClosedPublic

Authored by jhenderson on Feb 19 2018, 8:28 AM.

Details

Summary

This change adds a check for a .debug_line line_range value of 0 to prevent the program crashing when a special opcode or DW_LNS_const_add_pc is detected. The diagnostic is only issued once per table, and for this case the opcodes do not adjust the address or line.

Depends on D74819.

Diff Detail

Event Timeline

jhenderson created this revision.Feb 19 2018, 8:28 AM
aprantl added inline comments.Feb 19 2018, 8:32 AM
lib/DebugInfo/DWARF/DWARFDebugLine.cpp
309 ↗(On Diff #134927)

Can you also make it so that llvm-dwarfdump --verify reports this as an error?

aprantl accepted this revision.Feb 19 2018, 8:32 AM
aprantl added inline comments.
lib/DebugInfo/DWARF/DWARFDebugLine.cpp
309 ↗(On Diff #134927)

Nevermind. That's what you did!

This revision is now accepted and ready to land.Feb 19 2018, 8:32 AM
aprantl added inline comments.Feb 19 2018, 8:34 AM
lib/DebugInfo/DWARF/DWARFDebugLine.cpp
302 ↗(On Diff #134927)

Bonus points for fixing this one, too :-)

309 ↗(On Diff #134927)

Can you use DWARFVerifier::warn() here?

jhenderson added inline comments.Feb 19 2018, 9:31 AM
lib/DebugInfo/DWARF/DWARFDebugLine.cpp
290 ↗(On Diff #134927)

And presumably this one too @aprantl?

309 ↗(On Diff #134927)

I'll take a look for this and the other usages here.

llvm-dwarfdump --verify reports an error automatically if false is returned here, so that's how we get an error, as demonstrated in the test.

aprantl added inline comments.Feb 19 2018, 9:33 AM
lib/DebugInfo/DWARF/DWARFDebugLine.cpp
290 ↗(On Diff #134927)

That would be great. Can be a separate commit, and you don't need to open a review for that one.

jhenderson added inline comments.Feb 19 2018, 9:36 AM
lib/DebugInfo/DWARF/DWARFDebugLine.cpp
309 ↗(On Diff #134927)

Can you use DWARFVerifier::warn() here?

Just quickly looking at this before I leave the office. This function is currently private, and relies on an instance of DWARFVerifier, which I don't think is available. Do you mean to make it do the same thing as what DWARFVerifier::warn() does?

IMHO, since these "warnings" effectively abort parsing, the right thing to do here is returning an llvm::error which can then be printed by the verifier.

jhenderson planned changes to this revision.Feb 20 2018, 3:21 AM

IMHO, since these "warnings" effectively abort parsing, the right thing to do here is returning an llvm::error which can then be printed by the verifier.

Having thought about this a bit and played around, I'm inclined to agree that this is the correct approach. It'll be a bigger change, but it will make the code more amenable to being used as a library, where the consumers can decide what to do with the errors/warnings on a case-by-case basis. I'll experiment with it a bit more, and then upload an updated patch.

IMHO, since these "warnings" effectively abort parsing, the right thing to do here is returning an llvm::error which can then be printed by the verifier.

Having thought about this a bit and played around, I'm inclined to agree that this is the correct approach. It'll be a bigger change, but it will make the code more amenable to being used as a library, where the consumers can decide what to do with the errors/warnings on a case-by-case basis. I'll experiment with it a bit more, and then upload an updated patch.

Great, thanks!

jhenderson edited the summary of this revision. (Show Details)
jhenderson added reviewers: ikudrin, MaskRay.
jhenderson changed the repository for this revision from rL LLVM to rG LLVM Github Monorepo.
jhenderson added a subscriber: chrisjackson.

Resurrecting this change, by rewriting the fix on the latest HEAD. In this version, the check is done when a special opcode or DW_LNS_const_add_pc is encountered. An Error is reported if the line_range is 0 for the first such instance of these. Testing is now done via unit tests.

This revision is now accepted and ready to land.Feb 14 2020, 7:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2020, 7:54 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
jhenderson requested review of this revision.Feb 14 2020, 7:55 AM

Please could this be re-reviewed, given the significant changes in the area since the last review.

jhenderson planned changes to this revision.Feb 19 2020, 1:14 AM

I missed a case that needs checking. Will revisit.

jhenderson retitled this revision from [DWARF] Prevent crash when .debug_line line_range is zero to [DebugInfo] Prevent crash when .debug_line line_range is zero.
jhenderson edited the summary of this revision. (Show Details)

Rewrite based on D75188 and D74819.

It looks like getOperationAdvance() and advanceAddr() now share a lot of parameters. Is it feasible to combine them? I see that they're not *always* called together, but it seems like it would improve clarity not to repeat 3 lines of parameters.

jhenderson updated this revision to Diff 247604.Mar 2 2020, 4:41 AM

Rebase, to adopt interface change as suggested by @probinson. Also fix a warning in the test.

I see a pre-merge clang-format alert in DWARFDebugLine.h that you should take care of.

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

This seems a bit much for a ternary operator.

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

While you _can_ write this in one line, I think it's clearer to write

if (LineRange == 0)
  return Base + AdvanceIncr;
return AdjustAddressFixtureBase::getAdjustedAddr( ... );
jhenderson updated this revision to Diff 248140.Mar 4 2020, 3:29 AM
jhenderson marked 2 inline comments as done.

Fix formatting. Simplify ternaries.

probinson accepted this revision.Mar 4 2020, 9:20 AM

LGTM.

This revision is now accepted and ready to land.Mar 4 2020, 9:20 AM
jhenderson updated this revision to Diff 248467.Mar 5 2020, 6:37 AM

Rebase to take account of opcode name handling changes.

One more small bit of cleanup.

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

This could go under the if as you don't need the name unless you are reporting the warning.

628

Setting the flag to false can go under the if to help distinguish error handling from functional code.

jhenderson updated this revision to Diff 248684.Mar 6 2020, 2:56 AM
jhenderson marked 2 inline comments as done.

Rebase, and put bits inside the if.

jhenderson updated this revision to Diff 249056.Mar 9 2020, 4:09 AM

Remove unnecessary public.

This revision was automatically updated to reflect the committed changes.