This is an archive of the discontinued LLVM Phabricator instance.

[lldb][NFCI] Switch to using llvm::DWARFAbbreviationDeclaration
ClosedPublic

Authored by bulbazord on May 16 2023, 1:38 PM.

Details

Summary

Both LLVM and LLDB implement DWARFAbbreviationDeclaration. As of
631ff46cbf51, llvm's implementation of
DWARFAbbreviationDeclaration::extract behaves the same as LLDB's
implementation, making it easier to merge the implementations.

The only major difference between LLDB's implementation and LLVM's
implementation is that LLVM's DWARFAbbreviationDeclaration is slightly
larger. Specifically, it has some metadata that keeps track of the size
of a declaration (if it has a fixed size) so that it can potentially
optimize extraction in some scenarios. I think this increase in size
should be acceptable and possibly useful on the LLDB side.

Diff Detail

Event Timeline

bulbazord created this revision.May 16 2023, 1:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2023, 1:38 PM
bulbazord requested review of this revision.May 16 2023, 1:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2023, 1:38 PM
aprantl accepted this revision.May 18 2023, 9:17 AM

Overall seems good to me. Minor comments inline.

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

Is this intentionally a copy or should this be a reference? I have no idea how heavyweight this object is...

56–58

would std::find_if be shorter or would it look worse?

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

Nit: we usually do local includes first and then the stdlib at the bottom.

This revision is now accepted and ready to land.May 18 2023, 9:17 AM
bulbazord marked 2 inline comments as done.May 18 2023, 12:13 PM
bulbazord added inline comments.
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
27

This isn't exactly a copy. DataExtractor::GetAsLLVM creates an entirely new object, it can't be a reference. Luckily it is fairly lightweight -- We just copy a few numbers and a pointer I believe.

56–58
for (const auto &decl : m_decls) {
  if (decl.getCode() == abbrCode)
    return &decl;
}

vs.

auto pos = std::find_if(
     m_decls.begin(), m_decls.end(),
     [abbrCode](const auto &decl) { return decl.getCode() == abbrCode; });
if (pos != m_decls.end())
      return &*pos;

I think it would look worse, personally.

bulbazord marked 2 inline comments as done.

Redo include order in DWARFDebugAbbrev.h to match project guidelines

aprantl added inline comments.May 18 2023, 2:28 PM
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
56–58

Agreed!

lldb/source/Plugins/SymbolFile/DWARF/CMakeLists.txt