This is an archive of the discontinued LLVM Phabricator instance.

[DWARF v5] Supporting verbose dumping of .dbg_rnglist entries
ClosedPublic

Authored by wolfgangp on Feb 15 2018, 5:57 PM.

Details

Summary

Adding verbose dumping to the recent implementation of dumping of v5 range list entries. We're capturing the entries as is as they come in during extraction, including their file offset, so we can dump them in more detail.
The offset table entries which are table-relative are shown as is (as in non-verbose mode) and with the actual file offset they map to.

Adding support for RLE_base_address will come in a separate patch.

Diff Detail

Event Timeline

wolfgangp created this revision.Feb 15 2018, 5:57 PM

Seems awkward/not ideal to compute the start/end, then reverse those computations to compute the values appropriate to print in verbose mode - probably better to keep the original values & compute the final values when needed/printing?

Also I think there's probably some similar printing logic, maybe for the debug_loc.dwo section? Maybe check that to see how the different kinds of address range specifications (start+length, start+end, base address specifier, etc) are printed there & whether there's some uniformity that can be applied/reused/improved here?

wolfgangp updated this revision to Diff 135519.Feb 22 2018, 4:30 PM

Revised the code to take advantage of DWARFAddressRange::dump(), which simplified both non-verbose and verbose dumping. I reduced the range list entries to just raw fields of uint64_t which seems less awkward than using a union with one of the fields an AddressRange. After that there was enough commonality between verbose and non-verbose dumping that it could be handled in one function instead of two.

Added a flag to DWARFAddressRange::dump() to make the printing of interval brackets optional. This makes it a bit more flexible when dumping the raw contents of a range entry that is not a start/end pair. This change could be separated out into its own patch if needed.

I didn't see anything usable in DWARFLocLists, but it appears that DWARFLocLists could make use of DWARFAddressRange. This is probably best addressed in a different patch.

Seems awkward/not ideal to compute the start/end, then reverse those computations to compute the values appropriate to print in verbose mode - probably better to keep the original values & compute the final values when needed/printing?

Also I think there's probably some similar printing logic, maybe for the debug_loc.dwo section? Maybe check that to see how the different kinds of address range specifications (start+length, start+end, base address specifier, etc) are printed there & whether there's some uniformity that can be applied/reused/improved here?

As I pointed out in the revision comment, I didn't see much in the loclist code, but it seems that the loclist code could take advantage of DWARFAddressRange. It handles printing of an interval.

jhenderson added inline comments.Feb 23 2018, 1:31 AM
include/llvm/DebugInfo/DWARF/DWARFDebugRnglists.h
51

Perhaps worth mentioning that in the other cases the Value(s) are undefined.

lib/DebugInfo/DWARF/DWARFDebugRnglists.cpp
46

.dbg_rnglists -> .debug_rnglists

Let's not abbreviate things in error messages that look like section names! Also, use the createError() method, used elsewhere in this function, and it would be good if this error provides the offset of the offending table (again, see the use of createError elsewhere).

Also, test case for this error?

wolfgangp marked 2 inline comments as done.

Addressed James' review comments and gave default values to RangeListEntry's constructor parameters for better use. Added an error test for the currently unsupported DWARF64 format.

include/llvm/DebugInfo/DWARF/DWARFDebugRnglists.h
51

Gave default parameters to the constructor for the less-than-2-value cases.

lib/DebugInfo/DWARF/DWARFDebugRnglists.cpp
46

createError() does not take just a string, but with the added table offset it works.

JDevlieghere added inline comments.Feb 26 2018, 12:06 PM
include/llvm/DebugInfo/DWARF/DWARFAddressRange.h
51

Maybe this is an option we can store in the DIDumpOptions?

include/llvm/DebugInfo/DWARF/DWARFDebugRnglists.h
46

Can these members be const?

lib/DebugInfo/DWARF/DWARFDebugRnglists.cpp
170

Personally I'd prefer reading the verbose flag from the DIDumpOptions.

dblaikie added inline comments.Feb 26 2018, 3:55 PM
include/llvm/DebugInfo/DWARF/DWARFDebugRnglists.h
56

I think this is unnecessary - since there's a non-default ctor, so the default ctor wouldn't be provided by default?

58–61

maybe this ctor could be skipped and {} init could be used?

test/tools/llvm-dwarfdump/X86/debug_rnglists.s
19–24

Probably worth making the prefix indentation consistent (ie: padding the [DW_RLE_...] with spaces on the right, so the DW_RLE_s line up, and the range lines up on the RHS too?

26

Maybe "TERSE" rather than "DEFAULT" to make it easy to see it's the comparison between verbose and not verbose?

wolfgangp updated this revision to Diff 136425.Feb 28 2018, 4:31 PM
wolfgangp marked 6 inline comments as done.

Addressing review comments: The dump routine(s) are using DIDumpOptions more consistently. Added a "DisplayRawContents" flag to DIDumpOptions and using it to inform DWARFAddressRange to either display a range as an interval or print its contents as is.

Removed the constructor for RangeListEntry and used brace-initializer syntax instead.

Aligned the printed range entries for better readability.

include/llvm/DebugInfo/DWARF/DWARFAddressRange.h
51

Decided on an option "DisplayRawContents" with the intent to tell anybody who looks at it to not semantically interpret the content. It's a bit awkward as it's not settable by a user, but it's perhaps closer to what you had in mind.

test/tools/llvm-dwarfdump/X86/debug_rnglists.s
19–24

Collecting the length of the maximal DW_RLE* string and using it to align things. The output still doesn't seem to win any aesthetic awards, though.

dblaikie added inline comments.Feb 28 2018, 4:34 PM
test/tools/llvm-dwarfdump/X86/debug_rnglists.s
45–49

Consider putting the spaces inside the ]? so the ']:' are aligned? (I think some parts of the line table printing have that kind of alignment?)

wolfgangp updated this revision to Diff 137115.Mar 5 2018, 6:58 PM

Review comments: Aligning the closing brackets (']') of range list entries on the right, added a space to 'raw' content dumping of start/length pairs to align the contents with interval-style display.

wolfgangp marked an inline comment as done.Mar 5 2018, 6:59 PM
dblaikie accepted this revision.Mar 5 2018, 7:32 PM

Seems OK as an intermediate step - what's your end-goal/plan? I would think this would need to be merged in some way with existing range list support so that things like the symbolizer can work with DWARF 5? (see DWARFUnit::extractRangeList and DWARFDie::getAddressRanges for example)

lib/DebugInfo/DWARF/DWARFDebugRnglists.cpp
97

It's probably better to use push_back rather than emplace_back, if both are valid. (they'll be both as efficient, but emplace_back can do explicit conversions where push_back can only do implicit conversions - so it's simpler, there's less "going on")

This revision is now accepted and ready to land.Mar 5 2018, 7:32 PM

Seems OK as an intermediate step - what's your end-goal/plan? I would think this would need to be merged in some way with existing range list support so that things like the symbolizer can work with DWARF 5? (see DWARFUnit::extractRangeList and DWARFDie::getAddressRanges for example)

Right, this was just the first step. Next is to support more DW_RLE_* kinds (at least base_address and offset_pair so we can use rnglists in split units) and to make DW_AT_ranges work with DWARF5 rangelists (including DW_FORM_rnglistx), i.e. distill DWARFDebugRngLists into a DWARFAddressRangesVector, or something along those lines.

wolfgangp marked an inline comment as done.Mar 7 2018, 6:31 PM
wolfgangp added inline comments.
lib/DebugInfo/DWARF/DWARFDebugRnglists.cpp
97

Didn't update the review but I'm making this change before I commit.

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