[lldb/Platform] Prepare decouple instance and plugin names

[labath]

Summary

This patch changes the return value of Platform::GetName() to a StringRef, and
uses the opportunity (compile errors) to change some callsites to use
GetPluginName() instead. The two methods still remain hardwired to return the
same thing, but this will change once the ideas in
https://discourse.llvm.org/t/multiple-platforms-with-the-same-name/59594 are
implemented.

[JDevlieghere]

Returning a StringRef from GetName makes sense, but in the constructor, if you're eventually going to store it, why not pass and std::string in and std::move it into place? Other than that this LGTM, but I'll refrain from accepting so it continues to show up in other's review queue.

[labath]

The main reason for that is because the platform objects are generally created in a loop (for (factory : plugins) { result = factory(cantMove(name), ...)}), so we can't make use of the move semantics because we don't know which construction will stick.

[labath]

Well.. technically, I suppose I could make the constructors take a std::string, since at that point we're committed to creating the object, but the CreateInstance functions would still need to take a StringRef -- but the net effect is still one string copy.

[JDevlieghere]

I don't feel super strongly, I was just curious.

[clayborg]

I am fine with this. Only suggestion is to use ConstString still for the m_instance_name since we might compare against this name if we are looking for one with a matching name (instead of std::string).

lldb/include/lldb/Target/Platform.h

889

Do we want to make this a ConstString so finding matching instances is quick and just a pointer compare?

[labath]

Given the renewed interest in the size of the ConstString pool, I'd like to avoid adding strings there without a good reason. And I don't see any reason why a name-based platform lookup should be on any hot path. A Target object holds a platform pointer, the current platform is a pointer, and anything else that is hot enough should probably hold a pointer instead of a name as well.

[clayborg]

In D119146#3310353, @labath wrote:

Given the renewed interest in the size of the ConstString pool, I'd like to avoid adding strings there without a good reason. And I don't see any reason why a name-based platform lookup should be on any hot path. A Target object holds a platform pointer, the current platform is a pointer, and anything else that is hot enough should probably hold a pointer instead of a name as well.

As you mention there won't be many of these, so it won't take up much space in the string pool, but I am not adamant that this would need to change.

[labath]

Reduce the scope of the patch to GetName interface changes. Given the direction
of the discourse thread, it seems unlikely that we will be able to determine the
instance name at object creation time.

We will likely need a setter which will be called when the class is associated
with a debugger.

[JDevlieghere]

(Still) LGTM

[stella.stamenova]

It looks like this broke the windows lldb bot:

https://lab.llvm.org/buildbot/#/builders/83/builds/15999

lldb/include/lldb/Target/Platform.h

public:
 
   bool SetOSVersion(llvm::VersionTuple os_version);
 
   llvm::Optional<std::string> GetOSBuildString();
 
   llvm::Optional<std::string> GetOSKernelDescription();
 
   // Returns the name of the platform
-  ConstString GetName();
+  llvm::StringRef GetName() { return GetPluginName(); }
 
   virtual const char *GetHostname();
 
   virtual ConstString GetFullNameForDylib(ConstString basename);
 
   virtual llvm::StringRef GetDescription() = 0;
 
   /// Report the current status for this platform.
virtual lldb::user_id_t GetFileSize(const FileSpec &file_spec) {
     return UINT64_MAX;
   }
 
   virtual void AutoCompleteDiskFileOrDirectory(CompletionRequest &request,
                                                bool only_dir) {}
 
   virtual uint64_t ReadFile(lldb::user_id_t fd, uint64_t offset, void *dst,
                             uint64_t dst_len, Status &error) {
-    error.SetErrorStringWithFormat(
-        "Platform::ReadFile() is not supported in the %s platform",
-        GetName().GetCString());
+    error.SetErrorStringWithFormatv(
+        "Platform::ReadFile() is not supported in the {0} platform",
+        GetPluginName());
     return -1;
   }
 
   virtual uint64_t WriteFile(lldb::user_id_t fd, uint64_t offset,
                              const void *src, uint64_t src_len, Status &error) {
-    error.SetErrorStringWithFormat(
-        "Platform::WriteFile() is not supported in the %s platform",
-        GetName().GetCString());
+    error.SetErrorStringWithFormatv(
+        "Platform::WriteFile() is not supported in the {0} platform",
+        GetPluginName());
     return -1;
   }
 
   virtual Status GetFile(const FileSpec &source, const FileSpec &destination);
 
   virtual Status PutFile(const FileSpec &source, const FileSpec &destination,
                          uint32_t uid = UINT32_MAX, uint32_t gid = UINT32_MAX);
 
