[lldb-dev] RFC: Removing SymbolVendor indirection
Pavel Labath via lldb-dev
lldb-dev at lists.llvm.org
Tue Jun 11 06:11:56 PDT 2019
Hello all,
we currently have an almost identical set of about 20-odd functions on
ModuleList, Module, SymbolVendor and SymbolFile classes. The only thing
that the SymbolVendor functions are doing is taking the Module mutex,
and then calling the equivalent SymbolFile function to do the work. The
only useful task the objects of this class do is to hold the list of
parsed compile units and types.
It also seems like at some point the intention was for the SymbolVendor
to be able to host multiple SymbolFiles, but right now nobody makes use
of that feature.
Therefore, I propose to remove the SymbolVendor indirection and have
Module functions (and everybody else) call the SymbolFile directly when
they need to.
Holding the compile unit and type lists is something that can be easily
implemented in the base SymbolFile class instead of the symbol vendor.
In fact, I would say that it is more natural to implement it that way,
as it would change code like
m_obj_file->GetModule()->GetSymbolVendor()->SetCompileUnitAtIndex(
dwarf_cu->GetID(), cu_sp);
into
SetCompileUnitAtIndex(dwarf_cu->GetID(), cu_sp);
Locking the module mutex is also a responsibility that can be handled by
the SymbolFile class. I would say that this also makes things cleaner,
because even right now, the SymbolFile instances need to be aware of
locking. E.g., some functions in SymbolFileDWARF (which is the only
battle-tested symbol file implementation right now) take the module
mutex themselves, while others assert that the module mutex is already
taken.
The ability to have more than one SymbolFile for a module sounds useful,
but: a) nobody uses it; b) I'm not sure it would really work in the
current form. The reason for (b) is that the SymbolFile interface
assumes a fixed numbering of compile units starting with zero. As such,
this would require two SymbolFile instances to be aware of themselves
anyway, in order to present a coherent compile unit list to the rest of
lldb.
I don't think that the removal of SymbolVendor indirection would remove
the possibility of multiple SymbolFiles though. Should anyone wish to do
so, he can always implement a CombiningSymbolFile class, which will
contain other symbol files, and delegate to them. This is kind of what
is already happening, with SymbolFileDWARFDebugMap, and the external
module lists in SymbolFileDWARF.
I already have a proof of concept patch which implements this idea. It's
not a particularly big one -- it touches about 1k lines of code. If we
agree this is the way to go, I can clean it up and submit for review as
a sequence of smaller patches.
regards,
pavel
PS: I am not saying anything about the role of the SymbolVendor in
finding the symbol files. This is because I am not planning to change
that in any way. The idea is that the SymbolVendor will still be
responsible for finding the symbol file, but it will then return the
symbol file directly, instead of wrapping it in a SymbolVendor instance.
PPS: To get an idea of the changes involved, here's a stat of the
changes required. Most of the changes are in SymbolVendor.cpp, and
involve deleting code. The changes in other files are mostly mechanical
and involve changing code fetching the SymbolVendor to access the
SymbolFile directly.
include/lldb/Core/Module.h | 43 +-
include/lldb/Symbol/SymbolFile.h | 39 +-
include/lldb/Symbol/SymbolVendor.h | 135 +-----
include/lldb/Symbol/Type.h | 2 -
include/lldb/lldb-private-interfaces.h | 4 +-
source/API/SBCompileUnit.cpp | 9 +-
source/API/SBModule.cpp | 37 +-
source/Commands/CommandObjectTarget.cpp | 173 ++++----
source/Core/Address.cpp | 38 +-
source/Core/Module.cpp | 121 +++---
source/Core/SearchFilter.cpp | 8 +-
source/Expression/IRExecutionUnit.cpp | 6 +-
.../MacOSX-DYLD/DynamicLoaderMacOS.cpp | 27 +-
.../ExpressionParser/Clang/ClangASTSource.cpp | 27 +-
source/Plugins/Platform/MacOSX/PlatformDarwin.cpp | 196 +++++----
.../SymbolFile/Breakpad/SymbolFileBreakpad.cpp | 45 +-
.../SymbolFile/Breakpad/SymbolFileBreakpad.h | 15 +-
.../SymbolFile/DWARF/DWARFASTParserClang.cpp | 16 +-
.../Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp | 125 +++---
source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h | 14 +-
.../SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp | 105 ++---
.../SymbolFile/DWARF/SymbolFileDWARFDebugMap.h | 10 +-
.../SymbolFile/DWARF/SymbolFileDWARFDwo.cpp | 13 +-
.../Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h | 1 -
.../SymbolFile/NativePDB/SymbolFileNativePDB.cpp | 47 ++-
.../SymbolFile/NativePDB/SymbolFileNativePDB.h | 11 +-
source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp | 104 ++---
source/Plugins/SymbolFile/PDB/SymbolFilePDB.h | 15 +-
.../Plugins/SymbolFile/Symtab/SymbolFileSymtab.cpp | 28 +-
.../Plugins/SymbolFile/Symtab/SymbolFileSymtab.h | 12 +-
.../Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp | 80 ++--
source/Plugins/SymbolVendor/ELF/SymbolVendorELF.h | 15 +-
source/Symbol/Block.cpp | 6 +-
source/Symbol/CompileUnit.cpp | 35 +-
source/Symbol/Function.cpp | 20 +-
source/Symbol/SymbolFile.cpp | 103 ++++-
source/Symbol/SymbolVendor.cpp | 451 +-------------
source/Symbol/Type.cpp | 2 -
source/Symbol/UnwindTable.cpp | 6 +-
tools/lldb-test/lldb-test.cpp | 54 ++-
unittests/Core/CMakeLists.txt | 2 +-
unittests/Core/MangledTest.cpp | 6 +-
unittests/ObjectFile/ELF/CMakeLists.txt | 2 +-
unittests/ObjectFile/ELF/TestObjectFileELF.cpp | 6 +-
unittests/Symbol/CMakeLists.txt | 1 +
unittests/Symbol/TestDWARFCallFrameInfo.cpp | 3 +
.../SymbolFile/DWARF/SymbolFileDWARFTests.cpp | 7 +-
unittests/Target/CMakeLists.txt | 1 +
unittests/Target/ModuleCacheTest.cpp | 3 +
49 files changed, 833 insertions(+), 1396 deletions(-)
More information about the lldb-dev
mailing list