[DWARF v5] improved support for .debug_rnglists/consumer
ClosedPublic

Authored by wolfgangp on Apr 11 2018, 5:57 PM.

Details

Summary

This patch is intended to significantly improve the support for DWARF v5 range lists (in the .debug_rnglists section) on the consumer side. Basically, it enables any consumer to extract a DWARF v5 encoded rangelist (via DIE::getAddressRanges()).
Still missing for full support are the DW_RLE-* encodings that use an index into the .debug_addr section to designate address ranges (e.g. DW_RLE_startx_endx). These will be supported at a later stage.

This patch does the following:

  1. Adds a method to the DebugRnglist class to return a DWARFAddressRangesVector based on it.
  2. Adds methods to the DebugRnglistTable class to a) retrieve a rangelist based on an offset and if needed extract it from the section b) given an index, retrieve an entry into the table's offset table (which represents an offset to a range list). c) add a format property to the table class to designate which dwarf format the table is encoded in (only DWARF32 supported at the moment).
    1. Adds a range list table to the DWARFUnit class which holds the DebugRnglistTable that belongs to the unit.
    2. Adding 3 methods to DWARFUnit to a) obtain DWARFAddressRangesVectors given either a range list offset or an index. The former is used with DW_FORM_sec_offset, the latter with DW_FORM_rnglistx, when handling the DW_AT_ranges attribute in DIEs. b) retrieve the range list offset that results from using an index supplied with DW_FORM_rnglistx. This is used only to dump the offset.
  1. Slightly refactor the dumping code in DWARFContext in the face of dumping both .debug_rnglists and .debug_rnglists.dwo.
  1. Using getRelocatedAddress() instead of getAddress() when extracting range lists.
  1. When dumping a rangelist referenced from a DIE, and the DW_AT_ranges attribute uses FORM_rnglistx, add some code to dump the offset that is retrieved via the index.
  1. Change DWARFDie::getAddressRanges() to use the interface now provided by DWARFUnit to obtain a DWARFAddressRangesVector via DW_AT_ranges.
  1. Some additions to DWARFFormValue to deal with DW_Form_rnglistx.
  1. DWARFUnit now parses the range list table header in version 5 and tries to set up the range list table based on the DW_AT_rnglists_base attribute in the CU DIE. It does this in both split and full CUs (though no DW_AT_rnglists_base in split units).

Diff Detail

