[Lldb-commits] [lldb] [lldb] progressive progress reporting for darwin kernel/firmware (PR #98845)
Jason Molenda via lldb-commits
lldb-commits at lists.llvm.org
Mon Jul 15 18:08:18 PDT 2024
https://github.com/jasonmolenda updated https://github.com/llvm/llvm-project/pull/98845
>From cead9ae6de627ee64fb58a829fa3485f526a0afc Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Sun, 14 Jul 2024 16:59:51 -0700
Subject: [PATCH 1/3] [lldb] progressive progress reporting for darwin
kernel/firmware
When doing firmware/kernel debugging, it is frequent that
binaries and debug info need to be retrieved / downloaded,
and the lack of progress reports made for a poor experience,
with lldb seemingly hung while downloading things over the
network. This PR adds progress reports to the critical sites
for these use cases.
---
lldb/source/Core/DynamicLoader.cpp | 27 +++++++++++--
.../DynamicLoaderDarwinKernel.cpp | 39 ++++++++++++-------
2 files changed, 49 insertions(+), 17 deletions(-)
diff --git a/lldb/source/Core/DynamicLoader.cpp b/lldb/source/Core/DynamicLoader.cpp
index 7871be6fc451d..a59136381c23b 100644
--- a/lldb/source/Core/DynamicLoader.cpp
+++ b/lldb/source/Core/DynamicLoader.cpp
@@ -13,6 +13,7 @@
#include "lldb/Core/ModuleList.h"
#include "lldb/Core/ModuleSpec.h"
#include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Progress.h"
#include "lldb/Core/Section.h"
#include "lldb/Symbol/ObjectFile.h"
#include "lldb/Target/MemoryRegionInfo.h"
@@ -195,20 +196,40 @@ ModuleSP DynamicLoader::LoadBinaryWithUUIDAndAddress(
Target &target = process->GetTarget();
Status error;
+ StreamString prog_str;
+ if (!name.empty()) {
+ prog_str << name.str() << " ";
+ }
+ if (uuid.IsValid())
+ prog_str << uuid.GetAsString();
+ if (value_is_offset == 0 && value != LLDB_INVALID_ADDRESS) {
+ prog_str << "at ";
+ prog_str.PutHex64(value);
+ }
+
if (!uuid.IsValid() && !value_is_offset) {
+ Progress progress_memread("Reading load commands from memory",
+ prog_str.GetString().str());
memory_module_sp = ReadUnnamedMemoryModule(process, value, name);
- if (memory_module_sp)
+ if (memory_module_sp) {
uuid = memory_module_sp->GetUUID();
+ if (uuid.IsValid()) {
+ prog_str << " ";
+ prog_str << uuid.GetAsString();
+ }
+ }
}
ModuleSpec module_spec;
module_spec.GetUUID() = uuid;
FileSpec name_filespec(name);
- if (FileSystem::Instance().Exists(name_filespec))
- module_spec.GetFileSpec() = name_filespec;
if (uuid.IsValid()) {
+ Progress progress("Locating external symbol file",
+ prog_str.GetString().str());
+
// Has lldb already seen a module with this UUID?
+ // Or have external lookup enabled in DebugSymbols on macOS.
if (!module_sp)
error = ModuleList::GetSharedModule(module_spec, module_sp, nullptr,
nullptr, nullptr);
diff --git a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
index 8d83937aab668..93eb1e7b9dea9 100644
--- a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -13,6 +13,7 @@
#include "lldb/Core/Module.h"
#include "lldb/Core/ModuleSpec.h"
#include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Progress.h"
#include "lldb/Core/Section.h"
#include "lldb/Interpreter/OptionValueProperties.h"
#include "lldb/Symbol/ObjectFile.h"
@@ -757,6 +758,23 @@ bool DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
const ModuleList &target_images = target.GetImages();
m_module_sp = target_images.FindModule(m_uuid);
+ std::unique_ptr<Progress> progress_up;
+ if (IsKernel()) {
+ StreamString prog_str;
+ // 'mach_kernel' is a fake name we make up to find kernels
+ // that were located by the local filesystem scan.
+ if (GetName() != "mach_kernel")
+ prog_str << GetName() << " ";
+ if (GetUUID().IsValid())
+ prog_str << GetUUID().GetAsString() << " ";
+ if (GetLoadAddress() != LLDB_INVALID_ADDRESS) {
+ prog_str << "at ";
+ prog_str.PutHex64(GetLoadAddress());
+ }
+ progress_up = std::make_unique<Progress>("Loading kernel",
+ prog_str.GetString().str());
+ }
+
// Search for the kext on the local filesystem via the UUID
if (!m_module_sp && m_uuid.IsValid()) {
ModuleSpec module_spec;
@@ -1058,12 +1076,9 @@ void DynamicLoaderDarwinKernel::LoadKernelModuleIfNeeded() {
}
}
}
-
- if (m_kernel.GetLoadAddress() != LLDB_INVALID_ADDRESS) {
- if (!m_kernel.LoadImageUsingMemoryModule(m_process)) {
+ if (m_kernel.GetLoadAddress() != LLDB_INVALID_ADDRESS)
+ if (!m_kernel.LoadImageUsingMemoryModule(m_process))
m_kernel.LoadImageAtFileAddress(m_process);
- }
- }
// The operating system plugin gets loaded and initialized in
// LoadImageUsingMemoryModule when we discover the kernel dSYM. For a core
@@ -1352,14 +1367,17 @@ bool DynamicLoaderDarwinKernel::ParseKextSummaries(
for (uint32_t new_kext = 0; new_kext < num_of_new_kexts; new_kext++) {
if (to_be_added[new_kext]) {
KextImageInfo &image_info = kext_summaries[new_kext];
- bool kext_successfully_added = true;
if (load_kexts) {
+ std::string prog_str = kext_summaries[new_kext].GetName();
+ prog_str += " ";
+ prog_str += kext_summaries[new_kext].GetUUID().GetAsString();
+
+ Progress progress("Loading kext", prog_str);
if (!image_info.LoadImageUsingMemoryModule(m_process)) {
kexts_failed_to_load.push_back(std::pair<std::string, UUID>(
kext_summaries[new_kext].GetName(),
kext_summaries[new_kext].GetUUID()));
image_info.LoadImageAtFileAddress(m_process);
- kext_successfully_added = false;
}
}
@@ -1369,13 +1387,6 @@ bool DynamicLoaderDarwinKernel::ParseKextSummaries(
m_process->GetStopID() == image_info.GetProcessStopId())
loaded_module_list.AppendIfNeeded(image_info.GetModule());
- if (load_kexts) {
- if (kext_successfully_added)
- s.Printf(".");
- else
- s.Printf("-");
- }
-
if (log)
kext_summaries[new_kext].PutToLog(log);
}
>From b78da7e76d947c9742977e851085a372f1825e05 Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Sun, 14 Jul 2024 21:42:21 -0700
Subject: [PATCH 2/3] locate all progress reporting for kexts and kernels in
one method, LoadImageUsingMemoryModule. Remove the other progress reporting
from ParseKextSummaries. Add 0x prefix manually before PutHex64.
In LoadImageUsingMemoryModule add the kext/kernel name to the
ModuleSpec so it will be printed by the "Loading symbol file for
..." progress report in the generic DebugSymbols printer. Remove
the logic that was only calling the Platform GetSharedModule with
the kext/kernel filename if it was PlatformDarwinKernel. First,
it should always be PlatformDarwinKernel already, but even if it's
not, the kext bundle ID or placeholder "mach_kernel" will not match
a real file, the restriction to only use the name for
PlatformDarwinKernel was unnecessary.
If I have a correct UUID for the kext/kernel, don't add a possibly
incorrect ArchSpec to the ModuleSpec.
---
lldb/source/Core/DynamicLoader.cpp | 2 +-
.../DynamicLoaderDarwinKernel.cpp | 55 ++++++++-----------
2 files changed, 24 insertions(+), 33 deletions(-)
diff --git a/lldb/source/Core/DynamicLoader.cpp b/lldb/source/Core/DynamicLoader.cpp
index a59136381c23b..6b9a2337b5e14 100644
--- a/lldb/source/Core/DynamicLoader.cpp
+++ b/lldb/source/Core/DynamicLoader.cpp
@@ -203,7 +203,7 @@ ModuleSP DynamicLoader::LoadBinaryWithUUIDAndAddress(
if (uuid.IsValid())
prog_str << uuid.GetAsString();
if (value_is_offset == 0 && value != LLDB_INVALID_ADDRESS) {
- prog_str << "at ";
+ prog_str << "at 0x";
prog_str.PutHex64(value);
}
diff --git a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
index 93eb1e7b9dea9..6e74aff32d70d 100644
--- a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -758,28 +758,32 @@ bool DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
const ModuleList &target_images = target.GetImages();
m_module_sp = target_images.FindModule(m_uuid);
- std::unique_ptr<Progress> progress_up;
- if (IsKernel()) {
- StreamString prog_str;
- // 'mach_kernel' is a fake name we make up to find kernels
- // that were located by the local filesystem scan.
- if (GetName() != "mach_kernel")
- prog_str << GetName() << " ";
- if (GetUUID().IsValid())
- prog_str << GetUUID().GetAsString() << " ";
- if (GetLoadAddress() != LLDB_INVALID_ADDRESS) {
- prog_str << "at ";
- prog_str.PutHex64(GetLoadAddress());
- }
- progress_up = std::make_unique<Progress>("Loading kernel",
- prog_str.GetString().str());
+ StreamString prog_str;
+ // 'mach_kernel' is a fake name we make up to find kernels
+ // that were located by the local filesystem scan.
+ if (GetName() != "mach_kernel")
+ prog_str << GetName() << " ";
+ if (GetUUID().IsValid())
+ prog_str << GetUUID().GetAsString() << " ";
+ if (GetLoadAddress() != LLDB_INVALID_ADDRESS) {
+ prog_str << "at 0x";
+ prog_str.PutHex64(GetLoadAddress());
}
+ std::unique_ptr<Progress> progress_wp;
+ if (IsKernel())
+ progress_wp = std::make_unique<Progress>("Loading kernel",
+ prog_str.GetString().str());
+ else
+ progress_wp = std::make_unique<Progress>("Loading kext",
+ prog_str.GetString().str());
// Search for the kext on the local filesystem via the UUID
if (!m_module_sp && m_uuid.IsValid()) {
ModuleSpec module_spec;
module_spec.GetUUID() = m_uuid;
- module_spec.GetArchitecture() = target.GetArchitecture();
+ if (!m_uuid.IsValid())
+ module_spec.GetArchitecture() = target.GetArchitecture();
+ module_spec.GetFileSpec() = FileSpec(m_name);
// If the current platform is PlatformDarwinKernel, create a ModuleSpec
// with the filename set to be the bundle ID for this kext, e.g.
@@ -788,17 +792,9 @@ bool DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
// system.
PlatformSP platform_sp(target.GetPlatform());
if (platform_sp) {
- static ConstString g_platform_name(
- PlatformDarwinKernel::GetPluginNameStatic());
- if (platform_sp->GetPluginName() == g_platform_name.GetStringRef()) {
- ModuleSpec kext_bundle_module_spec(module_spec);
- FileSpec kext_filespec(m_name.c_str());
- FileSpecList search_paths = target.GetExecutableSearchPaths();
- kext_bundle_module_spec.GetFileSpec() = kext_filespec;
- platform_sp->GetSharedModule(kext_bundle_module_spec, process,
- m_module_sp, &search_paths, nullptr,
- nullptr);
- }
+ FileSpecList search_paths = target.GetExecutableSearchPaths();
+ platform_sp->GetSharedModule(module_spec, process, m_module_sp,
+ &search_paths, nullptr, nullptr);
}
// Ask the Target to find this file on the local system, if possible.
@@ -1368,11 +1364,6 @@ bool DynamicLoaderDarwinKernel::ParseKextSummaries(
if (to_be_added[new_kext]) {
KextImageInfo &image_info = kext_summaries[new_kext];
if (load_kexts) {
- std::string prog_str = kext_summaries[new_kext].GetName();
- prog_str += " ";
- prog_str += kext_summaries[new_kext].GetUUID().GetAsString();
-
- Progress progress("Loading kext", prog_str);
if (!image_info.LoadImageUsingMemoryModule(m_process)) {
kexts_failed_to_load.push_back(std::pair<std::string, UUID>(
kext_summaries[new_kext].GetName(),
>From 26e87f3b6c828867044820d5c1d934eff1a60df7 Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Mon, 15 Jul 2024 18:06:59 -0700
Subject: [PATCH 3/3] Changes to address Greg's feedback.
Move the logging about reading a binary from memory from
DynamicLoader::LoadBinaryWithUUIDAndAddress into
Process::ReadModuleFromMemory. Rename the progress title
to not be mach-o specific. Only issue the progress update
if this is a non-corefile Process; lldb will read the
binary out of the corefile so fast that logging is not
useful.
---
lldb/source/Core/DynamicLoader.cpp | 2 --
lldb/source/Target/Process.cpp | 6 ++++++
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/lldb/source/Core/DynamicLoader.cpp b/lldb/source/Core/DynamicLoader.cpp
index 6b9a2337b5e14..cc57905b8315b 100644
--- a/lldb/source/Core/DynamicLoader.cpp
+++ b/lldb/source/Core/DynamicLoader.cpp
@@ -208,8 +208,6 @@ ModuleSP DynamicLoader::LoadBinaryWithUUIDAndAddress(
}
if (!uuid.IsValid() && !value_is_offset) {
- Progress progress_memread("Reading load commands from memory",
- prog_str.GetString().str());
memory_module_sp = ReadUnnamedMemoryModule(process, value, name);
if (memory_module_sp) {
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index b91446e1c0e49..bc87c2381021b 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -21,6 +21,7 @@
#include "lldb/Core/Module.h"
#include "lldb/Core/ModuleSpec.h"
#include "lldb/Core/PluginManager.h"
+#include "lldb/Core/Progress.h"
#include "lldb/Expression/DiagnosticManager.h"
#include "lldb/Expression/DynamicCheckerFunctions.h"
#include "lldb/Expression/UserExpression.h"
@@ -2545,6 +2546,11 @@ ModuleSP Process::ReadModuleFromMemory(const FileSpec &file_spec,
ModuleSP module_sp(new Module(file_spec, ArchSpec()));
if (module_sp) {
Status error;
+ std::unique_ptr<Progress> progress_up;
+ if (!GetCoreFile())
+ progress_up = std::make_unique<Progress>(
+ "Reading binary from memory", file_spec.GetFilename().GetString());
+
ObjectFile *objfile = module_sp->GetMemoryObjectFile(
shared_from_this(), header_addr, error, size_to_read);
if (objfile)
More information about the lldb-commits
mailing list