[Lldb-commits] [PATCH] D42955: Make Module::GetSectionList output consistent

[Pavel Labath via Phabricator via lldb-commits]

labath updated this revision to Diff 135971.
labath added a comment.
Herald added subscribers: JDevlieghere, mgorny.

This version moves the logic for the building of unified section list into the
symbol vendor plugin.

The idea was to keep the construction of section lists of individual object
files inside the ObjectFile object, and then have the SymbolVendor do the
merging. This was easy to achieve for the SymbolVendorELF, and I'm quite happy
about how things look like there.

However, I had a lot of trouble with making this work for the MachO
objectfile+symbolvendor combo, because there the section building is
intertwined with the load command parsing and the building of the unified
sections list. In particular, the parse function seems to be doing some dodgy
things like taking a section from the "unified" list and inserting it into
per-object-file list. So, here I had to leave the building of the unified list
in the ObjectFile class, and the SymbolVendor just orchestrates the functions
to be called in the right order (for this I needed to cast the ObjectFile into
ObjectFileMachO, which seemingly adds a new dependency to the symbol vendor,
but this is not really true, as the vendor was already checking the type of the
object file via the GetPluginName() function). It would be great if someone
with more MachO knowledge could look at the CreateSections function and split
it up into more manageable chunks.

The other sub-optimal design decision I had to make was due to the mutual
recursion of the Module, SymbolVendor and SymbolFile classes: SymbolVendor
creates a SymbolFile class, which expect that the Module's section list is
already constructed, but SymbolVendor is the one constructing the list. This
means that I couldn't just construct the SymbolVendor and have the Module ask
it for the section list, but I had to make the vendor directly update the
section list within the module (before constructing the symbolfile class).

On the plus side, I really like that I was able to remove the
"update_module_section_list" argument from the ObjectFile::GetSectionList
function, which was very dodgy as well, because whether the module's section
list was updated, depended on the value of the argument of the *first* call of
that function.

All in all, I think patch cleans up the unified section list handling, but only
by a small bit. Let me know what you think.


https://reviews.llvm.org/D42955

Files:
  include/lldb/Core/Module.h
  include/lldb/Symbol/ObjectFile.h
  include/lldb/Symbol/SymbolVendor.h
  include/lldb/lldb-private-interfaces.h
  lit/Modules/Inputs/stripped.yaml
  lit/Modules/Inputs/unstripped.yaml
  lit/Modules/lit.local.cfg
  lit/Modules/unified-section-list.test
  lit/lit.cfg
  source/Core/Module.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp
  source/Plugins/ObjectFile/ELF/ObjectFileELF.h
  source/Plugins/ObjectFile/JIT/ObjectFileJIT.cpp
  source/Plugins/ObjectFile/JIT/ObjectFileJIT.h
  source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
  source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.h
  source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp
  source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.cpp
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDwp.cpp
  source/Plugins/SymbolVendor/CMakeLists.txt
  source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
  source/Plugins/SymbolVendor/ELF/SymbolVendorELF.h
  source/Plugins/SymbolVendor/MacOSX/CMakeLists.txt
  source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp
  source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.h
  source/Symbol/ObjectFile.cpp
  source/Symbol/SymbolFile.cpp
  source/Symbol/SymbolVendor.cpp
  tools/lldb-test/lldb-test.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D42955.135971.patch
Type: text/x-patch
Size: 32261 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20180226/bf56c4b2/attachment-0001.bin>