[Lldb-commits] [lldb] Clean up PlatformDarwinKernel::GetSharedModule, document (PR #78652)
Jason Molenda via lldb-commits
lldb-commits at lists.llvm.org
Thu Jan 18 23:22:57 PST 2024
https://github.com/jasonmolenda updated https://github.com/llvm/llvm-project/pull/78652
>From 0936638373f9ae720de0ca9f50d3e5436897254b Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Thu, 18 Jan 2024 16:25:01 -0800
Subject: [PATCH 1/2] Clean up PlatformDarwinKernel::GetSharedModule, docuemnt
PlatformDarwinKernel::GetSharedModule, which can find a kernel or
kext from a local filesystem scan, needed a little cleanup. The
method which finds kernels was (1) not looking for the SymbolFileSpec
when creating a Module, and (2) adding that newly created Module
to a Target, which GetSharedModule should not be doing - after
auditing many other subclass implementations of this method, I
haven't found any others doing it. Platform::GetSharedModule didn't
have a headerdoc so it took a little work to piece together the
intended behaviors.
This is addressing a bug where PlatformDarwinKernel::GetSharedModuleKernel
would find the ObjectFile for a kernel, create a Module, and add it
to the Target. Then up in DynamicLoaderDarwinKernel, it would check if
the Module had a SymbolFile FileSpec, and because it did not, it would
do its own search for a binary & dSYM, find them, and then add that to
the Target. Now we have two copies of the Module in the Target, one
with a dSYM and the other without, and only one of them has its load
addresses set.
GetSharedModule should not be adding binaries to the Target, and it
should set the SymbolFile FileSpec when it is creating the Module.
rdar://120895951
---
lldb/include/lldb/Target/Platform.h | 26 ++++++++
.../Platform/MacOSX/PlatformDarwinKernel.cpp | 62 +++++++------------
2 files changed, 47 insertions(+), 41 deletions(-)
diff --git a/lldb/include/lldb/Target/Platform.h b/lldb/include/lldb/Target/Platform.h
index 129e4565d9ff993..13196ff74d663f8 100644
--- a/lldb/include/lldb/Target/Platform.h
+++ b/lldb/include/lldb/Target/Platform.h
@@ -288,6 +288,32 @@ class Platform : public PluginInterface {
LocateExecutableScriptingResources(Target *target, Module &module,
Stream &feedback_stream);
+ /// \param[in] module_spec
+ /// The ModuleSpec of a binary to find.
+ ///
+ /// \param[in] process
+ /// A Process.
+ ///
+ /// \param[out] module_sp
+ /// A Module that matches the ModuleSpec, if one is found.
+ ///
+ /// \param[in] module_search_paths_ptr
+ /// Locations to possibly look for a binary that matches the ModuleSpec.
+ ///
+ /// \param[out] old_modules
+ /// Existing Modules in the Process' Target image list which match
+ /// the FileSpec.
+ ///
+ /// \param[out] did_create_ptr
+ /// Optional boolean, nullptr may be passed for this argument.
+ /// If this method is returning a *new* ModuleSP, this
+ /// will be set to true.
+ /// If this method is returning a ModuleSP that is already in the
+ /// Target's image list, it will be false.
+ ///
+ /// \return
+ /// The Status object for any errors found while searching for
+ /// the binary.
virtual Status GetSharedModule(
const ModuleSpec &module_spec, Process *process,
lldb::ModuleSP &module_sp, const FileSpecList *module_search_paths_ptr,
diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
index e2839f3285cce45..6490432c72e18d3 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -791,9 +791,10 @@ Status PlatformDarwinKernel::GetSharedModuleKernel(
const ModuleSpec &module_spec, Process *process, ModuleSP &module_sp,
const FileSpecList *module_search_paths_ptr,
llvm::SmallVectorImpl<ModuleSP> *old_modules, bool *did_create_ptr) {
- Status error;
module_sp.reset();
UpdateKextandKernelsLocalScan();
+ if (did_create_ptr)
+ *did_create_ptr = false;
// First try all kernel binaries that have a dSYM next to them
for (auto possible_kernel : m_kernel_binaries_with_dsyms) {
@@ -803,22 +804,19 @@ Status PlatformDarwinKernel::GetSharedModuleKernel(
module_sp.reset(new Module(kern_spec));
if (module_sp && module_sp->GetObjectFile() &&
module_sp->MatchesModuleSpec(kern_spec)) {
- // module_sp is an actual kernel binary we want to add.
- if (process) {
- const bool notify = false;
- process->GetTarget().GetImages().AppendIfNeeded(module_sp, notify);
- error.Clear();
- return error;
- } else {
- error = ModuleList::GetSharedModule(kern_spec, module_sp, nullptr,
- nullptr, nullptr);
- if (module_sp && module_sp->GetObjectFile() &&
- module_sp->GetObjectFile()->GetType() !=
- ObjectFile::Type::eTypeCoreFile) {
- return error;
- }
- module_sp.reset();
- }
+ // The dSYM is next to the binary (that's the only
+ // way it ends up in the index), but it might be a
+ // .dSYM.yaa that needs to be expanded, don't just
+ // append ".dSYM" to the filename for the SymbolFile.
+ FileSpecList search_paths =
+ process->GetTarget().GetDebugFileSearchPaths();
+ FileSpec dsym_fspec =
+ PluginManager::LocateExecutableSymbolFile(kern_spec, search_paths);
+ if (FileSystem::Instance().Exists(dsym_fspec))
+ module_sp->SetSymbolFileFileSpec(dsym_fspec);
+ if (did_create_ptr)
+ *did_create_ptr = true;
+ return {};
}
}
}
@@ -836,36 +834,18 @@ Status PlatformDarwinKernel::GetSharedModuleKernel(
module_sp.reset(new Module(kern_spec));
if (module_sp && module_sp->GetObjectFile() &&
module_sp->MatchesModuleSpec(kern_spec)) {
- // module_sp is an actual kernel binary we want to add.
- if (process) {
- const bool notify = false;
- process->GetTarget().GetImages().AppendIfNeeded(module_sp, notify);
- error.Clear();
- return error;
- } else {
- error = ModuleList::GetSharedModule(kern_spec, module_sp, nullptr,
- nullptr, nullptr);
- if (module_sp && module_sp->GetObjectFile() &&
- module_sp->GetObjectFile()->GetType() !=
- ObjectFile::Type::eTypeCoreFile) {
- return error;
- }
- module_sp.reset();
- }
+ if (did_create_ptr)
+ *did_create_ptr = true;
+ return {};
}
}
}
// Give the generic methods, including possibly calling into DebugSymbols
// framework on macOS systems, a chance.
- error = PlatformDarwin::GetSharedModule(module_spec, process, module_sp,
- module_search_paths_ptr, old_modules,
- did_create_ptr);
- if (error.Success() && module_sp.get()) {
- return error;
- }
-
- return error;
+ return PlatformDarwin::GetSharedModule(module_spec, process, module_sp,
+ module_search_paths_ptr, old_modules,
+ did_create_ptr);
}
std::vector<lldb_private::FileSpec>
>From 165942dfaf143574d44af4ba86f5cb259ab1a696 Mon Sep 17 00:00:00 2001
From: Jason Molenda <jmolenda at apple.com>
Date: Thu, 18 Jan 2024 23:21:56 -0800
Subject: [PATCH 2/2] Check that module_sp is an empty shared pointer on entry
Instead of resetting the module_sp out variable, add an
assert that it is empty to make it easier for readers of
this method to follow the logic.
---
lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
index 6490432c72e18d3..1f121f5c4e5d0ad 100644
--- a/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
+++ b/lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp
@@ -791,7 +791,7 @@ Status PlatformDarwinKernel::GetSharedModuleKernel(
const ModuleSpec &module_spec, Process *process, ModuleSP &module_sp,
const FileSpecList *module_search_paths_ptr,
llvm::SmallVectorImpl<ModuleSP> *old_modules, bool *did_create_ptr) {
- module_sp.reset();
+ assert(module_sp.get() == nullptr);
UpdateKextandKernelsLocalScan();
if (did_create_ptr)
*did_create_ptr = false;
@@ -841,7 +841,7 @@ Status PlatformDarwinKernel::GetSharedModuleKernel(
}
}
- // Give the generic methods, including possibly calling into DebugSymbols
+ // Give the generic methods, including possibly calling into DebugSymbols
// framework on macOS systems, a chance.
return PlatformDarwin::GetSharedModule(module_spec, process, module_sp,
module_search_paths_ptr, old_modules,
More information about the lldb-commits
mailing list