This is an archive of the discontinued LLVM Phabricator instance.

[dwarfdump] Add support for -debug-loc=OFFSET
ClosedPublic

Authored by JDevlieghere on Sep 25 2017, 7:06 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Sep 25 2017, 7:06 AM
dblaikie edited edge metadata.Sep 25 2017, 8:46 AM

That test case looks really long... - could it be a bit more terse?

Also is there a reasonable chance to modify the APIs here so only the loc contribution that's being dumped is parsed, rather than parsing them all and dumping one of them?

aprantl added inline comments.Sep 25 2017, 9:18 AM
lib/DebugInfo/DWARF/DWARFDebugLoc.cpp
55 ↗(On Diff #116549)

This looks like a good opportunity to add an API that uses std:lower_bound to quickly find an entry at an offset. (cf. DWARFDebugFrame::getEntryAtOffset(uint64_t Offset)).

We will need this anyway.

  • Feedback Adrian and Dave

That test case looks really long... - could it be a bit more terse?

I've updated the test with the most minimal thing I could come up with, but let me know if you have an idea to further reduce it.

Also is there a reasonable chance to modify the APIs here so only the loc contribution that's being dumped is parsed, rather than parsing them all and dumping one of them?

I have some code that uses parseOneLocationList(DataExtractor Data, unsigned *Offset) to do this. This is a good solution for valid offsets, but not so much for invalid offsets as it is incremented based on the previously parsed amount of bytes. I have a few concerns about this though:

  • We currently prints errors when we can't parse the section. Would we display them conditionally? (e.g. during regular parsing but not during parsing for dumping)
  • I don't know how likely this scenario is, but what if an invalid offset causes the data to be accidentally parsable?

If we move forward with this approach, I'd recommend doing it in in a separate differential as it will mostly build on top of the changes in this patch. (I'm thinking about the tests, the changes in dump method, and we could even keep the current approach as fallback)

JDevlieghere marked an inline comment as done.Sep 26 2017, 4:14 AM
aprantl edited edge metadata.Sep 26 2017, 10:29 AM

I have some code that uses parseOneLocationList(DataExtractor Data, unsigned *Offset) to do this. This is a good solution for valid offsets, but not so much for invalid offsets as it is incremented based on the previously parsed amount of bytes. I have a few concerns about this though:

  • We currently prints errors when we can't parse the section. Would we display them conditionally? (e.g. during regular parsing but not during parsing for dumping)
  • I don't know how likely this scenario is, but what if an invalid offset causes the data to be accidentally parsable?

If we move forward with this approach, I'd recommend doing it in in a separate differential as it will mostly build on top of the changes in this patch. (I'm thinking about the tests, the changes in dump method, and we could even keep the current approach as fallback)

I support doing the lazy API in a follow-up commit. More fine-grained commits is always preferable, it makes reviews easier and usually yields better test coverage.
As for errors:

  • When it is easily possible to determine that no entry exists at the give offset, we should not print anything.
  • Otherwise, we probably should suppress errors when dumping with DumpOffsets[<section>] != None. Why? Because someone might call llvm-dwarfdump --debug-info=0x000abcd fat_macho.o and I think it is reasonable to dump DIEs in each slice that has something at the given offset and be silent in all others.
aprantl accepted this revision.Sep 26 2017, 10:30 AM
This revision is now accepted and ready to land.Sep 26 2017, 10:30 AM
This revision was automatically updated to reflect the committed changes.