This is an archive of the discontinued LLVM Phabricator instance.

Defer the decision whether to use the CU or TU index until after reading the unit header.
ClosedPublic

Authored by jgorbe on Feb 5 2021, 6:19 PM.

Details

Summary

In DWARF v4 compile units go in .debug_info and type units go in
.debug_types. However, in v5 both kinds of units are in .debug_info.
Therefore we can't decide whether to use the CU or TU index just by
looking at which section we're reading from. We have to wait until we
have read the unit type from the header.

Diff Detail

Event Timeline

jgorbe created this revision.Feb 5 2021, 6:19 PM
jgorbe requested review of this revision.Feb 5 2021, 6:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2021, 6:19 PM
labath accepted this revision.Feb 6 2021, 4:31 AM

Nice catch. Regarding the implementation, I think it might be slightly cleaner (and slightly more consistent with the llvm dwarf parser) if the index handling has moved to the DWARFUnit class (as there's nothing to be gained now by doing it higher up). In DWARFUnit::extract, you could do something like:

if (dwarf.GetDWARFContext().isDWO()) {
  cu_index = &dwarf.GetDWARFContext().GetAsLLVM().getCUIndex();
  ...
lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
808–809

I guess this could be header.IsTypeUnit() (and !header.IsTypeUnit()))...

This revision is now accepted and ready to land.Feb 6 2021, 4:31 AM
jgorbe added inline comments.Feb 11 2021, 10:46 AM
lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
808–809

But !header.IsTypeUnit would also treat DW_UT_partial and DW_UT_skeleton as compile units, right?

jgorbe updated this revision to Diff 323166.Feb 11 2021, 2:54 PM

Moved index handling to DWARFUnit.cpp as suggested by Pavel.

labath added inline comments.Feb 15 2021, 3:47 AM
lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
808–809

That's true, but can either of those units legitimately appear in a dwp file?
Even if they do appear for whatever reason, it wouldn't make any sense to use them without an index entry, and it would be (somewhat) more reasonable to put them in the cu index.

I took this idea from the equivalent llvm code: https://github.com/llvm/llvm-project/blob/main/llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp#L81

jgorbe updated this revision to Diff 325884.Feb 23 2021, 1:20 PM

Changed logic that checked the type of unit to use header.IsTypeUnit() as suggested by reviewer.

I'm going to go ahead and commit this given that I have just made the last suggested modification and the patch has already been up for review without further comments for a long while (sorry for the late replies, work keeps getting in the way of work).

lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
808–809

Good point. Changed.

This revision was landed with ongoing or failed builds.Feb 23 2021, 1:26 PM
This revision was automatically updated to reflect the committed changes.