This is an archive of the discontinued LLVM Phabricator instance.

Make a DWARFDIE class that can help avoid using the wrong DWARFUnit when extracting attributes
ClosedPublic

Authored by clayborg on Dec 9 2016, 2:51 PM.

Details

Summary

Many places pass around a DWARFDebugInfoEntryMinimal and a DWARFUnit. It is easy to get things wrong by using the wrong DWARFUnit with a DWARFDebugInfoEntryMinimal. This patch creates a DWARFDie class that contains the DWARFUnit and DWARFDebugInfoEntryMinimal objects so that they can't get out of sync. All attribute extraction has been moved out of DWARFDebugInfoEntryMinimal and into DWARFDie. DWARFDebugInfoEntryMinimal was also renamed to DWARFDebugInfoEntry.

DWARFDie objects are temporary objects that are used by clients and contain 2 pointers that you always need to have anyway. Keeping them grouped will avoid errors and simplify many of the attribute extracting APIs by not having to pass in a DWARFUnit.

Diff Detail

Repository
rL LLVM

Event Timeline

clayborg updated this revision to Diff 80959.Dec 9 2016, 2:51 PM
clayborg retitled this revision from to Make a DWARFDIE class that can help avoid using the wrong DWARFUnit when extracting attributes.
clayborg updated this object.
clayborg added a subscriber: beanz.
aprantl added inline comments.Dec 9 2016, 3:16 PM
include/llvm/DebugInfo/DWARF/DWARFDebugInfoEntry.h
29 ↗(On Diff #80959)

Is this comment still accurate?

include/llvm/DebugInfo/DWARF/DWARFDie.h
61 ↗(On Diff #80959)

Optional<uint32_t> ?
Optional<DwarfOffset>, to allow 64-bit?

73 ↗(On Diff #80959)

Is it reasonable to call this if Die is null, or should this be an assertion?

77 ↗(On Diff #80959)

operator bool() const?
Oh, or does this mean the NULL DIE that is at the end of each group?

91 ↗(On Diff #80959)

Perhaps Optional instead of default-construction?

107 ↗(On Diff #80959)

attributes

125 ↗(On Diff #80959)

Just a general remark: I wonder if these interfaces wouldn't be better if they returned Optionals instead of taking fail values, but I haven't looked at the call sites yet.

295 ↗(On Diff #80959)

Typically we write these functions as taking a SmallVectorImpl&, but I wonder if this isn't an artifact from before we could use auto in the code base.

lib/DebugInfo/DWARF/DWARFContext.cpp
160 ↗(On Diff #80959)

these changes could be separate NFC commits. No need to pre-commit-review them.

Commented a bit with fixes coming soon.

include/llvm/DebugInfo/DWARF/DWARFDebugInfoEntry.h
29 ↗(On Diff #80959)

Yes it is still accurate.

include/llvm/DebugInfo/DWARF/DWARFDie.h
61 ↗(On Diff #80959)

These are wrapping the APIs that used to be in DWARFDebugInfoEntryMinimal for now. I will add Optional support to all values in a follow up patch.

71 ↗(On Diff #80959)

Many APIs can give you back an DWARFDie object that don't contain a DWARFUnit or DWARFDie so it does make sense to answer as correctly as possible

77 ↗(On Diff #80959)

This means valid but NULL Die. I should actually change this do be:

return Die != nullptr && getAbbreviationDeclarationPtr() == nullptr;

Because isNULL should only be true if we have a valid DWARFDebugInfoEntry, yet it has an abbrev code of zero

91 ↗(On Diff #80959)

Good idea.I'll do that in a follow up change once this is in.

124–125 ↗(On Diff #80959)

Let me do this in a follow up change otherwise this will introduce a ton of churn in this change. Is that ok?

lib/DebugInfo/DWARF/DWARFContext.cpp
159–160 ↗(On Diff #80959)

It isn't a pointer anymore, it is an object. So this does go along with this change. It is invoking the operator bool on DWARFDie.

clayborg updated this revision to Diff 80963.Dec 9 2016, 3:43 PM

Fixed Adrian's issues.

clayborg marked 2 inline comments as done.Dec 9 2016, 3:45 PM

Marked things as done.

clayborg updated this revision to Diff 80972.Dec 9 2016, 4:18 PM

Fixed some issues that David Blaikie mentioned on the mailing list: add asserts to functions in DWARFDie that return things like the getOffset(), getTag(), hasChildren() and a few others. Users should check a DWARFDie for validity before using it.

aprantl accepted this revision.Dec 13 2016, 9:33 AM
aprantl edited edge metadata.

This is looking good now. Looking forward to the follow-up commits that will clean up the API and introduce Optionals, etc.

This revision is now accepted and ready to land.Dec 13 2016, 9:33 AM
This revision was automatically updated to reflect the committed changes.