[lldb] Unify Platform::ResolveExecutable (llvm#96256)

The Platform class currently has two functions to resolve an executable: `ResolveExecutable` and `ResolveRemoteExecutable`. The former strictly deals with local files while the latter can handle potentially remote files. I couldn't figure out why the distinction matters, at the latter is a super-set of the former. To make things even more confusion, we had a similar but not identical implementation in RemoteAwarePlatform where its implementation of `ResolveExecutable` could handle remote files. To top it all off, we had copy-pasted implementation, dead code included in `PlatformAppleSimulator` and `PlatformRemoteDarwinDevice`. I went ahead and unified all the different implementation on the original `ResolveRemoteExecutable` implementation. As far as I can tell, it should work for every other platform, and the test suite (on macOS) seems to agree with me, except for a small wording change.
llvm-project-tlp · Jun 27, 2024 · b42418f · b42418f
1 parent c6c26c4
commit b42418f
Show file tree

Hide file tree

Showing 11 changed files with 15 additions and 348 deletions.
diff --git a/lldb/include/lldb/Target/Platform.h b/lldb/include/lldb/Target/Platform.h
@@ -139,7 +139,7 @@ class Platform : public PluginInterface {
   ///     Returns \b true if this Platform plug-in was able to find
   ///     a suitable executable, \b false otherwise.
   virtual Status ResolveExecutable(const ModuleSpec &module_spec,
-                                   lldb::ModuleSP &module_sp,
+                                   lldb::ModuleSP &exe_module_sp,
                                    const FileSpecList *module_search_paths_ptr);
 
   /// Find a symbol file given a symbol file module specification.
@@ -1004,11 +1004,6 @@ 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;
 

diff --git a/lldb/include/lldb/Target/RemoteAwarePlatform.h b/lldb/include/lldb/Target/RemoteAwarePlatform.h
@@ -23,10 +23,6 @@ class RemoteAwarePlatform : public Platform {
   bool GetModuleSpec(const FileSpec &module_file_spec, const ArchSpec &arch,
                      ModuleSpec &module_spec) override;
 
-  Status
-  ResolveExecutable(const ModuleSpec &module_spec, lldb::ModuleSP &module_sp,
-                    const FileSpecList *module_search_paths_ptr) override;
-
   lldb::user_id_t OpenFile(const FileSpec &file_spec, File::OpenOptions flags,
                            uint32_t mode, Status &error) override;
 

diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp b/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.cpp
@@ -383,81 +383,6 @@ PlatformSP PlatformAppleSimulator::CreateInstance(
   return PlatformSP();
 }
 
-Status PlatformAppleSimulator::ResolveExecutable(
-    const ModuleSpec &module_spec, lldb::ModuleSP &exe_module_sp,
-    const FileSpecList *module_search_paths_ptr) {
-  Status error;
-  // Nothing special to do here, just use the actual file and architecture
-
-  ModuleSpec resolved_module_spec(module_spec);
-
-  // If we have "ls" as the exe_file, resolve the executable loation based on
-  // the current path variables
-  // TODO: resolve bare executables in the Platform SDK
-  //    if (!resolved_exe_file.Exists())
-  //        resolved_exe_file.ResolveExecutableLocation ();
-
-  // Resolve any executable within a bundle on MacOSX
-  // TODO: verify that this handles shallow bundles, if not then implement one
-  // ourselves
-  Host::ResolveExecutableInBundle(resolved_module_spec.GetFileSpec());
-
-  if (FileSystem::Instance().Exists(resolved_module_spec.GetFileSpec())) {
-    if (resolved_module_spec.GetArchitecture().IsValid()) {
-      error = ModuleList::GetSharedModule(resolved_module_spec, exe_module_sp,
-                                          NULL, NULL, NULL);
-
-      if (exe_module_sp && exe_module_sp->GetObjectFile())
-        return error;
-      exe_module_sp.reset();
-    }
-    // No valid architecture was specified or the exact ARM slice 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;
-    llvm::ListSeparator LS;
-    ArchSpec platform_arch;
-    for (const ArchSpec &arch : GetSupportedArchitectures({})) {
-      resolved_module_spec.GetArchitecture() = arch;
-
-      // Only match x86 with x86 and x86_64 with x86_64...
-      if (!module_spec.GetArchitecture().IsValid() ||
-          module_spec.GetArchitecture().GetCore() ==
-              resolved_module_spec.GetArchitecture().GetCore()) {
-        error = ModuleList::GetSharedModule(resolved_module_spec, exe_module_sp,
-                                            NULL, NULL, NULL);
-        // Did we find an executable using one of the
-        if (error.Success()) {
-          if (exe_module_sp && exe_module_sp->GetObjectFile())
-            break;
-          else
-            error.SetErrorToGenericError();
-        }
-
-        arch_names << LS << platform_arch.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.GetString());
-      } else {
-        error.SetErrorStringWithFormat(
-            "'%s' is not readable",
-            resolved_module_spec.GetFileSpec().GetPath().c_str());
-      }
-    }
-  } else {
-    error.SetErrorStringWithFormat("'%s' does not exist",
-                                   module_spec.GetFileSpec().GetPath().c_str());
-  }
-
-  return error;
-}
-
 Status PlatformAppleSimulator::GetSymbolFile(const FileSpec &platform_file,
                                              const UUID *uuid_ptr,
                                              FileSpec &local_file) {

diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.h b/lldb/source/Plugins/Platform/MacOSX/PlatformAppleSimulator.h
@@ -87,10 +87,6 @@ class PlatformAppleSimulator : public PlatformDarwin {
   std::vector<ArchSpec>
   GetSupportedArchitectures(const ArchSpec &process_host_arch) override;
 
-  Status
-  ResolveExecutable(const ModuleSpec &module_spec, lldb::ModuleSP &module_sp,
-                    const FileSpecList *module_search_paths_ptr) override;
-
   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/PlatformRemoteDarwinDevice.cpp b/lldb/source/Plugins/Platform/MacOSX/PlatformRemoteDarwinDevice.cpp
@@ -63,71 +63,6 @@ void PlatformRemoteDarwinDevice::GetStatus(Stream &strm) {
   }
 }
 
-Status PlatformRemoteDarwinDevice::ResolveExecutable(
-    const ModuleSpec &ms, lldb::ModuleSP &exe_module_sp,
-    const FileSpecList *module_search_paths_ptr) {
-  Status error;
-  // Nothing special to do here, just use the actual file and architecture
-
-  ModuleSpec resolved_module_spec(ms);
-
-  // Resolve any executable within a bundle on MacOSX
-  // TODO: verify that this handles shallow bundles, if not then implement one
-  // ourselves
-  Host::ResolveExecutableInBundle(resolved_module_spec.GetFileSpec());
-
-  if (FileSystem::Instance().Exists(resolved_module_spec.GetFileSpec())) {
-    if (resolved_module_spec.GetArchitecture().IsValid() ||
-        resolved_module_spec.GetUUID().IsValid()) {
-      error = ModuleList::GetSharedModule(resolved_module_spec, exe_module_sp,
-                                          nullptr, nullptr, nullptr);
-
-      if (exe_module_sp && exe_module_sp->GetObjectFile())
-        return error;
-      exe_module_sp.reset();
-    }
-    // No valid architecture was specified or the exact ARM slice 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;
-    llvm::ListSeparator LS;
-    ArchSpec process_host_arch;
-    for (const ArchSpec &arch : GetSupportedArchitectures(process_host_arch)) {
-      resolved_module_spec.GetArchitecture() = arch;
-      error = ModuleList::GetSharedModule(resolved_module_spec, exe_module_sp,
-                                          nullptr, 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();
-      }
-
-      arch_names << LS << arch.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 PlatformRemoteDarwinDevice::GetFileInSDK(const char *platform_file_path,
                                      uint32_t sdk_idx,
                                      lldb_private::FileSpec &local_file) {

diff --git a/lldb/source/Plugins/Platform/MacOSX/PlatformRemoteDarwinDevice.h b/lldb/source/Plugins/Platform/MacOSX/PlatformRemoteDarwinDevice.h
@@ -40,10 +40,6 @@ class PlatformRemoteDarwinDevice : public PlatformDarwinDevice {
   ~PlatformRemoteDarwinDevice() override;
 
   // Platform functions
-  Status
-  ResolveExecutable(const ModuleSpec &module_spec, lldb::ModuleSP &module_sp,
-                    const FileSpecList *module_search_paths_ptr) override;
-
   void GetStatus(Stream &strm) override;
 
   virtual Status GetSymbolFile(const FileSpec &platform_file,

diff --git a/lldb/source/Target/Platform.cpp b/lldb/source/Target/Platform.cpp
@@ -768,41 +768,6 @@ Platform::ResolveExecutable(const ModuleSpec &module_spec,
                             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);
-      ArchSpec process_host_arch;
-      for (const ArchSpec &arch :
-           GetSupportedArchitectures(process_host_arch)) {
-        arch_module_spec.GetArchitecture() = arch;
-        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());
-  }
-  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);
@@ -822,9 +787,9 @@ Platform::ResolveRemoteExecutable(const ModuleSpec &module_spec,
         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
+    // No valid architecture was specified or the exact arch wasn't found.
+    // 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;
     llvm::ListSeparator LS;
     ArchSpec process_host_arch;
@@ -833,12 +798,10 @@ Platform::ResolveRemoteExecutable(const ModuleSpec &module_spec,
       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();
+        error.SetErrorToGenericError();
       }
 
       arch_names << LS << arch.GetArchitectureName();
@@ -1516,8 +1479,7 @@ Platform::GetCachedExecutable(ModuleSpec &module_spec,
   Status error = GetRemoteSharedModule(
       module_spec, nullptr, module_sp,
       [&](const ModuleSpec &spec) {
-        return ResolveRemoteExecutable(spec, module_sp,
-                                       module_search_paths_ptr);
+        return ResolveExecutable(spec, module_sp, module_search_paths_ptr);
       },
       nullptr);
   if (error.Success()) {