[Lldb-commits] [PATCH] D60172: Renamed Target::GetSharedModule to AddModule, allow for ModulesDidLoad to be delayed when batch adding Modules

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda created this revision.
jasonmolenda added reviewers: clayborg, jingham.
Herald added a subscriber: abidh.
Herald added a project: LLDB.

I'm addressing a perf issue where DynamicLoaderDarwin has been notified that a batch of solibs have been loaded.  It adds them to the target one-by-one with Target::GetSharedModule(), and then calls Target::ModulesDidLoad() to give breakpoints a chance to evaluate for new locations, etc.  One of the things we do on ModulesDidLoad is call ProcessGDBRemote::ModulesDidLoad which will send the qSymbol packet to the gdb remote serial protocol stub if it indicates it wants to know a symbol address.  Currently, because Target::GetSharedModule() calls ModulesDidLoad, we will send the qSymbol packet many times - an app with hundreds of solibs are not unusual.

This patch renames Target::GetSharedModule to Target::AddModule() which better reflects what it actually does -- given a ModuleSpec set of criteria, it finds that binary on the local system and adds it to the Target, if it isn't already present.  This method name has confused all of us at one point or another.  As DynamicLoaderWindowsDYLD notes,

  // Confusingly, there is no Target::AddSharedModule.  Instead, calling
  // GetSharedModule() with a new module will add it to the module list and
  // return a corresponding ModuleSP.

It adds a flag to Target::AddModule to suppress notification of the new Module (i.e. don't call ModulesDidLoad) - if the caller does this, the caller must call Target::ModulesDidLoad before resuming execution.  I added a description of the method in Target.h to make this clear.

I also had to add flag to ModuleList::Append and ModuleList::AppendIfNeeded.  I made these have default values because many uses of this are in a loop creating a standalone ModuleList, and forcing all of them to specify the notify boolean was nonintuitive for those users.  When a ModuleList is a part of the Target, it has notifier callback functions, but when it is a standalone object, it doesn't.

I'm trying to think of how to test this -- but the problem I'm directly trying to address, the qSymbol packet being sent for every solib, instead of the # of groups of solib's that are loaded, is something we don't track today.  I'll continue to think about it.

rdar://problem/48293064


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D60172

Files:
  include/lldb/Core/ModuleList.h
  include/lldb/Target/Target.h
  source/API/SBTarget.cpp
  source/Commands/CommandObjectTarget.cpp
  source/Core/DynamicLoader.cpp
  source/Core/ModuleList.cpp
  source/Expression/FunctionCaller.cpp
  source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
  source/Plugins/DynamicLoader/Hexagon-DYLD/DynamicLoaderHexagonDYLD.cpp
  source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
  source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
  source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp
  source/Plugins/Process/Windows/Common/ProcessWindows.cpp
  source/Plugins/Process/elf-core/ProcessElfCore.cpp
  source/Plugins/Process/minidump/ProcessMinidump.cpp
  source/Target/Target.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D60172.193418.patch
Type: text/x-patch
Size: 18278 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20190403/4e1f546c/attachment-0001.bin>