[lldb] Make Broadcaster mutexes non-recursive

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;

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());

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) {

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)));
+}