From 2f377c5bd713e9ee72faa6a99088bb81358059e3 Mon Sep 17 00:00:00 2001 From: Alex Langford Date: Mon, 28 Aug 2023 11:59:09 -0700 Subject: [PATCH] [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 --- lldb/include/lldb/Target/UnixSignals.h | 19 +++++---- .../gdb-server/PlatformRemoteGDBServer.cpp | 21 +++++++++- lldb/source/Target/UnixSignals.cpp | 41 ++++++++----------- 3 files changed, 48 insertions(+), 33 deletions(-) diff --git a/lldb/include/lldb/Target/UnixSignals.h b/lldb/include/lldb/Target/UnixSignals.h index b227b67e2892..b3605ccefddb 100644 --- a/lldb/include/lldb/Target/UnixSignals.h +++ b/lldb/include/lldb/Target/UnixSignals.h @@ -14,7 +14,6 @@ #include #include -#include "lldb/Utility/ConstString.h" #include "lldb/lldb-private.h" #include "llvm/Support/JSON.h" @@ -101,9 +100,10 @@ public: // 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 @@ protected: 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 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); diff --git a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp b/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp index 8734a3666a6d..88f1ad15b6b4 100644 --- a/lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp +++ b/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 #include 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 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; }); diff --git a/lldb/source/Target/UnixSignals.cpp b/lldb/source/Target/UnixSignals.cpp index b8e2a7675f7b..e3c7a83ece07 100644 --- a/lldb/source/Target/UnixSignals.cpp +++ b/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 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::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); }