This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Fix reading location tables headers of v5 units in DWP.
ClosedPublic

Authored by ikudrin on Mar 31 2020, 7:36 AM.

Details

Summary

This fixes the reading of location lists headers for compilation units in package files by adjusting the reading offset according to the corresponding record in the unit index. This is required for DW_FORM_loclistx to work.

Diff Detail

Event Timeline

ikudrin created this revision.Mar 31 2020, 7:36 AM
jhenderson accepted this revision.Apr 1 2020, 12:39 AM

Looks good to me, although I'm not fully up to speed on .debug_loclists to be 100% certain, so might be worth a second opinion.

What did the code do before? Parse the first one repeatedly?

llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
541–542

Maybe these shouldn't be auto? Certainly, C looks like it should be a number (e.g. uint64_t), but it obviously isn't.

This revision is now accepted and ready to land.Apr 1 2020, 12:39 AM
ikudrin marked an inline comment as done.Apr 1 2020, 1:18 AM

Looks good to me, although I'm not fully up to speed on .debug_loclists to be 100% certain, so might be worth a second opinion.

Thanks!

What did the code do before? Parse the first one repeatedly?

The code is OK for a DWO case, but is not ready for DWP. The support for DWARFv5 DWP files is added in D75929, so this code also needs to be updated.

llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
541–542

getOffset() returns const DWARFUnitIndex::Entry::SectionContribution *. Not that obvious, I agree. Do you think it would be better to rename C to, say, Contrib or Contribution?

jhenderson added inline comments.Apr 1 2020, 2:12 AM
llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
541–542

I think the issue actually is the function name: getOffset probably wants renaming to something like getContribution. Contrib is probably fine for the variable name (though I'm not sure if using`auto` here isn't a violation of LLVM's coding standards, regardless of the variable or function name).

dblaikie added inline comments.Apr 1 2020, 3:57 PM
llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
541–542

Yeah, I don't mind the rename - either before or after this change. I'm pretty OK with "auto" given the very short scope & pointer-ness (making it obvious what boolean testing the thing does, etc)

ikudrin updated this revision to Diff 254504.Apr 2 2020, 6:45 AM
  • C -> Contrib.

I've created D77302 for renaming getOffset functions.

ikudrin marked 2 inline comments as done.Apr 2 2020, 6:45 AM
ikudrin updated this revision to Diff 254805.Apr 3 2020, 8:00 AM
  • Rebase to the tip.
  • const auto *Contrib -> auto *Contrib.
dblaikie accepted this revision.Apr 5 2020, 4:37 PM

Seems good - thanks!

llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
542

I'd leave the const here

This revision was automatically updated to reflect the committed changes.