[lldb-dev] RFC: Removing SymbolVendor indirection
Greg Clayton via lldb-dev
lldb-dev at lists.llvm.org
Fri Jul 12 14:17:17 PDT 2019
Pavel stopped by a few days ago and we discussed this. SymbolVendor was originally made to allow one or more object files to be used to provide the most complete view of a program when debugging, but what happened is we put all of the "using multiple files" kind of support down into the SymbolFile level where it coordinates with the Module (via common sections lists and symbols). So the SymbolVendor level ends up being used to locate symbols mostly these days. So I am fine with looking at a patch that splits this up and possibly renames or moves the SymbolVendor functionality that locates symbol files to another class.
CC'ing Jim in case he has anything he wants to chime in with.
Greg
> On Jun 11, 2019, at 6:11 AM, Pavel Labath via lldb-dev <lldb-dev at lists.llvm.org> wrote:
>
> 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(-)
> _______________________________________________
> lldb-dev mailing list
> lldb-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
More information about the lldb-dev
mailing list