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

[bulbazord]

Summary

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.

[DavidSpickett]

LGTM.

[fdeazeve]

I am slightly wary of making this a global set without any kind of thread safe mechanism, as I (naively, not really knowing how this class is used) would expect us to be able to instantiate multiple servers and have them be accessed concurrently. As such, it makes more sense to have each server own its set of strings.

Have we considered having UnixSignals be the one to extend the lifetime of strings? It seems that UnixSignals is the one storing the StringRefs and therefore requiring extended lifetime. I couldn't figure out if there is any other advantage to this vs the other suggestion.

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

49

Is there a strong motivation for making this a global static, instead of a private member of PlatformRemoteGDBServer?
My assumption here is that we only ever have one PlatformRemoteGDBServer active at any given time, otherwise (if concurrent servers now or in the future) the global approach is not thread safe.

[bulbazord]

In D159011#4624495, @fdeazeve wrote:

I am slightly wary of making this a global set without any kind of thread safe mechanism, as I (naively, not really knowing how this class is used) would expect us to be able to instantiate multiple servers and have them be accessed concurrently. As such, it makes more sense to have each server own its set of strings.

Have we considered having UnixSignals be the one to extend the lifetime of strings? It seems that UnixSignals is the one storing the StringRefs and therefore requiring extended lifetime. I couldn't figure out if there is any other advantage to this vs the other suggestion.

Right, that's a fair concern I think. The reason I chose to make it not the job of UnixSignals is that the majority of these strings are string literals. I think having the one instance where the strings are not literals somewhere in a data section should be responsible for guaranteeing the lifetimes of these strings. I did not want to put them in the ConstString string pool because I don't think that string pool needs any more strings in it than already exist there.

That being said, you suggested the StringSet be a private member variable of PlatformRemoteGDBServer. This may work, but I wasn't sure what the lifecycle of PlatformRemoteGDBServer objects were. Perhaps it exists for a time but is destroyed sometime later. At that point, UnixSignals will hold onto some dangling references. I decided to make it a global object to guarantee its lifetime independent of any PlatformRemoteGDBServer instance. I don't mind wrapping it in a concurrent structure to guarantee thread safety though.

[fdeazeve]

I decided to make it a global object to guarantee its lifetime independent of any PlatformRemoteGDBServer instance. I don't mind wrapping it in a concurrent structure to guarantee thread safety though.

At that point we would pretty much be re-implementing the notion of a ConstString but with a separate pool, right?

By the way, feel free to disregard this discussion, I just wanted to make sure we are aware that we lost one safety we had before.

[bulbazord]

In D159011#4625915, @fdeazeve wrote:

I decided to make it a global object to guarantee its lifetime independent of any PlatformRemoteGDBServer instance. I don't mind wrapping it in a concurrent structure to guarantee thread safety though.

At that point we would pretty much be re-implementing the notion of a ConstString but with a separate pool, right?

By the way, feel free to disregard this discussion, I just wanted to make sure we are aware that we lost one safety we had before.

Yep, that's basically correct. It would be a ConstString but a separate pool. I also don't want to disregard your point, I think it's a good one. LLDB already has enough thread-safety issues as-is...

[bulbazord]

Protect StringSet with a mutex

[DavidSpickett]

ping! (in the interest of clearing Phabricator reviews soon)

[fdeazeve]

LGTM!

lldb/include/lldb/Target/UnixSignals.h

 //===-- UnixSignals.h -------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_TARGET_UNIXSIGNALS_H
 #define LLDB_TARGET_UNIXSIGNALS_H
 
 #include <map>
 #include <optional>
 #include <string>
 #include <vector>
 
