[lldb][NFCI] Platforms should own their SDKBuild and SDKRootDirectory strings

[bulbazord]

Summary

These don't need to be ConstStrings. They don't really benefit much from
deduplication and comparing them isn't on a hot path, so they don't
really benefit much from quick comparisons.

[JDevlieghere]

lldb/include/lldb/Interpreter/OptionGroupPlatform.h

52–53

This should take a std::string by value and move it.

58–60

Same here

[bulbazord]

lldb/include/lldb/Interpreter/OptionGroupPlatform.h

52–53

Is there a specific reason you want to do that instead of using llvm::StringRef::str()?

[JDevlieghere]

lldb/include/lldb/Interpreter/OptionGroupPlatform.h

52–53

Yes, it saves a copy when calling SetSDKRootDirectory with an rvalue reference or a moved std::string. There's no downside if you're going to store it as a std::string anyway.

[bulbazord]

Implement Jonas's suggestion

[JDevlieghere]

LGTM with the other two functions updated as well.

lldb/include/lldb/Target/Platform.h

453

457–459

std::string

[bulbazord]

Update other 2 functions (and one callsite)

[bulbazord]

Avoid construction of temporary std::string

[bulbazord]

Looks like this actually causes TestDebuggerAPI.py to segfault because SBPlatform::SetSDKRoot can take nullptr for its argument, and the conversion from nullptr -> std::string blows up. I'm fixing that in https://reviews.llvm.org/D152962.

lldb/include/lldb/Interpreter/OptionGroupPlatform.h

public:
 
   void SetPlatformName(const char *platform_name) {
     if (platform_name && platform_name[0])
       m_platform_name.assign(platform_name);
     else
       m_platform_name.clear();
   }
 
-  ConstString GetSDKRootDirectory() const { return m_sdk_sysroot; }
+  const std::string &GetSDKRootDirectory() const { return m_sdk_sysroot; }
 
