[lldb] Make Broadcaster mutexes non-recursive (#97400)

labath · web-flow · commit 2a14c0643597 · 2024-07-03T10:29:17.000+02:00
Non-recursive mutexes encourage better locking discipline and avoid bugs like #96750, where one can unexpectedly re-enter the critical section on the same thread, and interrupt a presumed-indivisible operation. In this case, the only needed fix was to remove locking from some BroadcastManager functions, which were only called from the Listener class (and the listener already locked those mutexes to preserve lock ordering). While doing that, I noticed we don't have unit tests for these functions, so I added one.
diff --git a/lldb/include/lldb/Utility/Broadcaster.h b/lldb/include/lldb/Utility/Broadcaster.h
@@ -87,12 +87,6 @@ class BroadcasterManager
 
   ~BroadcasterManager() = default;
 
-  uint32_t RegisterListenerForEvents(const lldb::ListenerSP &listener_sp,
-                                     const BroadcastEventSpec &event_spec);
-
-  bool UnregisterListenerForEvents(const lldb::ListenerSP &listener_sp,
-                                   const BroadcastEventSpec &event_spec);
-
   lldb::ListenerSP
   GetListenerForEventSpec(const BroadcastEventSpec &event_spec) const;
 
@@ -105,13 +99,20 @@ class BroadcasterManager
   void Clear();
 
 private:
+  uint32_t
+  RegisterListenerForEventsNoLock(const lldb::ListenerSP &listener_sp,
+                                  const BroadcastEventSpec &event_spec);
+
+  bool UnregisterListenerForEventsNoLock(const lldb::ListenerSP &listener_sp,
+                                         const BroadcastEventSpec &event_spec);
+
   typedef std::pair<BroadcastEventSpec, lldb::ListenerSP> event_listener_key;
   typedef std::map<BroadcastEventSpec, lldb::ListenerSP> collection;
   typedef std::set<lldb::ListenerSP> listener_collection;
   collection m_event_map;
   listener_collection m_listeners;
 
-  mutable std::recursive_mutex m_manager_mutex;
+  mutable std::mutex m_manager_mutex;
 };
 
 /// \class Broadcaster Broadcaster.h "lldb/Utility/Broadcaster.h" An event
