[lldb] Refactor Platform::ResolveExecutable

[labath]

Summary

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.

[labath]

lldb/source/Target/Platform.cpp

865

Next step is to merge this with the if (!m_remote_platform_sp) path in RemoteAwarePlatform::ResolveExecutable

[clayborg]

LGTM. I will ok soon if no one else chimes in!

[labath]

Thanks, Greg.

lldb/include/lldb/Target/Platform.h

protected:
   /// are different.
   ///
   /// The base class Platform ivar m_trap_handlers should be updated by the
   /// Platform subclass when this method is called.  If there are no
   /// predefined trap handlers, this method may be a no-op.
   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,
                                      const uint64_t src_size,
                                      const FileSpec &dst_file_spec);
 
   virtual Status DownloadSymbolFile(const lldb::ModuleSP &module_sp,
                                     const FileSpec &dst_file_spec);
 
   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;
 
   Status GetRemoteSharedModule(const ModuleSpec &module_spec, Process *process,
                                lldb::ModuleSP &module_sp,
                                const ModuleResolver &module_resolver,
                                bool *did_create_ptr);
 
   bool GetCachedSharedModule(const ModuleSpec &module_spec,
                              lldb::ModuleSP &module_sp, bool *did_create_ptr);
 
   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();
 };
 
 class PlatformList {
 public:
   PlatformList() : m_mutex(), m_platforms(), m_selected_platform_sp() {}
 

lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.h

public:
   PlatformRemoteGDBServer();
 
   ~PlatformRemoteGDBServer() override;
 
   // lldb_private::PluginInterface functions
   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;
 
   llvm::StringRef GetDescription() override;
 
   Status GetFileWithUUID(const FileSpec &platform_file, const UUID *uuid_ptr,
                          FileSpec &local_file) override;
 

lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp

if (m_platform_description.empty()) {
     }
   }
 
   if (!m_platform_description.empty())
     return m_platform_description.c_str();
   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) {
   Log *log = GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PLATFORM);
 
   const auto module_path = module_file_spec.GetPath(false);
 
   if (!m_gdb_client.GetModuleInfo(module_file_spec, arch, module_spec)) {

lldb/source/Target/Platform.cpp

bool Platform::SetOSVersion(llvm::VersionTuple version) {
   return false;
 }
 
 Status
 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,
                                           module_search_paths_ptr, nullptr,
                                           nullptr);
     } else {
       // No valid architecture was specified, 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
       ModuleSpec arch_module_spec(module_spec);
       for (uint32_t idx = 0; GetSupportedArchitectureAtIndex(
                idx, arch_module_spec.GetArchitecture());
            ++idx) {
         error = ModuleList::GetSharedModule(arch_module_spec, exe_module_sp,
                                             module_search_paths_ptr, nullptr,
                                             nullptr);
         // Did we find an executable using one of the
         if (error.Success() && exe_module_sp)
           break;
       }
     }
   } 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;
 }
 
 Status Platform::ResolveSymbolFile(Target &target, const ModuleSpec &sym_spec,
                                    FileSpec &sym_file) {
   Status error;
   if (FileSystem::Instance().Exists(sym_spec.GetSymbolFileSpec()))
     sym_file = sym_spec.GetSymbolFileSpec();
if (!m_calculated_trap_handlers) {
     if (!m_calculated_trap_handlers) {
       CalculateTrapHandlerSymbolNames();
       m_calculated_trap_handlers = true;
     }
   }
   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;
   }
 
   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,
+        return ResolveRemoteExecutable(spec, module_sp,
+                                       module_search_paths_ptr);
 }
 
 Status Platform::GetRemoteSharedModule(const ModuleSpec &module_spec,
                                        Process *process,
                                        lldb::ModuleSP &module_sp,
                                        const ModuleResolver &module_resolver,
                                        bool *did_create_ptr) {
   // Get module information from a target.

lldb/source/Target/RemoteAwarePlatform.cpp

else {
       else
         error.SetErrorStringWithFormat(
             "unable to find executable for '%s'",
             resolved_module_spec.GetFileSpec().GetPath().c_str());
     }
   } 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
     // local $PATH).
 
     // Resolve any executable within a bundle on MacOSX
     Host::ResolveExecutableInBundle(resolved_module_spec.GetFileSpec());
 

lldb/unittests/Target/RemoteAwarePlatformTest.cpp

public:
 
   MOCK_METHOD0(GetDescription, llvm::StringRef());
   MOCK_METHOD0(GetPluginName, llvm::StringRef());
   MOCK_METHOD2(GetSupportedArchitectureAtIndex, bool(uint32_t, ArchSpec &));
   MOCK_METHOD4(Attach,
                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;
   }
 };
 
 class TargetPlatformTester : public Platform {
 public:
   using Platform::Platform;
 
   MOCK_METHOD0(GetDescription, llvm::StringRef());
   MOCK_METHOD0(GetPluginName, llvm::StringRef());
   MOCK_METHOD2(GetSupportedArchitectureAtIndex, bool(uint32_t, ArchSpec &));
   MOCK_METHOD4(Attach,
                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 {
 class RemoteAwarePlatformTest : public testing::Test {
 public:
   static void SetUpTestCase() { FileSystem::Initialize(); }
   static void TearDownTestCase() { FileSystem::Terminate(); }
 };
 } // namespace
 
 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 =
       platform.ResolveExecutable(executable_spec, resolved_sp, nullptr);
 
   ASSERT_TRUE(status.Success());
   EXPECT_EQ(expected_executable.get(), resolved_sp.get());
 }