This is an archive of the discontinued LLVM Phabricator instance.

[DWARF] Modification of code for the verification of .debug_info section.
ClosedPublic

Authored by sgravani on Jul 14 2017, 5:48 PM.

Details

Summary

This patch modifies the handleDebugInfo() function so that we verify the contents of each unit
in the .debug_info section only if its header has been successfully verified.

This change will allow for more/different verification checks depending on the type of the unit since from
dwarf5, the .debug_info section may consist of different types of units.

Diff Detail

Event Timeline

sgravani created this revision.Jul 14 2017, 5:48 PM
sgravani retitled this revision from [DWARF] Refactoring of code for the verification of .debug_info section. to [DWARF] Modification of code for the verification of .debug_info section..Jul 14 2017, 5:51 PM
sgravani edited the summary of this revision. (Show Details)
sgravani updated this revision to Diff 106742.Jul 14 2017, 6:22 PM

Minor, yet very important, change to the previous diff.
OffsetStart has to be equal to the beginning of the unit before verifying the contents of the unit...

sgravani updated this revision to Diff 106743.Jul 14 2017, 6:24 PM

This is the diff from the trunk..

I made a mistake in my previous update and submitted the diff from the previous revision.

aprantl added inline comments.Jul 17 2017, 8:23 AM
lib/DebugInfo/DWARF/DWARFVerifier.cpp
95

Since Tag isn't used anywhere else:
if (Die.getTag() == DW_TAG_null)

102

It looks like this might also return false if the unit is correct, but there were previous errors? Is that what we want here?

141

I think it might be better to handle the default: case separately and have it produce a verification error.

167

Same point applies as in line 101, though here it looks more like this is intentional.

sgravani updated this revision to Diff 106922.Jul 17 2017, 1:03 PM
sgravani marked 4 inline comments as done.

Addressed reviewer's comments.

lib/DebugInfo/DWARF/DWARFVerifier.cpp
102

No this is not what we want. We want to know if each unit has errors or not.
I fixed it, thanks.

141

I think what makes the best sense here is to handle the cases for all valid types and UnitType = 0 ( meaning that we are verifying a compile unit in dwarf4 version), and have llvm_unreachable() for default case.

sgravani updated this revision to Diff 106926.Jul 17 2017, 1:09 PM

Addressed comments.
Added test case for verifying a "mix" of units in the .debug_info section.
The first unit has an invalid DIE, the second unit passes verification and the third unit has an invalid length.

aprantl accepted this revision.Jul 17 2017, 4:27 PM

LGTM with outstanding changes applied. Thanks!

lib/DebugInfo/DWARF/DWARFVerifier.cpp
142

-> for consistency: in DWARF v4.

172

Perhaps return the number of errors:

unsigned DWARFVerifier::verifyDebugInfoAttribute(const DWARFDie &Die, DWARFAttribute &AttrValue);

285

same here

This revision is now accepted and ready to land.Jul 17 2017, 4:27 PM
sgravani closed this revision.Jul 17 2017, 6:02 PM
sgravani marked 3 inline comments as done.

Closed with commit: rL308245