This is an archive of the discontinued LLVM Phabricator instance.

Manual DWARF index: don't skip over -gmodules debug info
ClosedPublic

Authored by aprantl on Jan 26 2023, 6:06 PM.

Details

Summary

This fixes a regression introduced by https://reviews.llvm.org/D131437. The intention of the patch was to avoid indexing DWO skeleton units, but it also skipped over full DWARF compile units linked via a -gmodules DW_AT_dwo_name attribute. This patch restores the functionality and adds a test for it.

Diff Detail

Event Timeline

aprantl created this revision.Jan 26 2023, 6:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2023, 6:06 PM
Herald added a subscriber: arphaman. · View Herald Transcript
aprantl requested review of this revision.Jan 26 2023, 6:06 PM
clayborg added inline comments.Jan 26 2023, 6:12 PM
lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
168

Won't this return nullptr in the fission case where the .dwo file is missing or deleted?

181–190

Can we do a bit more to ensure we aren't using fission here? Are there no other differences between -gmodules and fission we can key off of? Existence of .debug_addr section for fission only?

aprantl updated this revision to Diff 492782.Jan 27 2023, 8:50 AM

I added an unambiguous check for DWARF5+. The remaining incorrectly handled case is a DWARF4 + GNU fission extension where the .dwo file has been deleted. Is there any harm in indexing the skeleton in that case? It shouldn't be able to interfere with the declarations in the (missing) .dwo file in that case, right?
Let me know if this is not enough and I'll try finding another heuristic that we could apply here.

It shouldn't be able to interfere with the declarations in the (missing) .dwo file in that case, right?

Maybe I should have bothered reading your comment in the code. That's exactly the case you were trying to avoid m-(

aprantl added inline comments.Jan 27 2023, 10:20 AM
lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
181–190

The .debug_addr section only exists in DWARF 5 so the IsSkeletonUnit() check covers this.

aprantl updated this revision to Diff 492852.Jan 27 2023, 11:10 AM

Added another heuristic to distinguish gmodules and fission.

clayborg accepted this revision.Jan 27 2023, 1:14 PM

Much better, thanks for making sure we don't end up indexing real skeleton units as they cause problems if they do get indexed.

This revision is now accepted and ready to land.Jan 27 2023, 1:14 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2023, 4:00 PM