protected:
   bool m_is_host;
   // Set to true when we are able to actually set the OS version while being
   // connected. For remote platforms, we might set the version ahead of time
   // before we actually connect and this version might change when we actually
   // connect to a remote platform. For the host platform this will be set to
   // the once we call HostInfo::GetOSVersion().
   bool m_os_version_set_while_connected;
   bool m_system_arch_set_while_connected;
   ConstString
       m_sdk_sysroot; // the root location of where the SDK files are all located
   ConstString m_sdk_build;
   FileSpec m_working_dir; // The working directory which is used when installing
                           // modules that have no install path set
   std::string m_remote_url;
   std::string m_hostname;
   llvm::VersionTuple m_os_version;
   ArchSpec

lldb/source/API/SBPlatform.cpp

void SBPlatform::Clear() {
   m_opaque_sp.reset();
 }
 
 const char *SBPlatform::GetName() {
   LLDB_INSTRUMENT_VA(this);
 
   PlatformSP platform_sp(GetSP());
   if (platform_sp)
-    return platform_sp->GetName().GetCString();
+    return ConstString(platform_sp->GetName()).AsCString();
   return nullptr;
 }
 
 lldb::PlatformSP SBPlatform::GetSP() const { return m_opaque_sp; }
 
 void SBPlatform::SetSP(const lldb::PlatformSP &platform_sp) {
   m_opaque_sp = platform_sp;
 }

lldb/source/Commands/CommandObjectPlatform.cpp

