jasonmolenda created this revision.
jasonmolenda added reviewers: jingham, bulbazord.
jasonmolenda added a project: LLDB.
Herald added a subscriber: JDevlieghere.
Herald added a project: All.
jasonmolenda requested review of this revision.
Herald added a subscriber: lldb-commits.
I had two bugs with lldb's mach-o corefile loader using the "all image infos"
LC_NOTE, and while working on those, I was looking closely at the code and
found several perf mistakes where I would re-do work multiple times. This
patch unfortunately ends up doing several things at once.
1. If we load binaries in ObjectFileMachO via the "all image infos" LC_NOTE,
`ProcessMachCore::LoadBinariesViaMetadata()` should detect that binaries were
loaded by it, and not then scan for a `dyld` in the corefile, throw away the
Target image list already constructed, and try to recreate it (and possibly not
be successful). Also fix a mistake for the "main bin spec" LC_NOTE with
userland process type in `ProcessMachCore::LoadBinariesViaMetadata`, noting
that this is a userland dyld situation. It ProcessMachCore currently falls
back to a scan, and finds dyld by exhaustive search, but that's unnecessary
work.
2. Binaries would first be loaded into the Target at their mach-header vm
address, then ObjectFileMachO would load each binary's segments at the correct
vmaddrs. With binaries in the Darwin shared cache, the actual layout in memory
has TEXT and DATA in discontiguous vm ranges. But when we load all the
segments in the Target at the mach header address contiguously, we get "address
0x... maps to more than one section" warnings. The addresses for the file
would then be set correctly on a segment-by-segment basis in ObjectFileMachO,
but the warnings were displayed to the user. For this, I add a
`set_address_in_target` bool argument to
`DynamicLoader::LoadBinaryWithUUIDAndAddress` to control whether it sets the
address of the binary it finds in the Target, or if it leaves this to the
caller. In `ObjectFileMachO::LoadCoreFileImages` when a binary has per-segment
vmaddrs, it sets a local `set_load_address` and we pass that in to our calls.
Update other callers to all pass `set_load_address=true` (e.g.
`ProcessGDBRemote::LoadStubBinaries`) to
`DynamicLoader::LoadBinaryWithUUIDAndAddress` to preserve existing behavior for
them.
3. Perf fix in `ObjectFileMachO::LoadCoreFileImages` where it would try to find
the binary for a UUID multiple times as different approaches were unsuccessful.
These attempts can be slow, and doing them multiple times can be more slow.
Also a lot of code duplication if we have `image.load_address` or
`image.slide`, but most lldb internal API take a uint64_t value and a "value is
offset/slide" bool specifically to avoid this kind of duplication..
4. A previously committed perf fix https://reviews.llvm.org/D150621 was also
driven by this investigation, but that patch was easily chiseled off into a
separate patch, so I did that one already.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D150928
Files:
lldb/include/lldb/Target/DynamicLoader.h
lldb/source/Core/DynamicLoader.cpp
lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
Index: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
===================================================================
--- lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
+++ lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp
@@ -252,20 +252,19 @@
m_mach_kernel_addr = objfile_binary_value;
m_dyld_plugin_name = DynamicLoaderDarwinKernel::GetPluginNameStatic();
found_main_binary_definitively = true;
+ } else if (type == ObjectFile::eBinaryTypeUser) {
+ m_dyld_addr = objfile_binary_value;
+ m_dyld_plugin_name = DynamicLoaderMacOSXDYLD::GetPluginNameStatic();
} else {
const bool force_symbol_search = true;
const bool notify = true;
if (DynamicLoader::LoadBinaryWithUUIDAndAddress(
this, llvm::StringRef(), objfile_binary_uuid,
objfile_binary_value, objfile_binary_value_is_offset,
- force_symbol_search, notify)) {
+ force_symbol_search, notify, true)) {
found_main_binary_definitively = true;
m_dyld_plugin_name = DynamicLoaderStatic::GetPluginNameStatic();
}
- if (type == ObjectFile::eBinaryTypeUser) {
- m_dyld_addr = objfile_binary_value;
- m_dyld_plugin_name = DynamicLoaderMacOSXDYLD::GetPluginNameStatic();
- }
}
}
@@ -316,7 +315,7 @@
const bool notify = true;
if (DynamicLoader::LoadBinaryWithUUIDAndAddress(
this, llvm::StringRef(), ident_uuid, ident_binary_addr,
- value_is_offset, force_symbol_search, notify)) {
+ value_is_offset, force_symbol_search, notify, true)) {
found_main_binary_definitively = true;
m_dyld_plugin_name = DynamicLoaderStatic::GetPluginNameStatic();
}
@@ -325,7 +324,10 @@
// Finally, load any binaries noted by "load binary" LC_NOTEs in the
// corefile
- core_objfile->LoadCoreFileImages(*this);
+ if (core_objfile->LoadCoreFileImages(*this)) {
+ found_main_binary_definitively = true;
+ m_dyld_plugin_name = DynamicLoaderStatic::GetPluginNameStatic();
+ }
// LoadCoreFileImges may have set the dynamic loader, e.g. in
// PlatformDarwinKernel::LoadPlatformBinaryAndSetup().
Index: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
===================================================================
--- lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -997,7 +997,7 @@
const bool notify = true;
DynamicLoader::LoadBinaryWithUUIDAndAddress(
this, "", standalone_uuid, standalone_value,
- standalone_value_is_offset, force_symbol_search, notify);
+ standalone_value_is_offset, force_symbol_search, notify, true);
}
}
@@ -1026,9 +1026,9 @@
const bool force_symbol_search = true;
// Second manually load this binary into the Target.
- DynamicLoader::LoadBinaryWithUUIDAndAddress(this, llvm::StringRef(), uuid,
- addr, value_is_slide,
- force_symbol_search, notify);
+ DynamicLoader::LoadBinaryWithUUIDAndAddress(
+ this, llvm::StringRef(), uuid, addr, value_is_slide,
+ force_symbol_search, notify, true);
}
}
}
Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===================================================================
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -6955,62 +6955,35 @@
continue;
}
- // If this binary is currently executing, we want to force a
- // possibly expensive search for the binary and its dSYM.
- if (image.currently_executing && image.uuid.IsValid()) {
- ModuleSpec module_spec;
- module_spec.GetUUID() = image.uuid;
- Symbols::DownloadObjectAndSymbolFile(module_spec, error, true);
- if (FileSystem::Instance().Exists(module_spec.GetFileSpec())) {
- module_sp = process.GetTarget().GetOrCreateModule(module_spec, false);
- process.GetTarget().GetImages().AppendIfNeeded(module_sp,
- false /* notify */);
- }
+ ModuleSpec module_spec;
+ module_spec.GetUUID() = image.uuid;
+ bool value_is_offset = image.load_address == LLDB_INVALID_ADDRESS;
+ uint64_t value = value_is_offset ? image.slide : image.load_address;
+ if (value_is_offset && value == LLDB_INVALID_ADDRESS) {
+ // We have neither address nor slide; so we will find the binary
+ // by UUID and load it at slide/offset 0.
+ value = 0;
}
- // We have an address, that's the best way to discover the binary.
- if (!module_sp && image.load_address != LLDB_INVALID_ADDRESS) {
+ if (image.uuid.IsValid()) {
+ const bool set_load_address = image.segment_load_addresses.size() == 0;
module_sp = DynamicLoader::LoadBinaryWithUUIDAndAddress(
- &process, image.filename, image.uuid, image.load_address,
- false /* value_is_offset */, image.currently_executing,
- false /* notify */);
- if (module_sp) {
- // We've already set the load address in the Target,
- // don't do any more processing on this module.
- added_modules.Append(module_sp, false /* notify */);
- continue;
- }
- }
-
- // If we have a slide, we need to find the original binary
- // by UUID, then we can apply the slide value.
- if (!module_sp && image.uuid.IsValid() &&
- image.slide != LLDB_INVALID_ADDRESS) {
- module_sp = DynamicLoader::LoadBinaryWithUUIDAndAddress(
- &process, image.filename, image.uuid, image.slide,
- true /* value_is_offset */, image.currently_executing,
- false /* notify */);
- if (module_sp) {
- // We've already set the load address in the Target,
- // don't do any more processing on this module.
- added_modules.Append(module_sp, false /* notify */);
- continue;
- }
+ &process, image.filename, image.uuid, value, value_is_offset,
+ image.currently_executing, false /* notify */, set_load_address);
}
// Try to find the binary by UUID or filename on the local
// filesystem or in lldb's global module cache.
if (!module_sp) {
Status error;
- ModuleSpec module_spec;
- if (image.uuid.IsValid())
- module_spec.GetUUID() = image.uuid;
if (!image.filename.empty())
module_spec.GetFileSpec() = FileSpec(image.filename.c_str());
module_sp =
process.GetTarget().GetOrCreateModule(module_spec, false, &error);
- process.GetTarget().GetImages().AppendIfNeeded(module_sp,
- false /* notify */);
+ if (module_sp) {
+ process.GetTarget().GetImages().AppendIfNeeded(module_sp,
+ false /* notify */);
+ }
}
// We have a ModuleSP to load in the Target. Load it at the
@@ -7037,39 +7010,16 @@
}
}
}
- } else if (image.load_address != LLDB_INVALID_ADDRESS) {
- if (log) {
- std::string uuidstr = image.uuid.GetAsString();
- log->Printf("ObjectFileMachO::LoadCoreFileImages adding binary '%s' "
- "UUID %s with load address 0x%" PRIx64,
- image.filename.c_str(), uuidstr.c_str(),
- image.load_address);
- }
- const bool address_is_slide = false;
- bool changed = false;
- module_sp->SetLoadAddress(process.GetTarget(), image.load_address,
- address_is_slide, changed);
- } else if (image.slide != 0) {
- if (log) {
- std::string uuidstr = image.uuid.GetAsString();
- log->Printf("ObjectFileMachO::LoadCoreFileImages adding binary '%s' "
- "UUID %s with slide amount 0x%" PRIx64,
- image.filename.c_str(), uuidstr.c_str(), image.slide);
- }
- const bool address_is_slide = true;
- bool changed = false;
- module_sp->SetLoadAddress(process.GetTarget(), image.slide,
- address_is_slide, changed);
} else {
if (log) {
std::string uuidstr = image.uuid.GetAsString();
log->Printf("ObjectFileMachO::LoadCoreFileImages adding binary '%s' "
- "UUID %s at its file address, no slide applied",
- image.filename.c_str(), uuidstr.c_str());
+ "UUID %s with %s 0x%" PRIx64,
+ image.filename.c_str(), uuidstr.c_str(),
+ value_is_offset ? "slide" : "load address", value);
}
- const bool address_is_slide = true;
bool changed = false;
- module_sp->SetLoadAddress(process.GetTarget(), 0, address_is_slide,
+ module_sp->SetLoadAddress(process.GetTarget(), value, value_is_offset,
changed);
}
}
Index: lldb/source/Core/DynamicLoader.cpp
===================================================================
--- lldb/source/Core/DynamicLoader.cpp
+++ lldb/source/Core/DynamicLoader.cpp
@@ -187,7 +187,8 @@
ModuleSP DynamicLoader::LoadBinaryWithUUIDAndAddress(
Process *process, llvm::StringRef name, UUID uuid, addr_t value,
- bool value_is_offset, bool force_symbol_search, bool notify) {
+ bool value_is_offset, bool force_symbol_search, bool notify,
+ bool set_address_in_target) {
ModuleSP memory_module_sp;
ModuleSP module_sp;
PlatformSP platform_sp = process->GetTarget().GetPlatform();
@@ -212,9 +213,9 @@
// If we haven't found a binary, or we don't have a SymbolFile, see
// if there is an external search tool that can find it.
- if (force_symbol_search &&
- (!module_sp || !module_sp->GetSymbolFileFileSpec())) {
- Symbols::DownloadObjectAndSymbolFile(module_spec, error, true);
+ if (!module_sp || !module_sp->GetSymbolFileFileSpec()) {
+ Symbols::DownloadObjectAndSymbolFile(module_spec, error,
+ force_symbol_search);
if (FileSystem::Instance().Exists(module_spec.GetFileSpec())) {
module_sp = std::make_shared<Module>(module_spec);
}
@@ -239,25 +240,34 @@
target.GetImages().AppendIfNeeded(module_sp, false);
bool changed = false;
- if (module_sp->GetObjectFile()) {
- if (value != LLDB_INVALID_ADDRESS) {
- LLDB_LOGF(log, "Loading binary UUID %s at %s 0x%" PRIx64,
- uuid.GetAsString().c_str(),
- value_is_offset ? "offset" : "address", value);
- module_sp->SetLoadAddress(target, value, value_is_offset, changed);
+ if (set_address_in_target) {
+ if (module_sp->GetObjectFile()) {
+ if (value != LLDB_INVALID_ADDRESS) {
+ LLDB_LOGF(log,
+ "DynamicLoader::LoadBinaryWithUUIDAndAddress Loading "
+ "binary UUID %s at %s 0x%" PRIx64,
+ uuid.GetAsString().c_str(),
+ value_is_offset ? "offset" : "address", value);
+ module_sp->SetLoadAddress(target, value, value_is_offset, changed);
+ } else {
+ // No address/offset/slide, load the binary at file address,
+ // offset 0.
+ LLDB_LOGF(log,
+ "DynamicLoader::LoadBinaryWithUUIDAndAddress Loading "
+ "binary UUID %s at file address",
+ uuid.GetAsString().c_str());
+ module_sp->SetLoadAddress(target, 0, true /* value_is_slide */,
+ changed);
+ }
} else {
- // No address/offset/slide, load the binary at file address,
- // offset 0.
- LLDB_LOGF(log, "Loading binary UUID %s at file address",
- uuid.GetAsString().c_str());
+ // In-memory image, load at its true address, offset 0.
+ LLDB_LOGF(log,
+ "DynamicLoader::LoadBinaryWithUUIDAndAddress Loading binary "
+ "UUID %s from memory at address 0x%" PRIx64,
+ uuid.GetAsString().c_str(), value);
module_sp->SetLoadAddress(target, 0, true /* value_is_slide */,
changed);
}
- } else {
- // In-memory image, load at its true address, offset 0.
- LLDB_LOGF(log, "Loading binary UUID %s from memory at address 0x%" PRIx64,
- uuid.GetAsString().c_str(), value);
- module_sp->SetLoadAddress(target, 0, true /* value_is_slide */, changed);
}
if (notify) {
Index: lldb/include/lldb/Target/DynamicLoader.h
===================================================================
--- lldb/include/lldb/Target/DynamicLoader.h
+++ lldb/include/lldb/Target/DynamicLoader.h
@@ -256,11 +256,21 @@
/// to the Target. The caller may prefer to batch up these when loading
/// multiple binaries.
///
+ /// \param[in] set_address_in_target
+ /// Whether the address of the binary should be set in the Target if it
+ /// is added. The caller may want to set the section addresses
+ /// individually, instead of loading the binary the entire based on the
+ /// start address or slide. The caller is responsible for setting the
+ /// load address for the binary or its segments in the Target if it passes
+ /// true.
+ ///
/// \return
/// Returns a shared pointer for the Module that has been added.
- static lldb::ModuleSP LoadBinaryWithUUIDAndAddress(
- Process *process, llvm::StringRef name, UUID uuid, lldb::addr_t value,
- bool value_is_offset, bool force_symbol_search, bool notify);
+ static lldb::ModuleSP
+ LoadBinaryWithUUIDAndAddress(Process *process, llvm::StringRef name,
+ UUID uuid, lldb::addr_t value,
+ bool value_is_offset, bool force_symbol_search,
+ bool notify, bool set_address_in_target);
/// Get information about the shared cache for a process, if possible.
///
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
