[Lldb-commits] [lldb] 098c01c - [lldb] Refactor Platform::ResolveExecutable
Pavel Labath via lldb-commits
lldb-commits at lists.llvm.org
Tue Nov 16 03:59:36 PST 2021
Author: Pavel Labath
Date: 2021-11-16T12:52:51+01:00
New Revision: 098c01c132c8d14f08b847793afa045af5bb4522
URL: https://github.com/llvm/llvm-project/commit/098c01c132c8d14f08b847793afa045af5bb4522
DIFF: https://github.com/llvm/llvm-project/commit/098c01c132c8d14f08b847793afa045af5bb4522.diff
LOG: [lldb] Refactor Platform::ResolveExecutable
Module resolution is probably the most complex piece of lldb [citation
needed], with numerous levels of abstraction, each one implementing
various retry and fallback strategies.
It is also a very repetitive, with only small differences between
"host", "remote-and-connected" and "remote-but-not-(yet)-connected"
scenarios.
The goal of this patch (first in series) is to reduce the number of
abstractions, and deduplicate the code.
One of the reasons for this complexity is the tension between the desire
to offload the process of module resolution to the remote platform
instance (that's how most other platform methods work), and the desire
to keep it local to the outer platform class (its easier to subclass the
outer class, and it generally makes more sense).
This patch resolves that conflict in favour of doing everything in the
outer class. The gdb-remote (our only remote platform) implementation of
ResolveExecutable was not doing anything gdb-specific, and was rather
similar to the other implementations of that method (any divergence is
most likely the result of fixes not being applied everywhere rather than
intentional).
It does this by excising the remote platform out of the resolution
codepath. The gdb-remote implementation of ResolveExecutable is moved to
Platform::ResolveRemoteExecutable, and the (only) call site is
redirected to that. On its own, this does not achieve (much), but it
creates new opportunities for layer peeling and code sharing, since all
of the code now lives closer together.
Differential Revision: https://reviews.llvm.org/D113487
Added:
Modified:
lldb/include/lldb/Target/Platform.h
lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
lldb/source/Target/Platform.cpp
lldb/source/Target/RemoteAwarePlatform.cpp
lldb/unittests/Target/RemoteAwarePlatformTest.cpp
Removed:
################################################################################
diff --git a/lldb/include/lldb/Target/Platform.h b/lldb/include/lldb/Target/Platform.h
index f3d88d6397d28..956b29e45dbab 100644
--- a/lldb/include/lldb/Target/Platform.h
+++ b/lldb/include/lldb/Target/Platform.h
@@ -943,8 +943,7 @@ class Platform : public PluginInterface {
virtual void CalculateTrapHandlerSymbolNames() = 0;
Status GetCachedExecutable(ModuleSpec &module_spec, lldb::ModuleSP &module_sp,
- const FileSpecList *module_search_paths_ptr,
- Platform &remote_platform);
+ const FileSpecList *module_search_paths_ptr);
virtual Status DownloadModuleSlice(const FileSpec &src_file_spec,
const uint64_t src_offset,
@@ -956,6 +955,11 @@ class Platform : public PluginInterface {
virtual const char *GetCacheHostname();
+ virtual Status
+ ResolveRemoteExecutable(const ModuleSpec &module_spec,
+ lldb::ModuleSP &exe_module_sp,
+ const FileSpecList *module_search_paths_ptr);
+
private:
typedef std::function<Status(const ModuleSpec &)> ModuleResolver;
@@ -969,8 +973,7 @@ class Platform : public PluginInterface {
Status LoadCachedExecutable(const ModuleSpec &module_spec,
lldb::ModuleSP &module_sp,
- const FileSpecList *module_search_paths_ptr,
- Platform &remote_platform);
+ const FileSpecList *module_search_paths_ptr);
FileSpec GetModuleCacheRoot();
};
diff --git a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
index d0a8fc4ebf381..56e624ab982a8 100644
--- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
+++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
@@ -89,76 +89,6 @@ llvm::StringRef PlatformRemoteGDBServer::GetDescription() {
return GetDescriptionStatic();
}
-Status PlatformRemoteGDBServer::ResolveExecutable(
- const ModuleSpec &module_spec, lldb::ModuleSP &exe_module_sp,
- const FileSpecList *module_search_paths_ptr) {
- // copied from PlatformRemoteiOS
-
- Status error;
- // Nothing special to do here, just use the actual file and architecture
-
- ModuleSpec resolved_module_spec(module_spec);
-
- // Resolve any executable within an apk on Android?
- // Host::ResolveExecutableInBundle (resolved_module_spec.GetFileSpec());
-
- if (FileSystem::Instance().Exists(resolved_module_spec.GetFileSpec()) ||
- module_spec.GetUUID().IsValid()) {
- if (resolved_module_spec.GetArchitecture().IsValid() ||
- resolved_module_spec.GetUUID().IsValid()) {
- error = ModuleList::GetSharedModule(resolved_module_spec, exe_module_sp,
- module_search_paths_ptr, nullptr,
- nullptr);
-
- if (exe_module_sp && exe_module_sp->GetObjectFile())
- return error;
- exe_module_sp.reset();
- }
- // No valid architecture was specified or the exact arch wasn't found so
- // ask the platform for the architectures that we should be using (in the
- // correct order) and see if we can find a match that way
- StreamString arch_names;
- for (uint32_t idx = 0; GetSupportedArchitectureAtIndex(
- idx, resolved_module_spec.GetArchitecture());
- ++idx) {
- error = ModuleList::GetSharedModule(resolved_module_spec, exe_module_sp,
- module_search_paths_ptr, nullptr,
- nullptr);
- // Did we find an executable using one of the
- if (error.Success()) {
- if (exe_module_sp && exe_module_sp->GetObjectFile())
- break;
- else
- error.SetErrorToGenericError();
- }
-
- if (idx > 0)
- arch_names.PutCString(", ");
- arch_names.PutCString(
- resolved_module_spec.GetArchitecture().GetArchitectureName());
- }
-
- if (error.Fail() || !exe_module_sp) {
- if (FileSystem::Instance().Readable(resolved_module_spec.GetFileSpec())) {
- error.SetErrorStringWithFormatv(
- "'{0}' doesn't contain any '{1}' platform architectures: {2}",
- resolved_module_spec.GetFileSpec(), GetPluginName(),
- arch_names.GetData());
- } else {
- error.SetErrorStringWithFormat(
- "'%s' is not readable",
- resolved_module_spec.GetFileSpec().GetPath().c_str());
- }
- }
- } else {
- error.SetErrorStringWithFormat(
- "'%s' does not exist",
- resolved_module_spec.GetFileSpec().GetPath().c_str());
- }
-
- return error;
-}
-
bool PlatformRemoteGDBServer::GetModuleSpec(const FileSpec &module_file_spec,
const ArchSpec &arch,
ModuleSpec &module_spec) {
diff --git a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
index f3d7ef17ccabb..e8536f2744b40 100644
--- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
+++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
@@ -40,10 +40,6 @@ class PlatformRemoteGDBServer : public Platform, private UserIDResolver {
llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
// lldb_private::Platform functions
- Status
- ResolveExecutable(const ModuleSpec &module_spec, lldb::ModuleSP &module_sp,
- const FileSpecList *module_search_paths_ptr) override;
-
bool GetModuleSpec(const FileSpec &module_file_spec, const ArchSpec &arch,
ModuleSpec &module_spec) override;
diff --git a/lldb/source/Target/Platform.cpp b/lldb/source/Target/Platform.cpp
index 0a84a37d9534a..a46371a3dfb53 100644
--- a/lldb/source/Target/Platform.cpp
+++ b/lldb/source/Target/Platform.cpp
@@ -833,6 +833,7 @@ Platform::ResolveExecutable(const ModuleSpec &module_spec,
lldb::ModuleSP &exe_module_sp,
const FileSpecList *module_search_paths_ptr) {
Status error;
+
if (FileSystem::Instance().Exists(module_spec.GetFileSpec())) {
if (module_spec.GetArchitecture().IsValid()) {
error = ModuleList::GetSharedModule(module_spec, exe_module_sp,
@@ -855,9 +856,77 @@ Platform::ResolveExecutable(const ModuleSpec &module_spec,
}
}
} else {
- error.SetErrorStringWithFormat("'%s' does not exist",
- module_spec.GetFileSpec().GetPath().c_str());
+ error.SetErrorStringWithFormat(
+ "'%s' does not exist", module_spec.GetFileSpec().GetPath().c_str());
+ }
+ return error;
+}
+
+Status
+Platform::ResolveRemoteExecutable(const ModuleSpec &module_spec,
+ lldb::ModuleSP &exe_module_sp,
+ const FileSpecList *module_search_paths_ptr) {
+ Status error;
+
+ // We may connect to a process and use the provided executable (Don't use
+ // local $PATH).
+ ModuleSpec resolved_module_spec(module_spec);
+
+ // Resolve any executable within a bundle on MacOSX
+ Host::ResolveExecutableInBundle(resolved_module_spec.GetFileSpec());
+
+ if (FileSystem::Instance().Exists(resolved_module_spec.GetFileSpec()) ||
+ module_spec.GetUUID().IsValid()) {
+ if (resolved_module_spec.GetArchitecture().IsValid() ||
+ resolved_module_spec.GetUUID().IsValid()) {
+ error = ModuleList::GetSharedModule(resolved_module_spec, exe_module_sp,
+ module_search_paths_ptr, nullptr,
+ nullptr);
+
+ if (exe_module_sp && exe_module_sp->GetObjectFile())
+ return error;
+ exe_module_sp.reset();
+ }
+ // No valid architecture was specified or the exact arch wasn't found so
+ // ask the platform for the architectures that we should be using (in the
+ // correct order) and see if we can find a match that way
+ StreamString arch_names;
+ for (uint32_t idx = 0; GetSupportedArchitectureAtIndex(
+ idx, resolved_module_spec.GetArchitecture());
+ ++idx) {
+ error = ModuleList::GetSharedModule(resolved_module_spec, exe_module_sp,
+ module_search_paths_ptr, nullptr,
+ nullptr);
+ // Did we find an executable using one of the
+ if (error.Success()) {
+ if (exe_module_sp && exe_module_sp->GetObjectFile())
+ break;
+ else
+ error.SetErrorToGenericError();
+ }
+
+ if (idx > 0)
+ arch_names.PutCString(", ");
+ arch_names.PutCString(
+ resolved_module_spec.GetArchitecture().GetArchitectureName());
+ }
+
+ if (error.Fail() || !exe_module_sp) {
+ if (FileSystem::Instance().Readable(resolved_module_spec.GetFileSpec())) {
+ error.SetErrorStringWithFormatv(
+ "'{0}' doesn't contain any '{1}' platform architectures: {2}",
+ resolved_module_spec.GetFileSpec(), GetPluginName(),
+ arch_names.GetData());
+ } else {
+ error.SetErrorStringWithFormatv("'{0}' is not readable",
+ resolved_module_spec.GetFileSpec());
+ }
+ }
+ } else {
+ error.SetErrorStringWithFormatv("'{0}' does not exist",
+ resolved_module_spec.GetFileSpec());
}
+
return error;
}
@@ -1507,12 +1576,13 @@ const std::vector<ConstString> &Platform::GetTrapHandlerSymbolNames() {
return m_trap_handlers;
}
-Status Platform::GetCachedExecutable(
- ModuleSpec &module_spec, lldb::ModuleSP &module_sp,
- const FileSpecList *module_search_paths_ptr, Platform &remote_platform) {
+Status
+Platform::GetCachedExecutable(ModuleSpec &module_spec,
+ lldb::ModuleSP &module_sp,
+ const FileSpecList *module_search_paths_ptr) {
const auto platform_spec = module_spec.GetFileSpec();
- const auto error = LoadCachedExecutable(
- module_spec, module_sp, module_search_paths_ptr, remote_platform);
+ const auto error =
+ LoadCachedExecutable(module_spec, module_sp, module_search_paths_ptr);
if (error.Success()) {
module_spec.GetFileSpec() = module_sp->GetFileSpec();
module_spec.GetPlatformFileSpec() = platform_spec;
@@ -1521,15 +1591,17 @@ Status Platform::GetCachedExecutable(
return error;
}
-Status Platform::LoadCachedExecutable(
- const ModuleSpec &module_spec, lldb::ModuleSP &module_sp,
- const FileSpecList *module_search_paths_ptr, Platform &remote_platform) {
- return GetRemoteSharedModule(module_spec, nullptr, module_sp,
- [&](const ModuleSpec &spec) {
- return remote_platform.ResolveExecutable(
- spec, module_sp, module_search_paths_ptr);
- },
- nullptr);
+Status
+Platform::LoadCachedExecutable(const ModuleSpec &module_spec,
+ lldb::ModuleSP &module_sp,
+ const FileSpecList *module_search_paths_ptr) {
+ return GetRemoteSharedModule(
+ module_spec, nullptr, module_sp,
+ [&](const ModuleSpec &spec) {
+ return ResolveRemoteExecutable(spec, module_sp,
+ module_search_paths_ptr);
+ },
+ nullptr);
}
Status Platform::GetRemoteSharedModule(const ModuleSpec &module_spec,
diff --git a/lldb/source/Target/RemoteAwarePlatform.cpp b/lldb/source/Target/RemoteAwarePlatform.cpp
index 6d38025408b29..eb39fc6db304e 100644
--- a/lldb/source/Target/RemoteAwarePlatform.cpp
+++ b/lldb/source/Target/RemoteAwarePlatform.cpp
@@ -72,8 +72,7 @@ Status RemoteAwarePlatform::ResolveExecutable(
} else {
if (m_remote_platform_sp) {
return GetCachedExecutable(resolved_module_spec, exe_module_sp,
- module_search_paths_ptr,
- *m_remote_platform_sp);
+ module_search_paths_ptr);
}
// We may connect to a process and use the provided executable (Don't use
diff --git a/lldb/unittests/Target/RemoteAwarePlatformTest.cpp b/lldb/unittests/Target/RemoteAwarePlatformTest.cpp
index e2851e669b45f..dba4f81b6cd5f 100644
--- a/lldb/unittests/Target/RemoteAwarePlatformTest.cpp
+++ b/lldb/unittests/Target/RemoteAwarePlatformTest.cpp
@@ -31,6 +31,17 @@ class RemoteAwarePlatformTester : public RemoteAwarePlatform {
ProcessSP(ProcessAttachInfo &, Debugger &, Target *, Status &));
MOCK_METHOD0(CalculateTrapHandlerSymbolNames, void());
+ MOCK_METHOD2(ResolveRemoteExecutable,
+ std::pair<Status, ModuleSP>(const ModuleSpec &,
+ const FileSpecList *));
+ Status ResolveRemoteExecutable(
+ const ModuleSpec &module_spec, lldb::ModuleSP &exe_module_sp,
+ const FileSpecList *module_search_paths_ptr) /*override*/ {
+ auto pair = ResolveRemoteExecutable(module_spec, module_search_paths_ptr);
+ exe_module_sp = pair.second;
+ return pair.first;
+ }
+
void SetRemotePlatform(lldb::PlatformSP platform) {
m_remote_platform_sp = platform;
}
@@ -47,18 +58,6 @@ class TargetPlatformTester : public Platform {
ProcessSP(ProcessAttachInfo &, Debugger &, Target *, Status &));
MOCK_METHOD0(CalculateTrapHandlerSymbolNames, void());
MOCK_METHOD0(GetUserIDResolver, UserIDResolver &());
-
- MOCK_METHOD2(ResolveExecutable,
- std::pair<Status, ModuleSP>(const ModuleSpec &,
- const FileSpecList *));
- Status
- ResolveExecutable(const ModuleSpec &module_spec,
- lldb::ModuleSP &exe_module_sp,
- const FileSpecList *module_search_paths_ptr) /*override*/ {
- auto pair = ResolveExecutable(module_spec, module_search_paths_ptr);
- exe_module_sp = pair.second;
- return pair.first;
- }
};
namespace {
@@ -73,15 +72,13 @@ TEST_F(RemoteAwarePlatformTest, TestResolveExecutabelOnClientByPlatform) {
ModuleSpec executable_spec;
ModuleSP expected_executable(new Module(executable_spec));
- auto platform_sp = std::make_shared<TargetPlatformTester>(false);
- EXPECT_CALL(*platform_sp, ResolveExecutable(_, _))
- .WillRepeatedly(Return(std::make_pair(Status(), expected_executable)));
-
RemoteAwarePlatformTester platform(false);
EXPECT_CALL(platform, GetSupportedArchitectureAtIndex(_, _))
.WillRepeatedly(Return(false));
+ EXPECT_CALL(platform, ResolveRemoteExecutable(_, _))
+ .WillRepeatedly(Return(std::make_pair(Status(), expected_executable)));
- platform.SetRemotePlatform(platform_sp);
+ platform.SetRemotePlatform(std::make_shared<TargetPlatformTester>(false));
ModuleSP resolved_sp;
lldb_private::Status status =
More information about the lldb-commits
mailing list