https://github.com/Teemperor created https://github.com/llvm/llvm-project/pull/219431
m_saved_registers_map and m_next_saved_registers_id were guarded by a dedicated std::mutex. This replaces the manual locking logic with `Guarded`. This also fixes the missing lock in the Handle_QSaveRegisterState assert which was previously a race condition. >From d5fc2112dc99bc54f2e27388eb079a0e5535f61d Mon Sep 17 00:00:00 2001 From: Raphael Isemann <[email protected]> Date: Thu, 27 Aug 2026 15:28:32 +0100 Subject: [PATCH 1/2] [lldb] Add Guarded<T, Mutex> to Locked.h LLDB's code base has many variables that have an associated mutex that needs to be locked to safely access that variable from multiple threads. However, this locking scheme is currently not enforced by the compiler and code sometimes accesses these variables without aquiring the respective mutex first. This patch introduces a `Guarded` class that strictly enforces that some memory is only accessed after the respective mutex has been aquired. This class hands out `Locked` objects for every access which guarentee that the mutex is held as long as the variable is in scope. --- lldb/include/lldb/Utility/Locked.h | 28 ++++++++++++++++++++ lldb/unittests/Utility/LockedTest.cpp | 37 +++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/lldb/include/lldb/Utility/Locked.h b/lldb/include/lldb/Utility/Locked.h index acd34e454a918..f5d7b1aa8e4d9 100644 --- a/lldb/include/lldb/Utility/Locked.h +++ b/lldb/include/lldb/Utility/Locked.h @@ -168,6 +168,34 @@ template <typename T, typename Mutex = llvm::sys::RWMutex> using SharedLockedUP = SharedLocked<std::unique_ptr<const T>, Mutex>; /// @} +/// Bundles a value of type `T` with the `Mutex` that guards it. +/// +/// This class prevents accidential use of a value without aquiring the +/// lock and should be preferred over a member + mutex pair. +/// +/// `Mutex` must satisfy `Lockable` when calling `Lock()` and `SharedLockable` +/// when calling `LockShared()`. +template <typename T, typename Mutex = llvm::sys::RWMutex> class Guarded { +public: + Guarded() = default; + explicit Guarded(T value) : m_value(std::move(value)) {} + + Guarded(const Guarded &) = delete; + Guarded &operator=(const Guarded &) = delete; + + /// Exclusive (read/write) access to the value. + Locked<T *, Mutex> Lock() { return Locked<T *, Mutex>(m_mutex, &m_value); } + + /// Shared (read-only) access to the value. + SharedLocked<const T *, Mutex> LockShared() const { + return SharedLocked<const T *, Mutex>(m_mutex, &m_value); + } + +private: + mutable Mutex m_mutex; + T m_value{}; +}; + } // namespace lldb_private #endif // LLDB_UTILITY_LOCKED_H diff --git a/lldb/unittests/Utility/LockedTest.cpp b/lldb/unittests/Utility/LockedTest.cpp index cae24293dee2e..f898f21b56291 100644 --- a/lldb/unittests/Utility/LockedTest.cpp +++ b/lldb/unittests/Utility/LockedTest.cpp @@ -225,3 +225,40 @@ TEST(LockedTest, ExclusiveAccessOnRWMutex) { writer->value = 11; EXPECT_EQ(widget.value, 11); } + +// Guarded is neither copyable nor movable. +static_assert(!std::is_copy_constructible_v<Guarded<Widget>>); +static_assert(!std::is_move_constructible_v<Guarded<Widget>>); + +TEST(LockedTest, GuardedDefaultConstructed) { + Guarded<Widget> guarded; + EXPECT_EQ(guarded.Lock()->value, 0); +} + +TEST(LockedTest, GuardedValueConstructed) { + Guarded<Widget> guarded(Widget{42}); + EXPECT_EQ(guarded.Lock()->value, 42); +} + +TEST(LockedTest, GuardedExclusiveAccessMutatesValue) { + Guarded<Widget> guarded; + guarded.Lock()->value = 7; + EXPECT_EQ(guarded.Lock()->value, 7); +} + +TEST(LockedTest, GuardedSharedAccessIsReadOnly) { + Guarded<Widget> guarded(Widget{5}); + SharedLocked<const Widget *, llvm::sys::RWMutex> reader = + guarded.LockShared(); + EXPECT_EQ(reader->value, 5); + static_assert(std::is_same_v<decltype(reader.get()), const Widget *>, + "shared access borrows a const-qualified pointer"); +} + +// std::shared_mutex satisfies SharedLockable too, so Guarded works with it +// as a drop-in replacement for llvm::sys::RWMutex. +TEST(LockedTest, GuardedWorksWithStdSharedMutex) { + Guarded<Widget, std::shared_mutex> guarded; + guarded.Lock()->value = 3; + EXPECT_EQ(guarded.LockShared()->value, 3); +} >From 069bfe32e7bb973cd54dd5b6f1d4acbc361fa8db Mon Sep 17 00:00:00 2001 From: Raphael Isemann <[email protected]> Date: Thu, 27 Aug 2026 15:47:17 +0100 Subject: [PATCH 2/2] [lldb] Guarded GDBRemoteCommunicationServerLLGS::m_saved_registers_map m_saved_registers_map and m_next_saved_registers_id were guarded by a dedicated std::mutex. This replaces the manual locking logic with `Guarded`. This also fixes the missing lock in the Handle_QSaveRegisterState assert which was previously a race condition. --- .../GDBRemoteCommunicationServerLLGS.cpp | 19 +++++++++---------- .../GDBRemoteCommunicationServerLLGS.h | 11 ++++++++--- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp index cda790de16008..6446fb54fc145 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp @@ -3595,13 +3595,13 @@ GDBRemoteCommunicationServerLLGS::Handle_QSaveRegisterState( // Allocate a new save id. const uint32_t save_id = GetNextSavedRegistersID(); - assert((m_saved_registers_map.find(save_id) == m_saved_registers_map.end()) && - "GetNextRegisterSaveID() returned an existing register save id"); // Save the register data buffer under the save id. { - std::lock_guard<std::mutex> guard(m_saved_registers_mutex); - m_saved_registers_map[save_id] = register_data_sp; + auto saved_registers = m_saved_registers.Lock(); + assert((saved_registers->map.find(save_id) == saved_registers->map.end()) && + "GetNextRegisterSaveID() returned an existing register save id"); + saved_registers->map[save_id] = register_data_sp; } // Write the response. @@ -3645,11 +3645,11 @@ GDBRemoteCommunicationServerLLGS::Handle_QRestoreRegisterState( // Retrieve register state buffer, then remove from the list. DataBufferSP register_data_sp; { - std::lock_guard<std::mutex> guard(m_saved_registers_mutex); + auto saved_registers = m_saved_registers.Lock(); // Find the register set buffer for the given save id. - auto it = m_saved_registers_map.find(save_id); - if (it == m_saved_registers_map.end()) { + auto it = saved_registers->map.find(save_id); + if (it == saved_registers->map.end()) { LLDB_LOG(log, "pid {0} does not have a register set save buffer for id {1}", m_current_process->GetID(), save_id); @@ -3658,7 +3658,7 @@ GDBRemoteCommunicationServerLLGS::Handle_QRestoreRegisterState( register_data_sp = it->second; // Remove it from the map. - m_saved_registers_map.erase(it); + saved_registers->map.erase(it); } Status error = reg_context.WriteAllRegisterValues(register_data_sp); @@ -4425,8 +4425,7 @@ lldb::tid_t GDBRemoteCommunicationServerLLGS::GetCurrentThreadID() const { } uint32_t GDBRemoteCommunicationServerLLGS::GetNextSavedRegistersID() { - std::lock_guard<std::mutex> guard(m_saved_registers_mutex); - return m_next_saved_registers_id++; + return m_saved_registers.Lock()->next_id++; } void GDBRemoteCommunicationServerLLGS::ClearProcessSpecificData() { diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h index e5b4c9ec0bed0..dd4a4bd7bec3c 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h @@ -16,6 +16,7 @@ #include "lldb/Core/Communication.h" #include "lldb/Host/MainLoop.h" #include "lldb/Host/common/NativeProcessProtocol.h" +#include "lldb/Utility/Locked.h" #include "lldb/Utility/RegisterValue.h" #include "lldb/lldb-private-forward.h" @@ -133,9 +134,13 @@ class GDBRemoteCommunicationServerLLGS std::mutex m_pending_output_mutex; llvm::StringMap<std::unique_ptr<llvm::MemoryBuffer>> m_xfer_buffer_map; - std::mutex m_saved_registers_mutex; - std::unordered_map<uint32_t, lldb::DataBufferSP> m_saved_registers_map; - uint32_t m_next_saved_registers_id = 1; + + struct SavedRegisters { + std::unordered_map<uint32_t, lldb::DataBufferSP> map; + uint32_t next_id = 1; + }; + Guarded<SavedRegisters, std::mutex> m_saved_registers; + bool m_thread_suffix_supported = false; bool m_list_threads_in_stop_reply = false; bool m_non_stop = false; _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
