[lldb][NFCI] Remove use of ConstString from UnixSignals

The majority of UnixSignals strings are static in the sense that they do
not change. The overwhelming majority of these strings are string
literals. Using ConstString to manage their lifetime does not make
sense. The only exception to this is one of the subclasses of
UnixSignals, for which I have created a StringSet local to that file
which will guarantee the lifetimes of these StringRefs.

As for the other benefits of ConstString, string uniqueness is not a
concern (as many of them are already string literals) and comparing
signal names and aliases should not be a hot path.

Differential Revision: https://reviews.llvm.org/D159011

Author: bulbazord

lldb/include/lldb/Target/UnixSignals.h

@@ -14,7 +14,6 @@
 #include <string>
 #include <vector>
 
-#include "lldb/Utility/ConstString.h"
 #include "lldb/lldb-private.h"
 #include "llvm/Support/JSON.h"
 
@@ -101,9 +100,10 @@ class UnixSignals {
   // your subclass of UnixSignals or in your Process Plugin's GetUnixSignals
   // method before you return the UnixSignal object.
 
-  void AddSignal(int signo, const char *name, bool default_suppress,
+  void AddSignal(int signo, llvm::StringRef name, bool default_suppress,
                  bool default_stop, bool default_notify,
-                 const char *description, const char *alias = nullptr);
+                 llvm::StringRef description,
+                 llvm::StringRef alias = llvm::StringRef());
 
   enum SignalCodePrintOption { None, Address, Bounds };
 
@@ -147,17 +147,20 @@ class UnixSignals {
     const SignalCodePrintOption m_print_option;
   };
 
+  // The StringRefs in Signal are either backed by string literals or reside in
+  // persistent storage (e.g. a StringSet).
   struct Signal {
-    ConstString m_name;
-    ConstString m_alias;
-    std::string m_description;
+    llvm::StringRef m_name;
+    llvm::StringRef m_alias;
+    llvm::StringRef m_description;
     std::map<int32_t, SignalCode> m_codes;
     uint32_t m_hit_count = 0;
     bool m_suppress : 1, m_stop : 1, m_notify : 1;
     bool m_default_suppress : 1, m_default_stop : 1, m_default_notify : 1;
 
-    Signal(const char *name, bool default_suppress, bool default_stop,
-           bool default_notify, const char *description, const char *alias);
+    Signal(llvm::StringRef name, bool default_suppress, bool default_stop,
+           bool default_notify, llvm::StringRef description,
+           llvm::StringRef alias);
 
     ~Signal() = default;
     void Reset(bool reset_stop, bool reset_notify, bool reset_suppress);

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

@@ -28,10 +28,12 @@
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/UriParser.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/Support/FormatAdapters.h"
 
 #include "Plugins/Process/Utility/GDBRemoteSignals.h"
 #include "Plugins/Process/gdb-remote/ProcessGDBRemote.h"
+#include <mutex>
 #include <optional>
 
 using namespace lldb;
@@ -41,6 +43,11 @@ using namespace lldb_private::platform_gdb_server;
 LLDB_PLUGIN_DEFINE_ADV(PlatformRemoteGDBServer, PlatformGDB)
 
 static bool g_initialized = false;
+// UnixSignals does not store the signal names or descriptions itself.
+// It holds onto StringRefs. Becaue we may get signal information dynamically
+// from the remote, these strings need persistent storage client-side.
+static std::mutex g_signal_string_mutex;
+static llvm::StringSet<> g_signal_string_storage;
 
 void PlatformRemoteGDBServer::Initialize() {
   Platform::Initialize();
@@ -749,8 +756,18 @@ const UnixSignalsSP &PlatformRemoteGDBServer::GetRemoteUnixSignals() {
         if (object_sp && object_sp->IsValid())
           description = std::string(object_sp->GetStringValue());
 
-        remote_signals_sp->AddSignal(signo, name.str().c_str(), suppress, stop,
-                                     notify, description.c_str());
+        llvm::StringRef name_backed, description_backed;
+        {
+          std::lock_guard<std::mutex> guard(g_signal_string_mutex);
+          name_backed =
+              g_signal_string_storage.insert(name).first->getKeyData();
+          if (!description.empty())
+            description_backed =
+                g_signal_string_storage.insert(description).first->getKeyData();
+        }
+
+        remote_signals_sp->AddSignal(signo, name_backed, suppress, stop, notify,
+                                     description_backed);
         return true;
       });
 

lldb/source/Target/UnixSignals.cpp

@@ -18,17 +18,13 @@
 using namespace lldb_private;
 using namespace llvm;
 
-UnixSignals::Signal::Signal(const char *name, bool default_suppress,
+UnixSignals::Signal::Signal(llvm::StringRef name, bool default_suppress,
                             bool default_stop, bool default_notify,
-                            const char *description, const char *alias)
-    : m_name(name), m_alias(alias), m_description(),
+                            llvm::StringRef description, llvm::StringRef alias)
+    : m_name(name), m_alias(alias), m_description(description),
       m_suppress(default_suppress), m_stop(default_stop),
-      m_notify(default_notify),
-      m_default_suppress(default_suppress), m_default_stop(default_stop),
-      m_default_notify(default_notify) {
-  if (description)
-    m_description.assign(description);
-}
+      m_notify(default_notify), m_default_suppress(default_suppress),
+      m_default_stop(default_stop), m_default_notify(default_notify) {}
 
 lldb::UnixSignalsSP UnixSignals::Create(const ArchSpec &arch) {
   const auto &triple = arch.GetTriple();
@@ -104,9 +100,10 @@ void UnixSignals::Reset() {
   // clang-format on
 }
 
-void UnixSignals::AddSignal(int signo, const char *name, bool default_suppress,
-                            bool default_stop, bool default_notify,
-                            const char *description, const char *alias) {
+void UnixSignals::AddSignal(int signo, llvm::StringRef name,
+                            bool default_suppress, bool default_stop,
+                            bool default_notify, llvm::StringRef description,
+                            llvm::StringRef alias) {
   Signal new_signal(name, default_suppress, default_stop, default_notify,
                     description, alias);
   m_signals.insert(std::make_pair(signo, new_signal));
@@ -135,7 +132,7 @@ llvm::StringRef UnixSignals::GetSignalAsStringRef(int32_t signo) const {
   const auto pos = m_signals.find(signo);
   if (pos == m_signals.end())
     return {};
-  return pos->second.m_name.GetStringRef();
+  return pos->second.m_name;
 }
 
 std::string
@@ -147,7 +144,7 @@ UnixSignals::GetSignalDescription(int32_t signo, std::optional<int32_t> code,
 
   collection::const_iterator pos = m_signals.find(signo);
   if (pos != m_signals.end()) {
-    str = pos->second.m_name.GetCString();
+    str = pos->second.m_name.str();
 
     if (code) {
       std::map<int32_t, SignalCode>::const_iterator cpos =
@@ -199,14 +196,13 @@ llvm::StringRef UnixSignals::GetShortName(llvm::StringRef name) const {
 }
 
 int32_t UnixSignals::GetSignalNumberFromName(const char *name) const {
-  ConstString const_name(name);
+  llvm::StringRef name_ref(name);
 
   collection::const_iterator pos, end = m_signals.end();
   for (pos = m_signals.begin(); pos != end; pos++) {
-    if ((const_name == pos->second.m_name) ||
-        (const_name == pos->second.m_alias) ||
-        (const_name == GetShortName(pos->second.m_name)) ||
-        (const_name == GetShortName(pos->second.m_alias)))
+    if ((name_ref == pos->second.m_name) || (name_ref == pos->second.m_alias) ||
+        (name_ref == GetShortName(pos->second.m_name)) ||
+        (name_ref == GetShortName(pos->second.m_alias)))
       return pos->first;
   }
 
@@ -373,11 +369,10 @@ void UnixSignals::IncrementSignalHitCount(int signo) {
 
 json::Value UnixSignals::GetHitCountStatistics() const {
   json::Array json_signals;
-  for (const auto &pair: m_signals) {
+  for (const auto &pair : m_signals) {
     if (pair.second.m_hit_count > 0)
-      json_signals.emplace_back(json::Object{
-        { pair.second.m_name.GetCString(), pair.second.m_hit_count }
-      });
+      json_signals.emplace_back(
+          json::Object{{pair.second.m_name, pair.second.m_hit_count}});
   }
   return std::move(json_signals);
 }