[Lldb-commits] [PATCH] D159011: [lldb][NFCI] Remove use of ConstString from UnixSignals

[Alex Langford via Phabricator via lldb-commits]

bulbazord updated this revision to Diff 554504.
bulbazord added a comment.

Protect StringSet with a mutex


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159011/new/

https://reviews.llvm.org/D159011

Files:
  lldb/include/lldb/Target/UnixSignals.h
  lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
  lldb/source/Target/UnixSignals.cpp


Index: lldb/source/Target/UnixSignals.cpp
===================================================================
--- lldb/source/Target/UnixSignals.cpp
+++ 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 @@
   // 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 @@
   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 @@
 
   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 @@
 }
 
 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 @@
 
 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);
 }
Index: lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
===================================================================
--- lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
+++ 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 @@
 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 @@
         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;
       });
 
Index: lldb/include/lldb/Target/UnixSignals.h
===================================================================
--- lldb/include/lldb/Target/UnixSignals.h
+++ 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 @@
   // 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 @@
     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-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits