[Lldb-commits] [lldb] [lldb] Guarded GDBRemoteCommunicationServerLLGS::m_saved_registers_map (PR #219431)
via lldb-commits
lldb-commits at lists.llvm.org
Fri Aug 28 03:02:02 PDT 2026
llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-lldb
Author: Raphael Isemann (Teemperor)
<details>
<summary>Changes</summary>
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.
---
Full diff: https://github.com/llvm/llvm-project/pull/219431.diff
4 Files Affected:
- (modified) lldb/include/lldb/Utility/Locked.h (+28)
- (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp (+9-10)
- (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h (+8-3)
- (modified) lldb/unittests/Utility/LockedTest.cpp (+37)
``````````diff
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/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;
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);
+}
``````````
</details>
https://github.com/llvm/llvm-project/pull/219431
More information about the lldb-commits
mailing list