if (platform_sp) {
               }
             }
 
             if (matches == 0) {
               if (match_desc)
                 result.AppendErrorWithFormatv(
                     "no processes were found that {0} \"{1}\" on the \"{2}\" "
                     "platform\n",
-                    match_desc, match_name, platform_sp->GetPluginName());
+                    match_desc, match_name, platform_sp->GetName());
               else
                 result.AppendErrorWithFormatv(
                     "no processes were found on the \"{0}\" platform\n",
-                    platform_sp->GetPluginName());
+                    platform_sp->GetName());
             } else {
-              result.AppendMessageWithFormat(
-                  "%u matching process%s found on \"%s\"", matches,
-                  matches > 1 ? "es were" : " was",
-                  platform_sp->GetName().GetCString());
+              result.AppendMessageWithFormatv(
+                  "{0} matching process{1} found on \"{2}\"", matches,
+                  matches > 1 ? "es were" : " was", platform_sp->GetName());
               if (match_desc)
                 result.AppendMessageWithFormat(" whose name %s \"%s\"",
                                                match_desc, match_name);
               result.AppendMessageWithFormat("\n");
               ProcessInstanceInfo::DumpTableHeader(ostrm, m_options.show_args,
                                                    m_options.verbose);
               for (uint32_t i = 0; i < matches; ++i) {
                 proc_infos[i].DumpAsTableRow(

lldb/source/Commands/CommandObjectTarget.cpp

static void DumpTargetInfo(uint32_t target_idx, Target *target,
   uint32_t properties = 0;
   if (target_arch.IsValid()) {
     strm.Printf("%sarch=", properties++ > 0 ? ", " : " ( ");
     target_arch.DumpTriple(strm.AsRawOstream());
     properties++;
   }
   PlatformSP platform_sp(target->GetPlatform());
   if (platform_sp)
-    strm.Printf("%splatform=%s", properties++ > 0 ? ", " : " ( ",
-                platform_sp->GetName().GetCString());
+    strm.Format("{0}platform={1}", properties++ > 0 ? ", " : " ( ",
+                platform_sp->GetName());
 
   ProcessSP process_sp(target->GetProcessSP());
   bool show_process_status = false;
   if (process_sp) {
     lldb::pid_t pid = process_sp->GetID();
     StateType state = process_sp->GetState();
     if (show_stopped_process_status)
       show_process_status = StateIsStoppedState(state, true);

lldb/source/Core/IOHandlerCursesGUI.cpp

public:
   }
 
   // Returns the content of the choice as a string.
   std::string GetChoiceContent() { return m_choices[m_choice]; }
 
   // Returns the index of the choice.
   int GetChoice() { return m_choice; }
 
-  void SetChoice(const std::string &choice) {
+  void SetChoice(llvm::StringRef choice) {
     for (int i = 0; i < GetNumberOfChoices(); i++) {
       if (choice == m_choices[i]) {
         m_choice = i;
         return;
       }
     }
   }
 
 protected:
   std::string m_label;
   int m_number_of_visible_choices;
   std::vector<std::string> m_choices;
   // The index of the selected choice.
   int m_choice;
   // The index of the first visible choice in the field.
   int m_first_visibile_choice;
 };
 
 class PlatformPluginFieldDelegate : public ChoicesFieldDelegate {
 public:
   PlatformPluginFieldDelegate(Debugger &debugger)
       : ChoicesFieldDelegate("Platform Plugin", 3, GetPossiblePluginNames()) {
     PlatformSP platform_sp = debugger.GetPlatformList().GetSelectedPlatform();
     if (platform_sp)
-      SetChoice(platform_sp->GetName().AsCString());
+      SetChoice(platform_sp->GetPluginName());
   }
 
   std::vector<std::string> GetPossiblePluginNames() {
     std::vector<std::string> names;
     size_t i = 0;
     for (llvm::StringRef name =
              PluginManager::GetPlatformPluginNameAtIndex(i++);
          !name.empty(); name = PluginManager::GetProcessPluginNameAtIndex(i++))

lldb/source/Interpreter/OptionGroupPlatform.cpp

PlatformSP OptionGroupPlatform::CreatePlatformWithOptions(
     Status &error, ArchSpec &platform_arch) const {
   PlatformSP platform_sp;
 
   if (!m_platform_name.empty()) {
     platform_sp = Platform::Create(ConstString(m_platform_name.c_str()), error);
     if (platform_sp) {
       if (platform_arch.IsValid() &&
           !platform_sp->IsCompatibleArchitecture(arch, false, &platform_arch)) {
-        error.SetErrorStringWithFormat("platform '%s' doesn't support '%s'",
-                                       platform_sp->GetName().GetCString(),
-                                       arch.GetTriple().getTriple().c_str());
+        error.SetErrorStringWithFormatv("platform '{0}' doesn't support '{1}'",
+                                        platform_sp->GetPluginName(),
+                                        arch.GetTriple().getTriple());
         platform_sp.reset();
         return platform_sp;
       }
     }
   } else if (arch.IsValid()) {
     platform_sp = Platform::Create(arch, &platform_arch, error);
   }
 
OptionGroupPlatform::SetOptionValue(uint32_t option_idx,
   }
   return error;
 }
 
 bool OptionGroupPlatform::PlatformMatches(
     const lldb::PlatformSP &platform_sp) const {
   if (platform_sp) {
     if (!m_platform_name.empty()) {
-      if (platform_sp->GetName() != ConstString(m_platform_name.c_str()))
+      if (platform_sp->GetName() != m_platform_name)
         return false;
     }
 
     if (m_sdk_build && m_sdk_build != platform_sp->GetSDKBuild())
       return false;
 
     if (m_sdk_sysroot && m_sdk_sysroot != platform_sp->GetSDKRootDirectory())
       return false;
 
     if (!m_os_version.empty() && m_os_version != platform_sp->GetOSVersion())
       return false;
     return true;
   }
   return false;
 }

lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.h

 #include "AdbClient.h"
 
 namespace lldb_private {
 namespace platform_android {
 
 class PlatformAndroidRemoteGDBServer
     : public platform_gdb_server::PlatformRemoteGDBServer {
 public:
-  PlatformAndroidRemoteGDBServer();
+  using PlatformRemoteGDBServer::PlatformRemoteGDBServer;
 
   ~PlatformAndroidRemoteGDBServer() override;
 
   Status ConnectRemote(Args &args) override;
 
   Status DisconnectRemote() override;
 
   lldb::ProcessSP ConnectProcess(llvm::StringRef connect_url,

lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp

static Status FindUnusedPort(uint16_t &port) {
 
   error = tcp_socket->Listen("127.0.0.1:0", 1);
   if (error.Success())
     port = tcp_socket->GetLocalPortNumber();
 
   return error;
 }
 
-PlatformAndroidRemoteGDBServer::PlatformAndroidRemoteGDBServer() = default;
-
 PlatformAndroidRemoteGDBServer::~PlatformAndroidRemoteGDBServer() {
   for (const auto &it : m_port_forwards)
     DeleteForwardPortWithAdb(it.second, m_device_id);
 }
 
 bool PlatformAndroidRemoteGDBServer::LaunchGDBServer(lldb::pid_t &pid,
                                                      std::string &connect_url) {
   assert(IsConnected());

lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.h

 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/FileSystem.h"
 
 #include <string>
 #include <tuple>
 
 class PlatformDarwin : public PlatformPOSIX {
 public:
-  PlatformDarwin(bool is_host);
+  using PlatformPOSIX::PlatformPOSIX;
 
   ~PlatformDarwin() override;
 
   lldb_private::Status PutFile(const lldb_private::FileSpec &source,
                                const lldb_private::FileSpec &destination,
                                uint32_t uid = UINT32_MAX,
                                uint32_t gid = UINT32_MAX) override;
 

lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp

 
 #if defined(__APPLE__)
 #include <TargetConditionals.h>
 #endif
 
 using namespace lldb;
 using namespace lldb_private;
 
-/// Default Constructor
-PlatformDarwin::PlatformDarwin(bool is_host) : PlatformPOSIX(is_host) {}
-
 /// Destructor.
 ///
 /// The destructor is virtual since this class is designed to be
 /// inherited from by the plug-in instance.
 PlatformDarwin::~PlatformDarwin() = default;
 
 lldb_private::Status
 PlatformDarwin::PutFile(const lldb_private::FileSpec &source,

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

Status ProcessGDBRemote::DoDestroy() {
   //
   // And of course, if we weren't able to stop the process by the time we get
   // here, it isn't necessary (or helpful) to do any of this.
 
   if (!m_gdb_comm.GetThreadSuffixSupported() &&
       m_public_state.GetValue() != eStateRunning) {
     PlatformSP platform_sp = GetTarget().GetPlatform();
 
-    if (platform_sp && platform_sp->GetName() &&
-        platform_sp->GetName().GetStringRef() ==
+    if (platform_sp && platform_sp->GetPluginName() ==
                            PlatformRemoteiOS::GetPluginNameStatic()) {
       if (m_destroy_tried_resuming) {
         if (log)
           log->PutCString("ProcessGDBRemote::DoDestroy() - Tried resuming to "
                           "destroy once already, not doing it again.");
       } else {
         // At present, the plans are discarded and the breakpoints disabled
         // Process::Destroy, but we really need it to happen here and it
         // doesn't matter if we do it twice.

lldb/source/Target/Platform.cpp

 PlatformSP Platform::Find(ConstString name) {
   if (name) {
     static ConstString g_host_platform_name("host");
     if (name == g_host_platform_name)
       return GetHostPlatform();
 
     std::lock_guard<std::recursive_mutex> guard(GetPlatformListMutex());
     for (const auto &platform_sp : GetPlatformList()) {
-      if (platform_sp->GetName() == name)
+      if (platform_sp->GetName() == name.GetStringRef())
         return platform_sp;
     }
   }
   return PlatformSP();
 }
 
 PlatformSP Platform::Create(ConstString name, Status &error) {
   PlatformCreateInstance create_callback = nullptr;
Status Platform::SetFilePermissions(const FileSpec &file_spec,
   } else {
     Status error;
     error.SetErrorStringWithFormatv("remote platform {0} doesn't support {1}",
                                     GetPluginName(), LLVM_PRETTY_FUNCTION);
     return error;
   }
 }
 
-ConstString Platform::GetName() { return ConstString(GetPluginName()); }
-
 const char *Platform::GetHostname() {
   if (IsHost())
     return "127.0.0.1";
 
   if (m_hostname.empty())
     return nullptr;
   return m_hostname.c_str();
 }

 Status Platform::DownloadSymbolFile(const lldb::ModuleSP &module_sp,
                                     const FileSpec &dst_file_spec) {
   return Status(
       "Symbol file downloading not supported by the default platform.");
 }
 
 FileSpec Platform::GetModuleCacheRoot() {
   auto dir_spec = GetGlobalPlatformProperties().GetModuleCacheDirectory();
-  dir_spec.AppendPathComponent(GetName().AsCString());
+  dir_spec.AppendPathComponent(GetPluginName());
   return dir_spec;
 }
 
 const char *Platform::GetCacheHostname() { return GetHostname(); }
 
 const UnixSignalsSP &Platform::GetRemoteUnixSignals() {
   static const auto s_default_unix_signals_sp = std::make_shared<UnixSignals>();
   return s_default_unix_signals_sp;

lldb/source/Target/Process.cpp

if (platform_sp) {
     if (target_arch.IsValid() &&
         !platform_sp->IsCompatibleArchitecture(target_arch, false, nullptr)) {
       ArchSpec platform_arch;
       platform_sp =
           platform_sp->GetPlatformForArchitecture(target_arch, &platform_arch);
       if (platform_sp) {
         GetTarget().SetPlatform(platform_sp);
         GetTarget().SetArchitecture(platform_arch);
-        LLDB_LOGF(log,
-                  "Process::%s switching platform to %s and architecture "
-                  "to %s based on info from attach",
-                  __FUNCTION__, platform_sp->GetName().AsCString(""),
-                  platform_arch.GetTriple().getTriple().c_str());
+        LLDB_LOG(log,
+                 "switching platform to {0} and architecture to {1} based on "
+                 "info from attach",
+                 platform_sp->GetName(), platform_arch.GetTriple().getTriple());
       }
     } else if (!process_arch.IsValid()) {
       ProcessInstanceInfo process_info;
       GetProcessInfo(process_info);
       const ArchSpec &process_arch = process_info.GetArchitecture();
       const ArchSpec &target_arch = GetTarget().GetArchitecture();
       if (process_arch.IsValid() &&
           target_arch.IsCompatibleMatch(process_arch) &&

lldb/source/Target/TargetList.cpp

if (num_specs > 0) {
           std::set<Platform *> platform_set;
           error_strm.Printf(
               "more than one platform supports this executable (");
           for (const auto &the_platform_sp : platforms) {
             if (platform_set.find(the_platform_sp.get()) ==
                 platform_set.end()) {
               if (!platform_set.empty())
                 error_strm.PutCString(", ");
-              error_strm.PutCString(the_platform_sp->GetName().GetCString());
+              error_strm.PutCString(the_platform_sp->GetName());
               platform_set.insert(the_platform_sp.get());
             }
           }
           error_strm.Printf("), specify an architecture to disambiguate");
           error.SetErrorString(error_strm.GetString());
           return error;
         }
       }