This is an archive of the discontinued LLVM Phabricator instance.

01/06: Merge GetCompileUnit + GetCompileUnitContainingDIEOffset
ClosedPublic

Authored by jankratochvil on May 3 2019, 6:15 AM.

Details

Summary

These two methods are very similar and various refactorizations need to modify both similar ways. Merge them as a template.

One could also just remove GetCompileUnit and make GetCompileUnitContainingDIEOffset to also accept offset of the CU itself (currently it accepts only DIE offsets after the CU header). But that would be less safe regarding some internal sanity checking.

The whole patchset available for testing as: git clone -b debugtypes git://git.jankratochvil.net/lldb

Diff Detail

Repository
rL LLVM

Event Timeline

jankratochvil created this revision.May 3 2019, 6:15 AM
labath added a comment.May 3 2019, 7:10 AM

What you could do instead is have a single function which returns the CU if it is in the range cu.GetOffset() <= input < cu.GetNextUnitOffset(), then the wrapper functions could just perform a more stricter check on the returned cu (I.e. no templates or callbacks).

This may lower the need for this refactoring, but independently of that, I don't believe we should touch this function without simplifying it into something more like

uint32_t index = DW_INVALID_INDEX;
DWARFUnit result = nullptr;
auto pos = llvm::lower_bound(m_compile_units, offset, OffsetLessThanCompileUnitOffset);
if (pos != m_compile_units.end() && ???) {
  index = std::distance(...);
  result = ...
}
if (idx_ptr) *idx_ptr = index;
return result;

as the current implementation is hard to understand for no good reason.

What you could do instead is have a single function which returns the CU if it is in the range cu.GetOffset() <= input < cu.GetNextUnitOffset(), then the wrapper functions could just perform a more stricter check on the returned cu (I.e. no templates or callbacks).

OK, I agree; originally I did not want to touch more code than what I needed for .debug_types.

labath accepted this revision.May 6 2019, 12:08 AM

Thanks, this looks much better.

OK, I agree; originally I did not want to touch more code than what I needed for .debug_types.

I think that "not wanting to touch more code than needed" was the reason why all previous attemtps at debug_types were hacky in one way or another. :)

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp
129–130 ↗(On Diff #198148)

Ah, I think I finally understand the --pos thingy. :)

This revision is now accepted and ready to land.May 6 2019, 12:08 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2019, 5:00 AM