This is an archive of the discontinued LLVM Phabricator instance.

SymbolVendor: Move compile unit handling into the SymbolFile class
ClosedPublic

Authored by labath on Jul 22 2019, 6:42 AM.

Details

Summary

SymbolFile classes are responsible for creating CompileUnit instances
and they already need to have a notion of the id<->CompileUnit mapping
(because of APIs like ParseCompileUnitAtIndex). However, the
SymbolVendor has remained as the thing responsible for caching created
units (which the SymbolFiles were calling via convoluted constructs like
"m_obj_file->GetModule()->GetSymbolVendor()->SetCompileUnitAtIndex(...)").

This patch moves the responsibility of caching the units into the
SymbolFile class. It does this by moving the implementation of
SymbolVendor::{GetNumCompileUnits,GetCompileUnitAtIndex} into the
equivalent SymbolFile functions. The SymbolVendor functions become just
a passthrough much like the rest of SymbolVendor.

The original implementations of SymbolFile::GetNumCompileUnits is moved
to "CalculateNumCompileUnits", and are made protected, as the "Get"
function is the external api of the class.
SymbolFile::ParseCompileUnitAtIndex is made protected for the same
reason.

This is the first step in removing the SymbolVendor indirection, as
proposed in
http://lists.llvm.org/pipermail/lldb-dev/2019-June/015071.html. After
removing all interesting logic from the SymbolVendor class, I'll proceed
with removing the indirection itself.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Jul 22 2019, 6:42 AM

Looks good. Just one question about caching the calculated number of compile units.

source/Symbol/SymbolFile.cpp
174–182 ↗(On Diff #211079)

For symbol files without compile units we will end up calling into CalculateNumCompileUnits() many tines. Can we set a bool to indicate we have already calculated the number of compile units to avoid this?

JDevlieghere accepted this revision.Jul 22 2019, 9:47 AM
This revision is now accepted and ready to land.Jul 22 2019, 9:47 AM
labath updated this revision to Diff 211265.Jul 23 2019, 2:20 AM
  • Make the compile unit list an Optional<vector> to have an explicit way of saying "we've already tried to compute the number of units and it was zero"
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2019, 2:24 AM