Skip to content

Commit

Permalink
[lldb] Refactor Platform::ResolveExecutable
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@labath
labath committed Nov 16, 2021
1 parent 6c48f6a commit 098c01c
Show file tree
Hide file tree
Showing 6 changed files with 110 additions and 113 deletions.
11 changes: 7 additions & 4 deletions lldb/include/lldb/Target/Platform.h
View file
Expand Up @@ -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,
Expand All @@ -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;

Expand All @@ -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();
};
Expand Down
70 changes: 0 additions & 70 deletions lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
View file
Expand Up @@ -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) {
Expand Down
4 changes: 0 additions & 4 deletions lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
View file
Expand Up @@ -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;

Expand Down
104 changes: 88 additions & 16 deletions lldb/source/Target/Platform.cpp
View file
Expand Up @@ -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,
Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
Expand All @@ -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,
Expand Down
3 changes: 1 addition & 2 deletions lldb/source/Target/RemoteAwarePlatform.cpp
View file
Expand Up @@ -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
Expand Down
31 changes: 14 additions & 17 deletions lldb/unittests/Target/RemoteAwarePlatformTest.cpp
View file
Expand Up @@ -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;
}
Expand All @@ -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 {
Expand All @@ -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 =
Expand Down

0 comments on commit 098c01c

Please sign in to comment.