This is an archive of the discontinued LLVM Phabricator instance.

[DWARF] NFC: Collect info needed by DWARFFormValue into a helper
ClosedPublic

Authored by probinson on Jun 23 2017, 1:32 PM.

Details

Summary

The size of the value described by a form can depend on DWARF format (32/64-bit), DWARF version, or the size of a target address. Collect these parameters into a class to make it simpler to pass them around, and have just one place to define query methods that understand the effect of the parameters on form sizes.

Inspired by review comments on D33155. This patch seemed worth splitting out as it was a bunch of typing and fiddled with an API, while still being NFC. A subsequent step would be to collect up the section references needed for retrieving the value (as opposed to the size) of the attribute described be the form, which should finish the job of making DWARFFormValue agnostic to whether the data comes from a "unit" or a line-table section.

Diff Detail

Repository
rL LLVM

Event Timeline

probinson created this revision.Jun 23 2017, 1:32 PM

As a follow-up, I was thinking that the DwarfFormat enum could be given a 1-byte base type (given that it has only two values), which would make DWARFFormParams only 4 bytes, which would make it reasonable to pass around by value instead of by reference.

dblaikie accepted this revision.Jun 23 2017, 1:53 PM

Other than simplifying the DWARFFormParams a bit, this seems good to me.

(if DWARFAbbreviationDeclaration::extract is the only place that doesn't pass these extra params - maybe force that caller to construct a default DWARFFormParams, and remove the optionality from the primary API to avoid new callers neglecting to pass this important thing (& leave a comment there explaining that DWARF might need to be improved so that's supported? Or if it's supported & LLVM's parsing is missing functionality, a comment about that (about how parsing abbrevs can't be done without knowing the unit (& its address size, etc) that is specified there))

& shrinking the enum/passing by value seems fine by me in a followup if you like

llvm/include/llvm/DebugInfo/DWARF/DWARFFormValue.h
29–63 ↗(On Diff #103755)

This seems like a bit more encapsulation than I'd expect/seems warranted for a few parameters.

Especially the protected ctor, imho - it doesn't change any invariants of the class (the 3-arg ctor can be used to construct a default constructed object) & doesn't seem to protect from any really trivial misuse.

Also the 3-arg ctor would be about as good as braced init, and the value can be changed at any time with assignment anyway - maybe make this a struct? & leave the getRefAddrByteSize+getDwarfOffsetByteSize member functions for convenience - but I'd strip the rest of the accessors/ctors/friending out and make it a struct.

llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp
64 ↗(On Diff #103755)

Should this be optional? The only place that doesn't pass it looks to be DWARFAbbreviationDeclaration::extract - is that usage correct? Does the abbrev table implicit_const not support the extra features? Should there be some error path/check for any forms that would require the params?

This revision is now accepted and ready to land.Jun 23 2017, 1:53 PM
probinson added inline comments.
llvm/include/llvm/DebugInfo/DWARF/DWARFFormValue.h
29–63 ↗(On Diff #103755)

Yeah, I waffled on that. I'll simplify.
So to be clear, you're suggesting eliminating the 3-arg ctor as well?

llvm/lib/DebugInfo/DWARF/DWARFFormValue.cpp
64 ↗(On Diff #103755)

There is a use-case for scanning the abbreviations table to determine which ones are fixed-size in all cases. And the abbreviations table can be shared by units/line-tables with different formats/versions. This was an important point for @clayborg and LLDB, I think.
So yes, I think it should remain optional. We can change our minds later if I'm wrong.

This revision was automatically updated to reflect the committed changes.