Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[lldb] progressive progress reporting for darwin kernel/firmware #98845

Merged
merged 8 commits into from
Jul 17, 2024
Merged

[lldb] progressive progress reporting for darwin kernel/firmware #98845

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cead9ae
[lldb] progressive progress reporting for darwin kernel/firmware
jasonmolenda Jul 14, 2024
b78da7e
locate all progress reporting for kexts and kernels in one method,
jasonmolenda Jul 15, 2024
26e87f3
Changes to address Greg's feedback.
jasonmolenda Jul 16, 2024
2e89de9
Create a Progress object for kext loading, incremental updates
jasonmolenda Jul 16, 2024
e3c60ee
Add a comment why I check for corefiles & clarify msg
jasonmolenda Jul 16, 2024
ebd9b8b
ws fix
jasonmolenda Jul 17, 2024
dbbda57
Clarify my corefile check in ReadModuleFromMemory
jasonmolenda Jul 17, 2024
e5b4275
format nit
jasonmolenda Jul 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 22 additions & 3 deletions lldb/source/Core/DynamicLoader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -195,20 +196,38 @@ 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 0x";
prog_str.PutHex64(value);
}

if (!uuid.IsValid() && !value_is_offset) {
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 binary",
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);
Expand Down
64 changes: 36 additions & 28 deletions lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -714,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;
Expand Down Expand Up @@ -757,11 +758,37 @@ bool DynamicLoaderDarwinKernel::KextImageInfo::LoadImageUsingMemoryModule(
const ModuleList &target_images = target.GetImages();
m_module_sp = target_images.FindModule(m_uuid);

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_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()) {
ModuleSpec module_spec;
module_spec.GetUUID() = m_uuid;
module_spec.GetArchitecture() = target.GetArchitecture();
if (!m_uuid.IsValid())
module_spec.GetArchitecture() = target.GetArchitecture();
Comment on lines +789 to +790
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to check if the UUID is valid in the if statement and then set the architecture?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I have a UUID, it's authoritative, whereas the ArchSpec might be heuristically determined. I don't like setting both in a ModuleSpec if the UUID is valid, it it noramlly fine but it's a little footgun waiting for some unusual combination where the heuristically determined ArchSpec is not quite the same ("compatible") with the arch of the UUID specified.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know I shouldn't be making changes like this at the same time as adding progress status logging, but it irked me when I saw it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to be clear, it's almost always the "environment" or "os" part of the triple, which is nearly an entire fiction with firmware style debugging, that is the problem. One binary will say "hey I'm iOS" and another binary that needs to be loaded also is like "I'm something else" and lldb will reject the module load even though the UUIDs match.

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.
Expand All @@ -770,17 +797,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.
Expand Down Expand Up @@ -1058,12 +1077,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
Expand Down Expand Up @@ -1347,19 +1363,18 @@ 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];
bool kext_successfully_added = true;
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()));
image_info.LoadImageAtFileAddress(m_process);
kext_successfully_added = false;
}
}

Expand All @@ -1369,13 +1384,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);
}
Expand Down
4 changes: 3 additions & 1 deletion lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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; }

Expand Down
8 changes: 8 additions & 0 deletions lldb/source/Target/Process.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -2545,6 +2546,13 @@ 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;
// Reading an ObjectFile from a local corefile is very fast,
// don't print a progress report.
if (!GetCoreFile())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be better to use the bool accessor function:

if (IsLiveDebugSession())

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name is a little misleading, IsLiveDebugSession is only false for PostMortemProcess. Maybe we should have a Process::IsCorefileProcess or something, or I could put a comment there about what I'm doing.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostMortemProcess is what all of the core file processes (elf, mach-o and minidump) inherit from. That way all core file processes don't forget to override this function. I agree the name isn't the most clear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh whoops, my bad on that one, thanks. I'll change to use this instead, it's clearer.

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)
Expand Down
Loading