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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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? |
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). |
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) |
Seems good - thanks!
llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp | ||
---|---|---|
542 | I'd leave the const here |
Maybe these shouldn't be auto? Certainly, C looks like it should be a number (e.g. uint64_t), but it obviously isn't.