This is an archive of the discontinued LLVM Phabricator instance.

[DWARF] Factor out a DWARFUnitHeader class. NFC
ClosedPublic

Authored by probinson on May 10 2018, 11:03 AM.

Details

Summary

Extract information related to a "unit header" from DWARFUnit into a new
DWARFUnitHeader class, and have DWARFUnit inherit it. This is one step
in the direction of recognizing type units in the .debug_info section.

Diff Detail

Repository
rL LLVM

Event Timeline

probinson created this revision.May 10 2018, 11:03 AM
dblaikie added inline comments.May 10 2018, 2:47 PM
llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
238 ↗(On Diff #146161)

Any chance of composition rather than inheritance? (I realize that'll be a bunch more churn (changing "getLength" to "getHeader().getLength", etc, presumably) - but it seems like the inheritance here isn't accurate - a DWARFUnit is not a DWARFUnitHeader (in the "isa" kind of phrasing/test about inheritance))

Guess that could be done in a separate patch to make it clearer.

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
278 ↗(On Diff #146161)

Rather than testing the section inside DWARFUnitHeader::extract - perhaps you could pass down UnitType here?

clayborg added inline comments.May 10 2018, 3:53 PM
llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
238 ↗(On Diff #146161)

It should be easy as each constructor was taking a "const DWARFUnitHeader &Header" parameter already, so instead of constructing DWARFUnitHeader with it, we could copy or move it into the m_header ivar.

probinson added inline comments.May 11 2018, 6:44 AM
llvm/include/llvm/DebugInfo/DWARF/DWARFUnit.h
238 ↗(On Diff #146161)

Yeah, I wondered about composition versus inheritance. If I throw in DWARFUnit methods that forward to the Header member then it would be less churn.

llvm/lib/DebugInfo/DWARF/DWARFVerifier.cpp
278 ↗(On Diff #146161)

Not 100% sure what you're suggesting--yes in this path we do already have the UnitType, but in the non-verifier path we don't, so passing in the UnitType feels like adding unnecessary complexity to extract().

probinson updated this revision to Diff 146429.May 11 2018, 3:39 PM

Use composition instead of inheritance.

dblaikie accepted this revision.May 12 2018, 11:58 AM
dblaikie added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
114–116 ↗(On Diff #146429)

Not sure this comment makes sense - you can't look at the unit DIE, right? Because you have to parse the header before you parse the DIEs, so that doesn't seem like a relevant suggestion/approach here?

This revision is now accepted and ready to land.May 12 2018, 11:58 AM
probinson marked an inline comment as done.May 14 2018, 1:23 PM
probinson added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
114–116 ↗(On Diff #146429)

Fair. Changed the comment to avoid the suggestion.

This revision was automatically updated to reflect the committed changes.
probinson marked an inline comment as done.