-  void SetSDKRootDirectory(ConstString sdk_root_directory) {
-    m_sdk_sysroot = sdk_root_directory;
+  void SetSDKRootDirectory(std::string sdk_root_directory) {
+    m_sdk_sysroot = std::move(sdk_root_directory);
   }
 
-  ConstString GetSDKBuild() const { return m_sdk_build; }
+  const std::string &GetSDKBuild() const { return m_sdk_build; }
 
-  void SetSDKBuild(ConstString sdk_build) { m_sdk_build = sdk_build; }
+  void SetSDKBuild(std::string sdk_build) {
+    m_sdk_build = std::move(sdk_build);
+  }
 
   bool PlatformMatches(const lldb::PlatformSP &platform_sp) const;
 
 protected:
   std::string m_platform_name;
-  ConstString m_sdk_sysroot;
-  ConstString m_sdk_build;
+  std::string m_sdk_sysroot;
+  std::string m_sdk_build;
   llvm::VersionTuple m_os_version;
   bool m_include_platform_option;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_INTERPRETER_OPTIONGROUPPLATFORM_H

lldb/include/lldb/Target/Platform.h

public:
   ArchSpec GetAugmentedArchSpec(llvm::StringRef triple);
 
   // Used for column widths
   size_t GetMaxUserIDNameLength() const { return m_max_uid_name_len; }
 
   // Used for column widths
   size_t GetMaxGroupIDNameLength() const { return m_max_gid_name_len; }
 
-  ConstString GetSDKRootDirectory() const { return m_sdk_sysroot; }
+  const std::string &GetSDKRootDirectory() const { return m_sdk_sysroot; }
 
-  void SetSDKRootDirectory(ConstString dir) { m_sdk_sysroot = dir; }
+  void SetSDKRootDirectory(std::string dir) { m_sdk_sysroot = std::move(dir); }
 
-  ConstString GetSDKBuild() const { return m_sdk_build; }
+  const std::string &GetSDKBuild() const { return m_sdk_build; }
 
-  void SetSDKBuild(ConstString sdk_build) { m_sdk_build = sdk_build; }
+  void SetSDKBuild(std::string sdk_build) {
+    m_sdk_build = std::move(sdk_build);
+  }
 
   // Override this to return true if your platform supports Clang modules. You
   // may also need to override AddClangModuleCompilationOptions to pass the
   // right Clang flags for your platform.
   virtual bool SupportsModules() { return false; }
 
   // Appends the platform-specific options required to find the modules for the
   // current platform.
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
+  std::string
       m_sdk_sysroot; // the root location of where the SDK files are all located
-  ConstString m_sdk_build;
+  std::string 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
       m_system_arch; // The architecture of the kernel or the remote platform
   typedef std::map<uint32_t, ConstString> IDToNameMap;

lldb/source/API/SBPlatform.cpp

uint32_t SBPlatform::GetOSUpdateVersion() {
   if (PlatformSP platform_sp = GetSP())
     version = platform_sp->GetOSVersion();
   return version.getSubminor().value_or(UINT32_MAX);
 }
 
 void SBPlatform::SetSDKRoot(const char *sysroot) {
   LLDB_INSTRUMENT_VA(this, sysroot);
   if (PlatformSP platform_sp = GetSP())
-    platform_sp->SetSDKRootDirectory(ConstString(sysroot));
+    platform_sp->SetSDKRootDirectory(sysroot);
 }
 
 SBError SBPlatform::Get(SBFileSpec &src, SBFileSpec &dst) {
   LLDB_INSTRUMENT_VA(this, src, dst);
 
   SBError sb_error;
   PlatformSP platform_sp(GetSP());
   if (platform_sp) {

lldb/source/Interpreter/OptionGroupPlatform.cpp

PlatformSP OptionGroupPlatform::CreatePlatformWithOptions(
   }
 
   if (platform_sp) {
     if (make_selected)
       platforms.SetSelectedPlatform(platform_sp);
     if (!m_os_version.empty())
       platform_sp->SetOSVersion(m_os_version);
 
-    if (m_sdk_sysroot)
+    if (!m_sdk_sysroot.empty())
       platform_sp->SetSDKRootDirectory(m_sdk_sysroot);
 
-    if (m_sdk_build)
+    if (!m_sdk_build.empty())
       platform_sp->SetSDKBuild(m_sdk_build);
   }
 
   return platform_sp;
 }
 
 void OptionGroupPlatform::OptionParsingStarting(
     ExecutionContext *execution_context) {
   m_platform_name.clear();
-  m_sdk_sysroot.Clear();
-  m_sdk_build.Clear();
+  m_sdk_sysroot.clear();
+  m_sdk_build.clear();
   m_os_version = llvm::VersionTuple();
 }
 
 static constexpr OptionDefinition g_option_table[] = {
     {LLDB_OPT_SET_ALL, false, "platform", 'p', OptionParser::eRequiredArgument,
      nullptr, {}, 0, eArgTypePlatform, "Specify name of the platform to "
                                        "use for this target, creating the "
                                        "platform if necessary."},
OptionGroupPlatform::SetOptionValue(uint32_t option_idx,
   Status error;
   if (!m_include_platform_option)
     ++option_idx;
 
   const int short_option = g_option_table[option_idx].short_option;
 
   switch (short_option) {
   case 'p':
-    m_platform_name.assign(std::string(option_arg));
+    m_platform_name.assign(option_arg.str());
     break;
 
   case 'v':
     if (m_os_version.tryParse(option_arg))
       error.SetErrorStringWithFormatv("invalid version string '{0}'",
                                       option_arg);
     break;
 
   case 'b':
-    m_sdk_build.SetString(option_arg);
+    m_sdk_build.assign(option_arg.str());
     break;
 
   case 'S':
-    m_sdk_sysroot.SetString(option_arg);
+    m_sdk_sysroot.assign(option_arg.str());
     break;
 
   default:
     llvm_unreachable("Unimplemented option");
   }
   return error;
 }
 
 bool OptionGroupPlatform::PlatformMatches(
     const lldb::PlatformSP &platform_sp) const {
-  if (platform_sp) {
-    if (!m_platform_name.empty()) {
-      if (platform_sp->GetName() != m_platform_name)
-        return false;
-    }
+  if (!platform_sp)
 
-    if (m_sdk_build && m_sdk_build != platform_sp->GetSDKBuild())
-      return false;
+  if (!m_platform_name.empty() && platform_sp->GetName() != m_platform_name)
 
-    if (m_sdk_sysroot && m_sdk_sysroot != platform_sp->GetSDKRootDirectory())
-      return false;
+  if (!m_sdk_build.empty() && platform_sp->GetSDKBuild() != m_sdk_build)
 
-    if (!m_os_version.empty() && m_os_version != platform_sp->GetOSVersion())
-      return false;
-    return true;
-  }
+  if (!m_sdk_sysroot.empty() &&
+      platform_sp->GetSDKRootDirectory() != m_sdk_sysroot)
+
+  if (!m_os_version.empty() && platform_sp->GetOSVersion() != m_os_version)
     return false;
+
+  return true;
 }

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

PlatformDarwinDevice::GetContainedFilesIntoVectorOfStringsCallback(
   return FileSystem::eEnumerateDirectoryResultNext;
 }
 
 bool PlatformDarwinDevice::UpdateSDKDirectoryInfosIfNeeded() {
   Log *log = GetLog(LLDBLog::Host);
   std::lock_guard<std::mutex> guard(m_sdk_dir_mutex);
   if (m_sdk_directory_infos.empty()) {
     // A --sysroot option was supplied - add it to our list of SDKs to check
-    if (m_sdk_sysroot) {
-      FileSpec sdk_sysroot_fspec(m_sdk_sysroot.GetCString());
+    if (!m_sdk_sysroot.empty()) {
+      FileSpec sdk_sysroot_fspec(m_sdk_sysroot.c_str());
       FileSystem::Instance().Resolve(sdk_sysroot_fspec);
       const SDKDirectoryInfo sdk_sysroot_directory_info(sdk_sysroot_fspec);
       m_sdk_directory_infos.push_back(sdk_sysroot_directory_info);
       if (log) {
         LLDB_LOGF(log,
                   "PlatformDarwinDevice::UpdateSDKDirectoryInfosIfNeeded added "
                   "--sysroot SDK directory %s",
-                  m_sdk_sysroot.GetCString());
+                  m_sdk_sysroot.c_str());
       }
       return true;
     }
     const char *device_support_dir = GetDeviceSupportDirectory();
     if (log) {
       LLDB_LOGF(log,
                 "PlatformDarwinDevice::UpdateSDKDirectoryInfosIfNeeded Got "
                 "DeviceSupport directory %s",

 const PlatformDarwinDevice::SDKDirectoryInfo *
 PlatformDarwinDevice::GetSDKDirectoryForCurrentOSVersion() {
   uint32_t i;
   if (UpdateSDKDirectoryInfosIfNeeded()) {
     const uint32_t num_sdk_infos = m_sdk_directory_infos.size();
     std::vector<bool> check_sdk_info(num_sdk_infos, true);
 
     // Prefer the user SDK build string.
-    ConstString build = GetSDKBuild();
+    std::string build = GetSDKBuild();
 
     // Fall back to the platform's build string.
-    if (!build) {
-      if (std::optional<std::string> os_build_str = GetOSBuildString()) {
-        build = ConstString(*os_build_str);
-      }
+    if (build.empty()) {
+      if (std::optional<std::string> os_build_str = GetOSBuildString())
+        build.assign(*os_build_str);
     }
 
     // If we have a build string, only check platforms for which the build
     // string matches.
-    if (build) {
+    if (!build.empty()) {
       for (i = 0; i < num_sdk_infos; ++i)
-        check_sdk_info[i] = m_sdk_directory_infos[i].build == build;
+        check_sdk_info[i] = m_sdk_directory_infos[i].build.GetStringRef() ==
+                            llvm::StringRef(build);
     }
 
     // If we are connected we can find the version of the OS the platform us
     // running on and select the right SDK
     llvm::VersionTuple version = GetOSVersion();
     if (!version.empty()) {
       if (UpdateSDKDirectoryInfosIfNeeded()) {
         // First try for an exact match of major, minor and update.
if (!version.empty()) {
           if (check_sdk_info[i]) {
             if (m_sdk_directory_infos[i].version.getMajor() ==
                 version.getMajor()) {
               return &m_sdk_directory_infos[i];
             }
           }
         }
       }
-    } else if (build) {
+    } else if (!build.empty()) {
       // No version, just a build number, return the first one that matches.
       for (i = 0; i < num_sdk_infos; ++i)
         if (check_sdk_info[i])
           return &m_sdk_directory_infos[i];
     }
   }
   return nullptr;
 }
const char *PlatformDarwinDevice::GetDeviceSupportDirectory() {
   // or it should have a valid path if the code gets here
   assert(m_device_support_directory.empty() == false);
   if (m_device_support_directory[0])
     return m_device_support_directory.c_str();
   return nullptr;
 }
 
 const char *PlatformDarwinDevice::GetDeviceSupportDirectoryForOSVersion() {
-  if (m_sdk_sysroot)
-    return m_sdk_sysroot.GetCString();
+  if (!m_sdk_sysroot.empty())
+    return m_sdk_sysroot.c_str();
 
   if (m_device_support_directory_for_os_version.empty()) {
     const PlatformDarwinDevice::SDKDirectoryInfo *sdk_dir_info =
         GetSDKDirectoryForCurrentOSVersion();
     if (sdk_dir_info == nullptr)
       sdk_dir_info = GetSDKDirectoryForLatestOSVersion();
     if (sdk_dir_info) {
       char path[PATH_MAX];

lldb/source/Plugins/Platform/QemuUser/PlatformQemuUser.cpp

for (size_t i = 1; i < launch_info.GetArguments().size(); ++i)
     args.AppendArgument(launch_info.GetArguments()[i].ref());
 
   LLDB_LOG(log, "{0} -> {1}", get_arg_range(launch_info.GetArguments()),
            get_arg_range(args));
 
   launch_info.SetArguments(args, true);
 
   Environment emulator_env = Host::GetEnvironment();
-  if (ConstString sysroot = GetSDKRootDirectory())
-    emulator_env["QEMU_LD_PREFIX"] = sysroot.GetStringRef().str();
+  if (const std::string &sysroot = GetSDKRootDirectory(); !sysroot.empty())
+    emulator_env["QEMU_LD_PREFIX"] = sysroot;
   for (const auto &KV : GetGlobalProperties().GetEmulatorEnvVars())
     emulator_env[KV.first()] = KV.second;
   launch_info.GetEnvironment() = ComputeLaunchEnvironment(
       std::move(launch_info.GetEnvironment()), std::move(emulator_env));
 
   launch_info.SetLaunchInSeparateProcessGroup(true);
   launch_info.GetFlags().Clear(eLaunchFlagDebug);
   launch_info.SetMonitorProcessCallback(ProcessLaunchInfo::NoOpMonitorCallback);

lldb/source/Target/Platform.cpp

return ModuleList::GetSharedModule(module_spec, module_sp,
                                        module_search_paths_ptr, old_modules,
                                        did_create_ptr, false);
 
   // Module resolver lambda.
   auto resolver = [&](const ModuleSpec &spec) {
     Status error(eErrorTypeGeneric);
     ModuleSpec resolved_spec;
     // Check if we have sysroot set.
-    if (m_sdk_sysroot) {
+    if (!m_sdk_sysroot.empty()) {
       // Prepend sysroot to module spec.
       resolved_spec = spec;
-      resolved_spec.GetFileSpec().PrependPathComponent(
-          m_sdk_sysroot.GetStringRef());
+      resolved_spec.GetFileSpec().PrependPathComponent(m_sdk_sysroot);
       // Try to get shared module with resolved spec.
       error = ModuleList::GetSharedModule(resolved_spec, module_sp,
                                           module_search_paths_ptr, old_modules,
                                           did_create_ptr, false);
     }
     // If we don't have sysroot or it didn't work then
     // try original module spec.
     if (!error.Success()) {
if (IsHost()) {
     strm.Printf("  Hostname: %s\n", GetHostname());
   } else {
     const bool is_connected = IsConnected();
     if (is_connected)
       strm.Printf("  Hostname: %s\n", GetHostname());
     strm.Printf(" Connected: %s\n", is_connected ? "yes" : "no");
   }
 
-  if (GetSDKRootDirectory()) {
-    strm.Format("   Sysroot: {0}\n", GetSDKRootDirectory());
-  }
+  if (const std::string &sdk_root = GetSDKRootDirectory(); !sdk_root.empty())
+    strm.Format("   Sysroot: {0}\n", sdk_root);
+
   if (GetWorkingDirectory()) {
     strm.Printf("WorkingDir: %s\n", GetWorkingDirectory().GetPath().c_str());
   }
   if (!IsConnected())
     return;
 
   std::string specific_info(GetPlatformSpecificConnectionInformation());