This is an archive of the discontinued LLVM Phabricator instance.

[lldb][NFCI] Apply IndexEntry to DWARFUnitHeader outside of extraction
ClosedPublic

Authored by bulbazord on Jun 1 2023, 12:32 PM.

Details

Summary

I plan on replacing LLDB's DWARFUnitHeader implementation with LLVM's.
LLVM's DWARFUnitHeader::extract applies the DWARFUnitIndex::Entry to a
given DWARFUnitHeader outside of the extraction because the index entry
is only relevant to one place where we may parse DWARFUnitHeaders
(specifically when we're creating a DWARFUnit in a DWO context). To ease
the transition, I've reshaped LLDB's implementation to look closer to
LLVM's.

Diff Detail

Event Timeline

bulbazord created this revision.Jun 1 2023, 12:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2023, 12:32 PM
Herald added a subscriber: arphaman. · View Herald Transcript
bulbazord requested review of this revision.Jun 1 2023, 12:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2023, 12:32 PM
aprantl accepted this revision.Jun 1 2023, 2:32 PM
aprantl added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
889

I know this was also in the original code, so you may not know either: Should these error messages be limited to dwarf packages (.dwp) or should they be more generic?

980

llvm coding style would delete most of these {} ?

This revision is now accepted and ready to land.Jun 1 2023, 2:32 PM
bulbazord added inline comments.Jun 1 2023, 2:33 PM
lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
889

I'm not sure, but my understanding is that we only apply the index entry to the header in DWO contexts, so I assume DWP is involved.

980

Sounds good, I'll update.

fdeazeve accepted this revision.Jun 2 2023, 5:31 AM

I'm not exactly familiar with DWOs, but the code motions LGTM!

lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
882

If LLVM's function is like this as well, we shouldn't do anything now and just refactor it later once we make the switch. But a pointer parameter in a function that asserts not null immediately is more expressive as a reference. Note that, in the code where this is called, we have a:

if (ptr)
  ApplyIndex(ptr)

So the assert is not needed, as the callee respects the contract and can do a *ptr

bulbazord added inline comments.Jun 2 2023, 11:18 AM
lldb/source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp
882

Yes, this was my thought process as well. I wrote it this way because LLVM's function is just like this. I'm also touching that equivalent function in LLVM in a different way (adding error handling, see https://reviews.llvm.org/D151933). Let's rework this in a follow-up?

bulbazord updated this revision to Diff 529769.Jun 8 2023, 4:18 PM

Changed a bit of formatting to follow LLVM style more closely.