[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

6 files changed, 110 insertions, 113 deletions

diff --git a/lldb/include/lldb/Target/Platform.h b/lldb/include/lldb/Target/Platform.h
index f3d88d6..956b29e 100644
--- a/lldb/include/lldb/Target/Platform.h
+++ b/lldb/include/lldb/Target/Platform.h
@@ -943,8 +943,7 @@ protected:
   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 @@ protected:
 
   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 @@ private:
 
   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 d0a8fc4..56e624a 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 f3d7ef1..e8536f2 100644
--- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
+++ b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h
@@ -40,10 +40,6 @@ public:
   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 0a84a37..a46371a 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 6d38025..eb39fc6 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 e2851e6..dba4f81 100644
--- a/lldb/unittests/Target/RemoteAwarePlatformTest.cpp
+++ b/lldb/unittests/Target/RemoteAwarePlatformTest.cpp
@@ -31,6 +31,17 @@ public:
                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 @@ public:
                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 =