@@ -441,7 +442,7 @@ class Broadcaster {
     collection m_listeners;
 
     /// A mutex that protects \a m_listeners.
-    std::recursive_mutex m_listeners_mutex;
+    std::mutex m_listeners_mutex;
 
     /// See the discussion of Broadcasters and Listeners above.
     lldb::ListenerSP m_primary_listener_sp;
diff --git a/lldb/source/Utility/Broadcaster.cpp b/lldb/source/Utility/Broadcaster.cpp
@@ -87,7 +87,7 @@ bool Broadcaster::BroadcasterImpl::HasListeners(uint32_t event_mask) {
 }
 
 void Broadcaster::BroadcasterImpl::Clear() {
-  std::lock_guard<std::recursive_mutex> guard(m_listeners_mutex);
+  std::lock_guard<std::mutex> guard(m_listeners_mutex);
 
   // Make sure the listener forgets about this broadcaster. We do this in the
   // broadcaster in case the broadcaster object initiates the removal.
@@ -137,7 +137,7 @@ Broadcaster::BroadcasterImpl::AddListener(const lldb::ListenerSP &listener_sp,
   if (!listener_sp)
     return 0;
 
-  std::lock_guard<std::recursive_mutex> guard(m_listeners_mutex);
+  std::lock_guard<std::mutex> guard(m_listeners_mutex);
 
   // See if we already have this listener, and if so, update its mask
 
@@ -171,7 +171,7 @@ Broadcaster::BroadcasterImpl::AddListener(const lldb::ListenerSP &listener_sp,
 }
 
 bool Broadcaster::BroadcasterImpl::EventTypeHasListeners(uint32_t event_type) {
-  std::lock_guard<std::recursive_mutex> guard(m_listeners_mutex);
+  std::lock_guard<std::mutex> guard(m_listeners_mutex);
 
   if (!m_hijacking_listeners.empty() && event_type & m_hijacking_masks.back())
     return true;
@@ -195,7 +195,7 @@ bool Broadcaster::BroadcasterImpl::RemoveListener(
     return true;
   }
 
-  std::lock_guard<std::recursive_mutex> guard(m_listeners_mutex);
+  std::lock_guard<std::mutex> guard(m_listeners_mutex);
   for (auto it = m_listeners.begin(); it != m_listeners.end();) {
     lldb::ListenerSP curr_listener_sp(it->first.lock());
 
@@ -243,7 +243,7 @@ void Broadcaster::BroadcasterImpl::PrivateBroadcastEvent(EventSP &event_sp,
 
   const uint32_t event_type = event_sp->GetType();
 
-  std::lock_guard<std::recursive_mutex> guard(m_listeners_mutex);
+  std::lock_guard<std::mutex> guard(m_listeners_mutex);
 
   ListenerSP hijacking_listener_sp;
 
@@ -327,7 +327,7 @@ void Broadcaster::BroadcasterImpl::SetPrimaryListener(lldb::ListenerSP
 
 bool Broadcaster::BroadcasterImpl::HijackBroadcaster(
     const lldb::ListenerSP &listener_sp, uint32_t event_mask) {
-  std::lock_guard<std::recursive_mutex> guard(m_listeners_mutex);
+  std::lock_guard<std::mutex> guard(m_listeners_mutex);
 
   Log *log = GetLog(LLDBLog::Events);
   LLDB_LOG(
@@ -341,7 +341,7 @@ bool Broadcaster::BroadcasterImpl::HijackBroadcaster(
 }
 
 bool Broadcaster::BroadcasterImpl::IsHijackedForEvent(uint32_t event_mask) {
-  std::lock_guard<std::recursive_mutex> guard(m_listeners_mutex);
+  std::lock_guard<std::mutex> guard(m_listeners_mutex);
 
   if (!m_hijacking_listeners.empty())
     return (event_mask & m_hijacking_masks.back()) != 0;
@@ -356,7 +356,7 @@ const char *Broadcaster::BroadcasterImpl::GetHijackingListenerName() {
 }
 
 void Broadcaster::BroadcasterImpl::RestoreBroadcaster() {
-  std::lock_guard<std::recursive_mutex> guard(m_listeners_mutex);
+  std::lock_guard<std::mutex> guard(m_listeners_mutex);
 
   if (!m_hijacking_listeners.empty()) {
     ListenerSP listener_sp = m_hijacking_listeners.back();
@@ -391,10 +391,8 @@ lldb::BroadcasterManagerSP BroadcasterManager::MakeBroadcasterManager() {
   return lldb::BroadcasterManagerSP(new BroadcasterManager());
 }
 
-uint32_t BroadcasterManager::RegisterListenerForEvents(
+uint32_t BroadcasterManager::RegisterListenerForEventsNoLock(
     const lldb::ListenerSP &listener_sp, const BroadcastEventSpec &event_spec) {
-  std::lock_guard<std::recursive_mutex> guard(m_manager_mutex);
-
   collection::iterator iter = m_event_map.begin(), end_iter = m_event_map.end();
   uint32_t available_bits = event_spec.GetEventBits();
 
@@ -419,9 +417,8 @@ uint32_t BroadcasterManager::RegisterListenerForEvents(
   return available_bits;
 }
 
-bool BroadcasterManager::UnregisterListenerForEvents(
+bool BroadcasterManager::UnregisterListenerForEventsNoLock(
     const lldb::ListenerSP &listener_sp, const BroadcastEventSpec &event_spec) {
-  std::lock_guard<std::recursive_mutex> guard(m_manager_mutex);
   bool removed_some = false;
 
   if (m_listeners.erase(listener_sp) == 0)
@@ -464,7 +461,7 @@ bool BroadcasterManager::UnregisterListenerForEvents(
 
 ListenerSP BroadcasterManager::GetListenerForEventSpec(
     const BroadcastEventSpec &event_spec) const {
-  std::lock_guard<std::recursive_mutex> guard(m_manager_mutex);
+  std::lock_guard<std::mutex> guard(m_manager_mutex);
 
   auto event_spec_matches =
       [&event_spec](const event_listener_key &input) -> bool {
@@ -479,7 +476,7 @@ ListenerSP BroadcasterManager::GetListenerForEventSpec(
 }
 
 void BroadcasterManager::RemoveListener(Listener *listener) {
-  std::lock_guard<std::recursive_mutex> guard(m_manager_mutex);
+  std::lock_guard<std::mutex> guard(m_manager_mutex);
   auto listeners_predicate =
       [&listener](const lldb::ListenerSP &input) -> bool {
     return input.get() == listener;
@@ -504,7 +501,7 @@ void BroadcasterManager::RemoveListener(Listener *listener) {
 }
 
 void BroadcasterManager::RemoveListener(const lldb::ListenerSP &listener_sp) {
-  std::lock_guard<std::recursive_mutex> guard(m_manager_mutex);
+  std::lock_guard<std::mutex> guard(m_manager_mutex);
 
   auto listener_matches =
       [&listener_sp](const event_listener_key &input) -> bool {
@@ -526,7 +523,7 @@ void BroadcasterManager::RemoveListener(const lldb::ListenerSP &listener_sp) {
 
 void BroadcasterManager::SignUpListenersForBroadcaster(
     Broadcaster &broadcaster) {
-  std::lock_guard<std::recursive_mutex> guard(m_manager_mutex);
+  std::lock_guard<std::mutex> guard(m_manager_mutex);
 
   collection::iterator iter = m_event_map.begin(), end_iter = m_event_map.end();
 
@@ -544,7 +541,7 @@ void BroadcasterManager::SignUpListenersForBroadcaster(
 }
 
 void BroadcasterManager::Clear() {
-  std::lock_guard<std::recursive_mutex> guard(m_manager_mutex);
+  std::lock_guard<std::mutex> guard(m_manager_mutex);
 
   for (auto &listener : m_listeners)
     listener->BroadcasterManagerWillDestruct(this->shared_from_this());
diff --git a/lldb/source/Utility/Listener.cpp b/lldb/source/Utility/Listener.cpp
@@ -356,11 +356,10 @@ Listener::StartListeningForEventSpec(const BroadcasterManagerSP &manager_sp,
   };
   // The BroadcasterManager mutex must be locked before m_broadcasters_mutex to
   // avoid violating the lock hierarchy (manager before broadcasters).
-  std::lock_guard<std::recursive_mutex> manager_guard(
-      manager_sp->m_manager_mutex);
+  std::lock_guard<std::mutex> manager_guard(manager_sp->m_manager_mutex);
   std::lock_guard<std::recursive_mutex> guard(m_broadcasters_mutex);
 
-  uint32_t bits_acquired = manager_sp->RegisterListenerForEvents(
+  uint32_t bits_acquired = manager_sp->RegisterListenerForEventsNoLock(
       this->shared_from_this(), event_spec);
   if (bits_acquired) {
     BroadcasterManagerWP manager_wp(manager_sp);
@@ -377,9 +376,12 @@ bool Listener::StopListeningForEventSpec(const BroadcasterManagerSP &manager_sp,
   if (!manager_sp)
     return false;
 
+  // The BroadcasterManager mutex must be locked before m_broadcasters_mutex to
+  // avoid violating the lock hierarchy (manager before broadcasters).
+  std::lock_guard<std::mutex> manager_guard(manager_sp->m_manager_mutex);
   std::lock_guard<std::recursive_mutex> guard(m_broadcasters_mutex);
-  return manager_sp->UnregisterListenerForEvents(this->shared_from_this(),
-                                                 event_spec);
+  return manager_sp->UnregisterListenerForEventsNoLock(this->shared_from_this(),
+                                                       event_spec);
 }
 
 ListenerSP Listener::MakeListener(const char *name) {
diff --git a/lldb/unittests/Utility/ListenerTest.cpp b/lldb/unittests/Utility/ListenerTest.cpp
@@ -9,6 +9,7 @@
 #include "gtest/gtest.h"
 
 #include "lldb/Utility/Broadcaster.h"
+#include "lldb/Utility/Event.h"
 #include "lldb/Utility/Listener.h"
 #include <future>
 #include <thread>
@@ -111,3 +112,41 @@ TEST(ListenerTest, GetEventWait) {
       &broadcaster, event_mask, event_sp, std::nullopt));
   async_broadcast.get();
 }
+
+TEST(ListenerTest, StartStopListeningForEventSpec) {
+  constexpr uint32_t event_mask = 1;
+  static constexpr llvm::StringLiteral broadcaster_class = "broadcaster-class";
+
+  class TestBroadcaster : public Broadcaster {
+    using Broadcaster::Broadcaster;
+    llvm::StringRef GetBroadcasterClass() const override {
+      return broadcaster_class;
+    }
+  };
+
+  BroadcasterManagerSP manager_sp =
+      BroadcasterManager::MakeBroadcasterManager();
+  ListenerSP listener_sp = Listener::MakeListener("test-listener");
+
+  // Create two broadcasters, one while we're waiting for new broadcasters, and
+  // one when we're not.
+  ASSERT_EQ(listener_sp->StartListeningForEventSpec(
+                manager_sp, BroadcastEventSpec(broadcaster_class, event_mask)),
+            event_mask);
+  TestBroadcaster broadcaster1(manager_sp, "test-broadcaster-1");
+  broadcaster1.CheckInWithManager();
+  ASSERT_TRUE(listener_sp->StopListeningForEventSpec(
+      manager_sp, BroadcastEventSpec(broadcaster_class, event_mask)));
+  TestBroadcaster broadcaster2(manager_sp, "test-broadcaster-2");
+  broadcaster2.CheckInWithManager();
+
+  // Use both broadcasters to send an event.
+  for (auto *b : {&broadcaster1, &broadcaster2})
+    b->BroadcastEvent(event_mask, nullptr);
+
+  // Use should only get the event from the first one.
+  EventSP event_sp;
+  ASSERT_TRUE(listener_sp->GetEvent(event_sp, std::chrono::seconds(0)));
+  ASSERT_EQ(event_sp->GetBroadcaster(), &broadcaster1);
+  ASSERT_FALSE(listener_sp->GetEvent(event_sp, std::chrono::seconds(0)));
+}
