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 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
