[Lldb-commits] [lldb] [lldb] Use Guarded in MemoryRegionInfoCache (PR #219425)

[via lldb-commits]

llvmorg-github-actions[bot] wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Raphael Isemann (Teemperor)

<details>
<summary>Changes</summary>

This replaces the mutex + variable pair with a `Guarded` variable
that enforces the locking.

---
Full diff: https://github.com/llvm/llvm-project/pull/219425.diff


4 Files Affected:

- (modified) lldb/include/lldb/Target/MemoryRegionInfoCache.h (+3-2) 
- (modified) lldb/include/lldb/Utility/Locked.h (+28) 
- (modified) lldb/source/Target/MemoryRegionInfoCache.cpp (+6-11) 
- (modified) lldb/unittests/Utility/LockedTest.cpp (+37) 


``````````diff
diff --git a/lldb/include/lldb/Target/MemoryRegionInfoCache.h b/lldb/include/lldb/Target/MemoryRegionInfoCache.h
index 5117f1f598e55..7daea43671646 100644
--- a/lldb/include/lldb/Target/MemoryRegionInfoCache.h
+++ b/lldb/include/lldb/Target/MemoryRegionInfoCache.h
@@ -10,6 +10,7 @@
 #define LLDB_TARGET_MEMORYREGIONINFOCACHE_H
 
 #include "lldb/Target/MemoryRegionInfo.h"
+#include "lldb/Utility/Locked.h"
 
 #include <map>
 #include <mutex>
@@ -34,8 +35,8 @@ class MemoryRegionInfoCache {
   size_t GetSize();
 
 private:
-  std::map<lldb::addr_t, MemoryRegionInfo> m_region_infos;
-  std::mutex m_mutex;
+  Guarded<std::map<lldb::addr_t, MemoryRegionInfo>, std::mutex>
+      m_region_infos;
 };
 } // namespace lldb_private
 
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/Target/MemoryRegionInfoCache.cpp b/lldb/source/Target/MemoryRegionInfoCache.cpp
index 47694b3ad58fc..e84e56fbcc71e 100644
--- a/lldb/source/Target/MemoryRegionInfoCache.cpp
+++ b/lldb/source/Target/MemoryRegionInfoCache.cpp
@@ -12,21 +12,17 @@
 using namespace lldb;
 using namespace lldb_private;
 
-void MemoryRegionInfoCache::Clear() {
-  std::lock_guard<std::mutex> guard(m_mutex);
-  m_region_infos.clear();
-}
+void MemoryRegionInfoCache::Clear() { m_region_infos.Lock()->clear(); }
 
 size_t MemoryRegionInfoCache::GetSize() {
-  std::lock_guard<std::mutex> guard(m_mutex);
-  return m_region_infos.size();
+  return m_region_infos.Lock()->size();
 }
 
 std::optional<MemoryRegionInfo>
 MemoryRegionInfoCache::GetMemoryRegion(addr_t load_addr) {
-  std::lock_guard<std::mutex> guard(m_mutex);
-  auto it = m_region_infos.upper_bound(load_addr);
-  if (it == m_region_infos.begin())
+  auto region_infos = m_region_infos.Lock();
+  auto it = region_infos->upper_bound(load_addr);
+  if (it == region_infos->begin())
     return std::nullopt;
   --it;
   if (load_addr < it->second.GetRange().GetRangeEnd())
@@ -36,6 +32,5 @@ MemoryRegionInfoCache::GetMemoryRegion(addr_t load_addr) {
 }
 
 void MemoryRegionInfoCache::AddRegion(const MemoryRegionInfo &ri) {
-  std::lock_guard<std::mutex> guard(m_mutex);
-  m_region_infos.insert_or_assign(ri.GetRange().GetRangeBase(), ri);
+  m_region_infos.Lock()->insert_or_assign(ri.GetRange().GetRangeBase(), ri);
 }
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/219425