Page MenuHomePhabricator

[DWARFDataExtractor] Add a "truncating" constructor
ClosedPublic

Authored by labath on Apr 6 2020, 8:02 AM.

Details

Summary

This constructor allows us to create a new DWARFDataExtractor which will
only present a subrange of an entire debug section. Since debug sections
typically consist of multiple contributions, it is expected that one
will create a new data extractor for each contribution in order to
avoid unexpectedly running off into the next one.

This is very useful for unifying the flows for detecting parse errors.
Without it, the code needs to consider two very different scenarios:

  1. If there is another contribution after the current one, the DataExtractor functions will just start reading from there. This is detectable by comparing the current offset against the known end-of-contribution offset.
  2. If this is the last contribution, the data extractor will just start returning zeroes (or other default values). This situation can *not* be detected by checking the parsing offset, as this will not be advanced in case of errors.

Using a truncated data extractor simplifies the code (and reduces
cognitive load) by making these two cases behave identically -- a
running off the end of a contribution will _always_ produce an EOF error
(if one uses error-aware parsing methods) or return default values.

Diff Detail

Event Timeline

labath created this revision.Apr 6 2020, 8:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2020, 8:02 AM
Herald added a subscriber: aprantl. · View Herald Transcript
dblaikie accepted this revision.Apr 6 2020, 11:06 AM

Looks good - thanks! (I guess at some point we might want a DWARFDataExtractor that can be offset as well - for reading sections in DWP files (essentially create a view of the file based on the index, restricting a CU to only being able to see (& resolve references relative to) the regions described by the index))

This revision is now accepted and ready to land.Apr 6 2020, 11:06 AM
jhenderson added inline comments.Apr 7 2020, 12:37 AM
llvm/include/llvm/DebugInfo/DWARF/DWARFDataExtractor.h
42–46

I wonder if this should be more generic than the DWARFDataExtractor class, i.e. be in DataExtractor? The concept of limiting length to a specified amount is hardly DWARF specific - ELF sections, for example, sometimes need parsing and have a limited length.

Preumably of course, we'd still want this constructor, but it would just forward to the base class one.

llvm/unittests/DebugInfo/DWARF/DWARFDataExtractorTest.cpp
197

I think it would make sense to show the behaviour where an entry can be partially read, i.e. the length truncates the field. I think we'd need this tested both for a location with a relocation and for one without.

(Sorry about the delay, last week has been pretty busy for me.)

Replying to these two comments together:

Looks good - thanks! (I guess at some point we might want a DWARFDataExtractor that can be offset as well - for reading sections in DWP files (essentially create a view of the file based on the index, restricting a CU to only being able to see (& resolve references relative to) the regions described by the index))

I wonder if this should be more generic than the DWARFDataExtractor class, i.e. be in DataExtractor? The concept of limiting length to a specified amount is hardly DWARF specific - ELF sections, for example, sometimes need parsing and have a limited length.

Preumably of course, we'd still want this constructor, but it would just forward to the base class one.

An offseting constructor for a DWARFDataExtractor would be slightly tricky because we would still need to use the "original" offset when resolving relocations. Nothing unsolvable, but it did not seem worth implementing when there's no use case -- dwp files don't have relocations and they deal with this by substr-ing the StringRef before they create a DWARFDataExtractor. The same approach can be used for regular DataExtractors, so a substr constructor is not strictly necessary there. However, it would be trivial to add, and it seems like it could come in handy sometimes, so I can add one if you want.

labath updated this revision to Diff 257352.Apr 14 2020, 8:28 AM
  • add more tests for reads which cross extractor boundaries. This required a fix to getRelocatedValue, which was spun out to a separate patch
  • this patch does not (yet) introduce any additional constructors
labath updated this revision to Diff 257374.Apr 14 2020, 9:17 AM
  • add const & to Twine
jhenderson accepted this revision.Apr 17 2020, 7:43 AM

LGTM, thanks. I don't know that we need the other constructor, but if there's other code that could be simplified by adding it, it might make sense (but probably could be a separate change).

This revision was automatically updated to reflect the committed changes.