[Lldb-commits] [lldb] [lldb] progressive progress reporting for darwin kernel/firmware (PR #98845)
Jason Molenda via lldb-commits
lldb-commits at lists.llvm.org
Tue Jul 16 16:56:02 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/5] [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/5] 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/5] 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)
>From 2e89de95e0388d574e0338b678884056b84753fb Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Tue, 16 Jul 2024 14:56:55 -0700
Subject: [PATCH 4/5] Create a Progress object for kext loading, incremental
updates
In `ParseKextSummaries()` I know how many kexts will be loaded,
create a Progress object there with that number, pass it in to
`LoadImageUsingMemoryModule()`, and issue progressive updates if
the caller passed in an existing Progress object. Else do a one-off
progress update. This give us a 1-out-of-n number for the updates,
e.g.
[30/180] Loading kext: com.apple.Libm 5D39F799-ADEA-3CAE-8A7B-830CE3EAEF72 at 0xfffffe002aa6b98..
Thanks to Chelsea for pointing out how I could use the API more
correctly.
---
.../DynamicLoaderDarwinKernel.cpp | 24 ++++++++++++-------
.../Darwin-Kernel/DynamicLoaderDarwinKernel.h | 4 +++-
2 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
index 6e74aff32d70d..20e5652c65bf8 100644
--- a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
+++ b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
@@ -715,7 +715,7 @@ void DynamicLoaderDarwinKernel::KextImageInfo::SetIsKernel(bool is_kernel) {
}
bool DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
- Process *process) {
+ Process *process, Progress *progress) {
Log *log = GetLog(LLDBLog::DynamicLoader);
if (IsLoaded())
return true;
@@ -769,13 +769,18 @@ bool DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
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());
+
+ std::unique_ptr<Progress> progress_up;
+ if (progress)
+ progress->Increment(1, prog_str.GetString().str());
+ else {
+ if (IsKernel())
+ progress_up = std::make_unique<Progress>("Loading kernel",
+ prog_str.GetString().str());
+ else
+ progress_up = 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()) {
@@ -1358,13 +1363,14 @@ bool DynamicLoaderDarwinKernel::ParseKextSummaries(
std::vector<std::pair<std::string, UUID>> kexts_failed_to_load;
if (number_of_new_kexts_being_added > 0) {
ModuleList loaded_module_list;
+ Progress progress("Loading kext", "", number_of_new_kexts_being_added);
const uint32_t num_of_new_kexts = kext_summaries.size();
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];
if (load_kexts) {
- if (!image_info.LoadImageUsingMemoryModule(m_process)) {
+ if (!image_info.LoadImageUsingMemoryModule(m_process, &progress)) {
kexts_failed_to_load.push_back(std::pair<std::string, UUID>(
kext_summaries[new_kext].GetName(),
kext_summaries[new_kext].GetUUID()));
diff --git a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.h b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.h
index 000c382b2c011..69dd8ca579158 100644
--- a/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.h
+++ b/lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.h
@@ -16,6 +16,7 @@
#include "lldb/Host/SafeMachO.h"
+#include "lldb/Core/Progress.h"
#include "lldb/Target/DynamicLoader.h"
#include "lldb/Target/Process.h"
#include "lldb/Utility/FileSpec.h"
@@ -137,7 +138,8 @@ class DynamicLoaderDarwinKernel : public lldb_private::DynamicLoader {
bool LoadImageAtFileAddress(lldb_private::Process *process);
- bool LoadImageUsingMemoryModule(lldb_private::Process *process);
+ bool LoadImageUsingMemoryModule(lldb_private::Process *process,
+ lldb_private::Progress *progress = nullptr);
bool IsLoaded() { return m_load_process_stop_id != UINT32_MAX; }
>From e3c60eea579d3948156ec5d16f0f429700e8112b Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Tue, 16 Jul 2024 16:55:26 -0700
Subject: [PATCH 5/5] Add a comment why I check for corefiles & clarify msg
---
lldb/source/Core/DynamicLoader.cpp | 2 +-
lldb/source/Target/Process.cpp | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/lldb/source/Core/DynamicLoader.cpp b/lldb/source/Core/DynamicLoader.cpp
index cc57905b8315b..e0db4d8105519 100644
--- a/lldb/source/Core/DynamicLoader.cpp
+++ b/lldb/source/Core/DynamicLoader.cpp
@@ -223,7 +223,7 @@ ModuleSP DynamicLoader::LoadBinaryWithUUIDAndAddress(
FileSpec name_filespec(name);
if (uuid.IsValid()) {
- Progress progress("Locating external symbol file",
+ Progress progress("Locating binary",
prog_str.GetString().str());
// Has lldb already seen a module with this UUID?
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index bc87c2381021b..29cc97b675587 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -2547,6 +2547,8 @@ ModuleSP Process::ReadModuleFromMemory(const FileSpec &file_spec,
if (module_sp) {
Status error;
std::unique_ptr<Progress> progress_up;
+ // Reading an ObjectFile from a local corefile is very fast,
+ // don't print a progress report.
if (!GetCoreFile())
progress_up = std::make_unique<Progress>(
"Reading binary from memory", file_spec.GetFilename().GetString());
More information about the lldb-commits
mailing list