[Lldb-commits] [lldb] 2a14c06 - [lldb] Make Broadcaster mutexes non-recursive (#97400)
via lldb-commits
lldb-commits at lists.llvm.org
Wed Jul 3 01:29:21 PDT 2024
Author: Pavel Labath
Date: 2024-07-03T10:29:17+02:00
New Revision: 2a14c0643597c5932af85f22172c99800f9b4a6c
URL: https://github.com/llvm/llvm-project/commit/2a14c0643597c5932af85f22172c99800f9b4a6c
DIFF: https://github.com/llvm/llvm-project/commit/2a14c0643597c5932af85f22172c99800f9b4a6c.diff
LOG: [lldb] Make Broadcaster mutexes non-recursive (#97400)
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.
Added:
Modified:
lldb/include/lldb/Utility/Broadcaster.h
lldb/source/Utility/Broadcaster.cpp
lldb/source/Utility/Listener.cpp
lldb/unittests/Utility/ListenerTest.cpp
Removed:
################################################################################
diff --git a/lldb/include/lldb/Utility/Broadcaster.h b/lldb/include/lldb/Utility/Broadcaster.h
index 58436ddb9f26d..c6f63f1916573 100644
--- 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
index b6d8ae39325d3..c6b2606afe0c8 100644
--- 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
index 5aacb4104e1cf..0b28cb5cdc642 100644
--- 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
index e586d99fd6305..f7aa0f59d1848 100644
--- 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)));
+}
More information about the lldb-commits
mailing list