-#include "lldb/Utility/ConstString.h"
 #include "lldb/lldb-private.h"
 #include "llvm/Support/JSON.h"
 
 namespace lldb_private {
 
 class UnixSignals {
 public:
   static lldb::UnixSignalsSP Create(const ArchSpec &arch);
public:
   int32_t GetSignalAtIndex(int32_t index) const;
 
   // We assume that the elements of this object are constant once it is
   // constructed, since a process should never need to add or remove symbols as
   // it runs.  So don't call these functions anywhere but the constructor of
   // 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 };
 
   // Instead of calling this directly, use a ADD_SIGCODE macro to get compile
   // time checks when on the native platform.
   void AddSignalCode(
       int signo, int code, const llvm::StringLiteral description,
       SignalCodePrintOption print_option = SignalCodePrintOption::None);

 protected:
   // Classes that inherit from UnixSignals can see and modify these
 
   struct SignalCode {
     const llvm::StringLiteral m_description;
     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);
   };
 
   llvm::StringRef GetShortName(llvm::StringRef name) const;
 
   virtual void Reset();

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

 #include "lldb/Target/Target.h"
 #include "lldb/Utility/FileSpec.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/ProcessInfo.h"
 #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;
 using namespace lldb_private;
 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();
 
   if (!g_initialized) {
     g_initialized = true;
     PluginManager::RegisterPlugin(
         PlatformRemoteGDBServer::GetPluginNameStatic(),
bool done = array_sp->ForEach(
         if (object_sp && object_sp->IsValid())
           notify = object_sp->GetBooleanValue();
 
         std::string description;
         object_sp = dict->GetValueForKey("description");
         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;
       });
 
   if (done)
     m_remote_signals_sp = std::move(remote_signals_sp);
 
   return m_remote_signals_sp;
 }

lldb/source/Target/UnixSignals.cpp

 #include "lldb/Host/HostInfo.h"
 #include "lldb/Utility/ArchSpec.h"
 #include <optional>
 #include <sstream>
 
 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();
   switch (triple.getOS()) {
   case llvm::Triple::Linux:
     return std::make_shared<LinuxSignals>();
   case llvm::Triple::FreeBSD:
   case llvm::Triple::OpenBSD:
void UnixSignals::Reset() {
   AddSignal(27,     "SIGPROF",      false,    false,  false,  "profiling time alarm");
   AddSignal(28,     "SIGWINCH",     false,    false,  false,  "window size changes");
   AddSignal(29,     "SIGINFO",      false,    true,   true,   "information request");
   AddSignal(30,     "SIGUSR1",      false,    true,   true,   "user defined signal 1");
   AddSignal(31,     "SIGUSR2",      false,    true,   true,   "user defined signal 2");
   // 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));
   ++m_version;
 }
 
 void UnixSignals::AddSignalCode(int signo, int code,
                                 const llvm::StringLiteral description,
if (pos != m_signals.end())
     m_signals.erase(pos);
   ++m_version;
 }
 
 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
 UnixSignals::GetSignalDescription(int32_t signo, std::optional<int32_t> code,
                                   std::optional<lldb::addr_t> addr,
                                   std::optional<lldb::addr_t> lower,
                                   std::optional<lldb::addr_t> upper) const {
   std::string str;
 
   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 =
           pos->second.m_codes.find(*code);
       if (cpos != pos->second.m_codes.end()) {
         const SignalCode &sc = cpos->second;
         str += ": ";
         if (sc.m_print_option != SignalCodePrintOption::Bounds)
bool UnixSignals::SignalIsValid(int32_t signo) const {
   return m_signals.find(signo) != m_signals.end();
 }
 
 llvm::StringRef UnixSignals::GetShortName(llvm::StringRef name) const {
   return name.substr(3); // Remove "SIG" from name
 }
 
 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;
   }
 
   int32_t signo;
   if (llvm::to_integer(name, signo))
     return signo;
   return LLDB_INVALID_SIGNAL_NUMBER;
 }

 void UnixSignals::IncrementSignalHitCount(int signo) {
   collection::iterator pos = m_signals.find(signo);
   if (pos != m_signals.end())
     pos->second.m_hit_count += 1;
 }
 
 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);
 }
 
 void UnixSignals::Signal::Reset(bool reset_stop, bool reset_notify, 
                                 bool reset_suppress) {
   if (reset_stop)
     m_stop = m_default_stop;