This is an archive of the discontinued LLVM Phabricator instance.

[dwarfdump] Verify that unit type matches root DIE
ClosedPublic

Authored by JDevlieghere on Oct 2 2017, 4:40 AM.

Details

Summary

This patch adds two new verifications:

  • Check that the root DIE of a CU is actually a valid unit DIE. (based on its tag)
  • For DWARF5 which contains a unit type int he CU header, check that this matches the type of the unit DIE.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Oct 2 2017, 4:40 AM
aprantl added inline comments.Oct 2 2017, 9:07 AM
include/llvm/DebugInfo/DWARF/DWARFUnit.h
315 ↗(On Diff #117325)

Is that correct? I thought imported units are an orthogonal concept to split units. (But I may be wrong here)

lib/DebugInfo/DWARF/DWARFVerifier.cpp
187 ↗(On Diff #117325)

Perhaps: Compilation unit header without any DIEs?

  • Feedback Adrian
JDevlieghere marked an inline comment as done.Oct 2 2017, 9:24 AM
JDevlieghere added inline comments.
include/llvm/DebugInfo/DWARF/DWARFUnit.h
315 ↗(On Diff #117325)

Nope, this is wrong. I made this assumption before checking the standard which in its example uses a regular compile_unit. I must haveuploaded the patch before fixing this.

lib/DebugInfo/DWARF/DWARFVerifier.cpp
187 ↗(On Diff #117325)

I'd be glad to change this if you think that's better. I felt like this is implicit/obvious and prevents the confusion that we might still be verifying the header. But again, I have no strong opinion about this.

aprantl accepted this revision.Oct 6 2017, 10:51 AM
aprantl added inline comments.
include/llvm/DebugInfo/DWARF/DWARFUnit.h
315 ↗(On Diff #117325)

this seems right now, thanks.

This revision is now accepted and ready to land.Oct 6 2017, 10:51 AM
aprantl added inline comments.Oct 6 2017, 10:52 AM
include/llvm/DebugInfo/DWARF/DWARFUnit.h
296 ↗(On Diff #117366)

Should we move these two into Dwarf.h as isUnitTag ?

JDevlieghere marked an inline comment as done.Oct 6 2017, 11:55 AM
JDevlieghere added inline comments.
include/llvm/DebugInfo/DWARF/DWARFUnit.h
296 ↗(On Diff #117366)

Sure, sounds reasonable!

This revision was automatically updated to reflect the committed changes.
dblaikie added inline comments.Oct 9 2017, 11:07 AM
llvm/trunk/include/llvm/BinaryFormat/Dwarf.h
348

This is probably incorrect - imported_units seem to be used to import a unit into another unit, they aren't themselves a valid top-level unit. (See Figure E.6 in DWARF5 for an example of their use)

JDevlieghere added inline comments.Oct 9 2017, 3:34 PM
llvm/trunk/include/llvm/BinaryFormat/Dwarf.h
348

Thanks David! I sent an e-mail to the dwarf standard mailing list and the first reply confirms you are indeed correct. I've committed a fix that removes this.