This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Remove lldb's DWARFAbbreviationDeclarationSet in favor of llvm's
ClosedPublic

Authored by bulbazord on Jun 8 2023, 3:34 PM.

Details

Summary

lldb's and llvm's implementations of DWARFAbbreviationDeclarationSet are
now close enough (almost the same, actually) to replace lldb's with
llvm's wholesale. llvm's is also tested against the same kinds of
scenarios that lldb's is tested against so we can remove lldb's tests
here. (see: llvm/unittests/DebugInfo/DWARF/DWARFDebugAbbrevTest.cpp).

Diff Detail

Event Timeline

bulbazord created this revision.Jun 8 2023, 3:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2023, 3:34 PM
bulbazord requested review of this revision.Jun 8 2023, 3:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2023, 3:34 PM
fdeazeve accepted this revision.Jun 9 2023, 6:59 AM

This is awesome! I believe you said there was no measurable perf diff?

This revision is now accepted and ready to land.Jun 9 2023, 6:59 AM
aprantl accepted this revision.Jun 9 2023, 8:57 AM

Nice!

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
61

at some point we could modernize this into

llvm::DenseSet<dw_form_t> DWARFDebugAbbrev::GetUnsupportedForms()
62

nit: LLVM coding style would probably elide the {}

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h
23

for a later commit: this should probably be a DenseMap for better performance?

This is awesome! I believe you said there was no measurable perf diff?

Yeah, my initial experiments measured no significant perf difference no matter how you built lldb. llvm's and lldb's DWARFAbbreviationDeclarationSet implementations both use llvm's DWARFAbbreviationDeclaration and have the same semantics around their extraction. I intentionally made both implementations behave the exact same so that this change would be as simple as possible.

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
62

Will update, thanks!

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h
23

Possibly? It would be interesting to switch away from std::map to a different type (like DenseMap) to measure performance. Either way, I also want to switch us to use llvm::DWARFDebugAbbrev, so it might be worth doing that work first.

bulbazord updated this revision to Diff 530021.Jun 9 2023, 10:41 AM

Adjust formatting