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
Index: source/Target/Target.cpp
===================================================================
--- source/Target/Target.cpp
+++ source/Target/Target.cpp
@@ -1442,7 +1442,8 @@
"Target::SetExecutableModule (executable = '%s')",
executable_sp->GetFileSpec().GetPath().c_str());
- m_images.Append(executable_sp); // The first image is our executable file
+ const bool notify = true;
+ m_images.Append(executable_sp, notify); // The first image is our executable file
// If we haven't set an architecture yet, reset our architecture based on
// what we found in the executable module.
@@ -1482,7 +1483,8 @@
platform_dependent_file_spec = dependent_file_spec;
ModuleSpec module_spec(platform_dependent_file_spec, m_arch.GetSpec());
- ModuleSP image_module_sp(GetSharedModule(module_spec));
+ const bool notify = true;
+ ModuleSP image_module_sp(AddModule(module_spec, notify));
if (image_module_sp) {
ObjectFile *objfile = image_module_sp->GetObjectFile();
if (objfile)
@@ -1984,8 +1986,8 @@
return false;
}
-ModuleSP Target::GetSharedModule(const ModuleSpec &module_spec,
- Status *error_ptr) {
+ModuleSP Target::AddModule(const ModuleSpec &module_spec, bool notify,
+ Status *error_ptr) {
ModuleSP module_sp;
Status error;
@@ -2118,8 +2120,9 @@
Module *old_module_ptr = old_module_sp.get();
old_module_sp.reset();
ModuleList::RemoveSharedModuleIfOrphaned(old_module_ptr);
- } else
- m_images.Append(module_sp);
+ } else {
+ m_images.Append(module_sp, notify);
+ }
} else
module_sp.reset();
}
Index: source/Plugins/Process/minidump/ProcessMinidump.cpp
===================================================================
--- source/Plugins/Process/minidump/ProcessMinidump.cpp
+++ source/Plugins/Process/minidump/ProcessMinidump.cpp
@@ -380,7 +380,8 @@
ModuleSpec module_spec(file_spec, uuid);
module_spec.GetArchitecture() = GetArchitecture();
Status error;
- lldb::ModuleSP module_sp = GetTarget().GetSharedModule(module_spec, &error);
+ const bool notify = true;
+ lldb::ModuleSP module_sp = GetTarget().AddModule(module_spec, notify, &error);
if (!module_sp || error.Fail()) {
// We failed to locate a matching local object file. Fortunately, the
// minidump format encodes enough information about each module's memory
@@ -397,7 +398,8 @@
module_sp = Module::CreateModuleFromObjectFile<PlaceholderObjectFile>(
module_spec, module->base_of_image, module->size_of_image);
- GetTarget().GetImages().Append(module_sp);
+ const bool notify = true;
+ GetTarget().GetImages().Append(module_sp, notify);
}
if (log) {
Index: source/Plugins/Process/elf-core/ProcessElfCore.cpp
===================================================================
--- source/Plugins/Process/elf-core/ProcessElfCore.cpp
+++ source/Plugins/Process/elf-core/ProcessElfCore.cpp
@@ -244,7 +244,8 @@
exe_module_spec.GetFileSpec().SetFile(
m_nt_file_entries[0].path.GetCString(), FileSpec::Style::native);
if (exe_module_spec.GetFileSpec()) {
- exe_module_sp = GetTarget().GetSharedModule(exe_module_spec);
+ const bool notify = true;
+ exe_module_sp = GetTarget().AddModule(exe_module_spec, notify);
if (exe_module_sp)
GetTarget().SetExecutableModule(exe_module_sp, eLoadDependentsNo);
}
Index: source/Plugins/Process/Windows/Common/ProcessWindows.cpp
===================================================================
--- source/Plugins/Process/Windows/Common/ProcessWindows.cpp
+++ source/Plugins/Process/Windows/Common/ProcessWindows.cpp
@@ -915,7 +915,8 @@
FileSystem::Instance().Resolve(executable_file);
ModuleSpec module_spec(executable_file);
Status error;
- module = GetTarget().GetSharedModule(module_spec, &error);
+ const bool notify = true;
+ module = GetTarget().AddModule(module_spec, notify, &error);
if (!module) {
return;
}
Index: source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp
===================================================================
--- source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp
+++ source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp
@@ -68,11 +68,9 @@
// Resolve the module unless we already have one.
if (!module_sp) {
- // 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.
Status error;
- module_sp = m_process->GetTarget().GetSharedModule(module_spec, &error);
+ const bool notify = true;
+ module_sp = m_process->GetTarget().AddModule(module_spec, notify, &error);
if (error.Fail())
return;
}
Index: source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
===================================================================
--- source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
+++ source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
@@ -543,7 +543,8 @@
FileSpec file(info.GetName().GetCString());
ModuleSpec module_spec(file, target.GetArchitecture());
- if (ModuleSP module_sp = target.GetSharedModule(module_spec)) {
+ const bool notify = true;
+ if (ModuleSP module_sp = target.AddModule(module_spec, notify)) {
UpdateLoadedSections(module_sp, LLDB_INVALID_ADDRESS, m_interpreter_base,
false);
return module_sp;
Index: source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
===================================================================
--- source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
+++ source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
@@ -122,7 +122,10 @@
if (!module_sp) {
if (can_create) {
- module_sp = target.GetSharedModule(module_spec);
+ // We'll call Target::ModulesDidLoad after all the modules have been
+ // added to the target, don't let it be called for every one.
+ const bool notify = false;
+ module_sp = target.AddModule(module_spec, notify);
if (!module_sp || module_sp->GetObjectFile() == NULL)
module_sp = m_process->ReadModuleFromMemory(image_info.file_spec,
image_info.address);
@@ -637,7 +640,8 @@
module_spec.SetObjectOffset(objfile->GetFileOffset() +
commpage_section->GetFileOffset());
module_spec.SetObjectSize(objfile->GetByteSize());
- commpage_image_module_sp = target.GetSharedModule(module_spec);
+ const bool notify = true;
+ commpage_image_module_sp = target.AddModule(module_spec, notify);
if (!commpage_image_module_sp ||
commpage_image_module_sp->GetObjectFile() == NULL) {
commpage_image_module_sp = m_process->ReadModuleFromMemory(
Index: source/Plugins/DynamicLoader/Hexagon-DYLD/DynamicLoaderHexagonDYLD.cpp
===================================================================
--- source/Plugins/DynamicLoader/Hexagon-DYLD/DynamicLoaderHexagonDYLD.cpp
+++ source/Plugins/DynamicLoader/Hexagon-DYLD/DynamicLoaderHexagonDYLD.cpp
@@ -199,7 +199,8 @@
return executable;
// TODO: What case is this code used?
- executable = target.GetSharedModule(module_spec);
+ const bool notify = true;
+ executable = target.AddModule(module_spec, notify);
if (executable.get() != target.GetExecutableModulePointer()) {
// Don't load dependent images since we are in dyld where we will know and
// find out about all images that are loaded
Index: source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
===================================================================
--- source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -841,7 +841,8 @@
// the DebugSymbols framework with the UUID to find the binary via its
// search methods.
if (!m_module_sp) {
- m_module_sp = target.GetSharedModule(module_spec);
+ const bool notify = true;
+ m_module_sp = target.AddModule(module_spec, notify);
}
if (IsKernel() && !m_module_sp) {
Index: source/Expression/FunctionCaller.cpp
===================================================================
--- source/Expression/FunctionCaller.cpp
+++ source/Expression/FunctionCaller.cpp
@@ -103,7 +103,8 @@
jit_file.GetFilename() = const_func_name;
jit_module_sp->SetFileSpecAndObjectName(jit_file, ConstString());
m_jit_module_wp = jit_module_sp;
- process->GetTarget().GetImages().Append(jit_module_sp);
+ const bool notify = true;
+ process->GetTarget().GetImages().Append(jit_module_sp, notify);
}
}
if (process && m_jit_start_addr)
Index: source/Core/ModuleList.cpp
===================================================================
--- source/Core/ModuleList.cpp
+++ source/Core/ModuleList.cpp
@@ -151,7 +151,9 @@
}
}
-void ModuleList::Append(const ModuleSP &module_sp) { AppendImpl(module_sp); }
+void ModuleList::Append(const ModuleSP &module_sp, bool notify) {
+ AppendImpl(module_sp, notify);
+}
void ModuleList::ReplaceEquivalent(const ModuleSP &module_sp) {
if (module_sp) {
@@ -177,7 +179,7 @@
}
}
-bool ModuleList::AppendIfNeeded(const ModuleSP &module_sp) {
+bool ModuleList::AppendIfNeeded(const ModuleSP &module_sp, bool notify) {
if (module_sp) {
std::lock_guard<std::recursive_mutex> guard(m_modules_mutex);
collection::iterator pos, end = m_modules.end();
@@ -186,7 +188,7 @@
return false; // Already in the list
}
// Only push module_sp on the list if it wasn't already in there.
- Append(module_sp);
+ Append(module_sp, notify);
return true;
}
return false;
Index: source/Core/DynamicLoader.cpp
===================================================================
--- source/Core/DynamicLoader.cpp
+++ source/Core/DynamicLoader.cpp
@@ -96,7 +96,8 @@
}
if (!executable) {
- executable = target.GetSharedModule(module_spec);
+ const bool notify = true;
+ executable = target.AddModule(module_spec, notify);
if (executable.get() != target.GetExecutableModulePointer()) {
// Don't load dependent images since we are in dyld where we will
// know and find out about all images that are loaded
@@ -166,7 +167,8 @@
return module_sp;
}
- if ((module_sp = target.GetSharedModule(module_spec))) {
+ const bool notify = true;
+ if ((module_sp = target.AddModule(module_spec, notify))) {
UpdateLoadedSections(module_sp, link_map_addr, base_addr,
base_addr_is_offset);
return module_sp;
@@ -202,7 +204,7 @@
return module_sp;
}
- if ((module_sp = target.GetSharedModule(new_module_spec))) {
+ if ((module_sp = target.AddModule(new_module_spec, notify))) {
UpdateLoadedSections(module_sp, link_map_addr, base_addr, false);
return module_sp;
}
Index: source/Commands/CommandObjectTarget.cpp
===================================================================
--- source/Commands/CommandObjectTarget.cpp
+++ source/Commands/CommandObjectTarget.cpp
@@ -395,7 +395,9 @@
debugger.GetTargetList().SetSelectedTarget(target_sp.get());
if (must_set_platform_path) {
ModuleSpec main_module_spec(file_spec);
- ModuleSP module_sp = target_sp->GetSharedModule(main_module_spec);
+ const bool notify = true;
+ ModuleSP module_sp = target_sp->AddModule(main_module_spec,
+ notify);
if (module_sp)
module_sp->SetPlatformFileSpec(remote_file);
}
@@ -2598,7 +2600,9 @@
module_spec.GetSymbolFileSpec() =
m_symbol_file.GetOptionValue().GetCurrentValue();
if (Symbols::DownloadObjectAndSymbolFile(module_spec)) {
- ModuleSP module_sp(target->GetSharedModule(module_spec));
+ const bool notify = true;
+ ModuleSP module_sp(target->AddModule(module_spec,
+ notify));
if (module_sp) {
result.SetStatus(eReturnStatusSuccessFinishResult);
return true;
@@ -2660,7 +2664,8 @@
if (!module_spec.GetArchitecture().IsValid())
module_spec.GetArchitecture() = target->GetArchitecture();
Status error;
- ModuleSP module_sp(target->GetSharedModule(module_spec, &error));
+ const bool notify = true;
+ ModuleSP module_sp(target->AddModule(module_spec, notify, &error));
if (!module_sp) {
const char *error_cstr = error.AsCString();
if (error_cstr)
Index: source/API/SBTarget.cpp
===================================================================
--- source/API/SBTarget.cpp
+++ source/API/SBTarget.cpp
@@ -1591,7 +1591,8 @@
if (symfile)
module_spec.GetSymbolFileSpec().SetFile(symfile, FileSpec::Style::native);
- sb_module.SetSP(target_sp->GetSharedModule(module_spec));
+ const bool notify = true;
+ sb_module.SetSP(target_sp->AddModule(module_spec, notify));
}
return LLDB_RECORD_RESULT(sb_module);
}
@@ -1602,8 +1603,10 @@
lldb::SBModule sb_module;
TargetSP target_sp(GetSP());
- if (target_sp)
- sb_module.SetSP(target_sp->GetSharedModule(*module_spec.m_opaque_up));
+ if (target_sp) {
+ const bool notify = true;
+ sb_module.SetSP(target_sp->AddModule(*module_spec.m_opaque_up, notify));
+ }
return LLDB_RECORD_RESULT(sb_module);
}
Index: include/lldb/Target/Target.h
===================================================================
--- include/lldb/Target/Target.h
+++ include/lldb/Target/Target.h
@@ -505,8 +505,34 @@
static void SetDefaultArchitecture(const ArchSpec &arch);
- lldb::ModuleSP GetSharedModule(const ModuleSpec &module_spec,
- Status *error_ptr = nullptr);
+ //------------------------------------------------------------------
+ /// Add a Module into the Target
+ ///
+ /// Given a ModuleSpec, find a binary satisifying that specification,
+ /// or identify a matching Module already present in the Target,
+ /// and return a shared pointer to it.
+ ///
+ /// \param[in] module_spec
+ /// The criteria that must be matched for the binary being loaded.
+ /// e.g. UUID, architecture, file path.
+ ///
+ /// \param[in] notify
+ /// If notify is true, and the Module is new to this Target,
+ /// Target::ModulesDidLoad will be called.
+ /// If notify is false, it is assumed that the caller is adding
+ /// multiple Modules and will call ModulesDidLoad with the
+ /// full list at the end.
+ /// ModulesDidLoad must be called when a Module/Modules have
+ /// been added to the target, one way or the other.
+ ///
+ /// \param[out] error_ptr
+ /// Optional argument, pointing to a Status object to fill in
+ /// with any results / messages while attempting to find/load
+ /// this binary.
+ //------------------------------------------------------------------
+ lldb::ModuleSP AddModule(const ModuleSpec &module_spec,
+ bool notify,
+ Status *error_ptr = nullptr);
//----------------------------------------------------------------------
// Settings accessors
Index: include/lldb/Core/ModuleList.h
===================================================================
--- include/lldb/Core/ModuleList.h
+++ include/lldb/Core/ModuleList.h
@@ -146,12 +146,20 @@
//------------------------------------------------------------------
/// Append a module to the module list.
///
- /// Appends the module to the collection.
- ///
/// \param[in] module_sp
/// A shared pointer to a module to add to this collection.
+ ///
+ /// \param[in] notify
+ /// If true, and a notifier function is set, the notifier function
+ /// will be called. Defaults to true.
+ ///
+ /// When this ModuleList is the Target's ModuleList, the notifier
+ /// function is Target::ModulesDidLoad -- the call to
+ /// ModulesDidLoad may be deferred when adding multiple Modules
+ /// to the Target, but it must be called at the end,
+ /// before resuming execution.
//------------------------------------------------------------------
- void Append(const lldb::ModuleSP &module_sp);
+ void Append(const lldb::ModuleSP &module_sp, bool notify = true);
//------------------------------------------------------------------
/// Append a module to the module list and remove any equivalent modules.
@@ -165,7 +173,22 @@
//------------------------------------------------------------------
void ReplaceEquivalent(const lldb::ModuleSP &module_sp);
- bool AppendIfNeeded(const lldb::ModuleSP &module_sp);
+ //------------------------------------------------------------------
+ /// Append a module to the module list, if it is not already there.
+ ///
+ /// \param[in] module_sp
+ ///
+ /// \param[in] notify
+ /// If true, and a notifier function is set, the notifier function
+ /// will be called. Defaults to true.
+ ///
+ /// When this ModuleList is the Target's ModuleList, the notifier
+ /// function is Target::ModulesDidLoad -- the call to
+ /// ModulesDidLoad may be deferred when adding multiple Modules
+ /// to the Target, but it must be called at the end,
+ /// before resuming execution.
+ //------------------------------------------------------------------
+ bool AppendIfNeeded(const lldb::ModuleSP &module_sp, bool notify = true);
void Append(const ModuleList &module_list);
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