Repository
rL LLVM
wolfgangp created this revision.Apr 11 2018, 5:57 PM
JDevlieghere added inline comments.Apr 12 2018, 8:32 AM
include/llvm/DebugInfo/DWARF/DWARFDebugRnglists.h
120 ↗(On Diff #142094)

Is Format checked before this function is called? If not, this should probably also be an Optional as it can fail due to bad input. Otherwise I'd add a comment specifying this as a pre-condition.

lib/DebugInfo/DWARF/DWARFDebugRnglists.cpp
200 ↗(On Diff #142094)

Same as my other comment, we shouldn't crash on bad input.

lib/DebugInfo/DWARF/DWARFUnit.cpp
152 ↗(On Diff #142094)

Would it make sense to turn this into an Expected and propagate the Error from extractHeaderAndOffsets?

This looks generally good to me.

include/llvm/DebugInfo/DWARF/DWARFDebugRnglists.h
120 ↗(On Diff #142094)

Nit: By moving the unreachable outside of the switch statement, we could benefit from the incomplete-switch warning if a new enumerator is added.

lib/DebugInfo/DWARF/DWARFContext.cpp
255 ↗(On Diff #142094)

Not your code, but since you touched it: IIRC there's supposed to be a helper function that prints "error:" in color somewhere in libDebugInfo.

lib/DebugInfo/DWARF/DWARFUnit.cpp
381 ↗(On Diff #142094)

That's a matter of taste, but I would have put the Version <= 4 case up front since its shorter.

dblaikie added inline comments.Apr 12 2018, 1:42 PM
test/DebugInfo/X86/dwarfdump-rnglists.s
6–8 ↗(On Diff #142094)

You could use one CU with rnglists_base (but maybe it's better to test the case where there is no rnglists_base, as you are doing?) - the spec says that rnglistx /may/ be used in the presence of rngilsts_base, but doesn't have to be used.

(the indirection of indexing into a table of offsets in rnglists, and the fact that the rnglists_base points to the bytes /after/ the header seem both a bit weird/unfortunate, but I guess they were picked that way for a reason (the latter seems problematic because it looks to me like that would mean the section can't be versioned effectively - if a newer version of the table changed the layout, how would that be parsed? The header would have to remain the same size & the parser would have to walk backwards from rnglists_base to see if the version is the same? :/)

135 ↗(On Diff #142094)

This comment should be DW_RLE_end_of_list, perhaps?

155–176 ↗(On Diff #142094)

Usually we put the CHECK lines at the start, or sometimes interleaved with the content they're checking - perhaps you could do that? (you can even have multiple section chunks so you could keep the ranges for a specific CU next to the CU to make the tests relatively isolated rather than interleaved, perhaps?)

wolfgangp marked 2 inline comments as done.

Addressed review comments:

  1. Changed parseRngListTableHeader to return an Expected<> rather than Optional<> to allow reporting of errors we find during initial parsing of a range list table.
  2. Use helper routines to use color encoding with error reporting.
include/llvm/DebugInfo/DWARF/DWARFDebugRnglists.h
120 ↗(On Diff #142094)

Is Format checked before this function is called? If not, this should probably also > be an Optional as it can fail due to bad input. Otherwise I'd add a comment
specifying this as a pre-condition.

Actually, format is (or rather, will be once DWARF64 support is added) derived from parsing the table, and it really can only be either DWARF32 or DWARF64 (or something else to be added in the future). If it isn't it's clearly an internal error. I'll add a comment to that effect.

lib/DebugInfo/DWARF/DWARFContext.cpp
255 ↗(On Diff #142094)

Found it. So far it seems to be only used by the verifier.

lib/DebugInfo/DWARF/DWARFDebugRnglists.cpp
200 ↗(On Diff #142094)

Same as my other comment, we shouldn't crash on bad input.

Any unsupported encodings should have been found during the initial extraction of the range list, which would have prevented the list from being internalized. So at this point we expect a list with only supported encodings. I'll add a comment to that effect.

lib/DebugInfo/DWARF/DWARFUnit.cpp
152 ↗(On Diff #142094)

Good idea. This allows reporting of errors when we first parse the range list table.

JDevlieghere added inline comments.Apr 14 2018, 3:11 PM
lib/DebugInfo/DWARF/DWARFContext.cpp
255 ↗(On Diff #142094)

I've added convenience methods in WithColor (rL330091) and I've updated the other uses in libDebugInfo (rL330092).

Sorry for the long delay on this (I was out of the country with sketchy internet connections). I'm using the 'WithColor' convenience functions as Jonas suggested. Otherwise, I believe all review comments have been addressed.

JDevlieghere accepted this revision.May 16 2018, 2:22 AM

Looks good to me, thanks Wolfgang!

lib/DebugInfo/DWARF/DWARFDebugRnglists.cpp
366 ↗(On Diff #146643)

Would it be worth propagating the error and have this function return an Expected<>?

This revision is now accepted and ready to land.May 16 2018, 2:22 AM
wolfgangp added inline comments.May 16 2018, 10:21 AM
lib/DebugInfo/DWARF/DWARFDebugRnglists.cpp
366 ↗(On Diff #146643)

Would it be worth propagating the error and have this function return an Expected<>?

It would improve the reporting on invalid DW_AT_ranges attributes in the DIEs. Seems to require a few more changes in DWARFDie.cpp, but it would be worthwhile. I'll take a look at it.

jhenderson added inline comments.May 17 2018, 1:44 AM
lib/DebugInfo/DWARF/DWARFDebugRnglists.cpp
366 ↗(On Diff #146643)

This would be a nice parallel to the work I did on improving the debug line error handling in D44560, and probably something we should keep in mind for any further changes/additions to the DWARF implementation.

wolfgangp added inline comments.May 18 2018, 9:47 AM
lib/DebugInfo/DWARF/DWARFDebugRnglists.cpp
366 ↗(On Diff #146643)

I'm inclined to do this in a follow-on patch to keep the number of changes smaller.

This revision was automatically updated to reflect the committed changes.