Author: athierry-oct Date: 2025-07-15T09:33:00-07:00 New Revision: 8612926c306c5191a5fb385dd11467728c59e982
URL: https://github.com/llvm/llvm-project/commit/8612926c306c5191a5fb385dd11467728c59e982 DIFF: https://github.com/llvm/llvm-project/commit/8612926c306c5191a5fb385dd11467728c59e982.diff LOG: [lldb] Fix race condition in Process::WaitForProcessToStop() (#144919) This PR addresses a race condition encountered when using LLDB through the Python scripting interface. I'm relatively new to LLDB, so feedback is very welcome, especially if there's a more appropriate way to address this issue. ### Bug Description When running a script that repeatedly calls `debugger.GetListener().WaitForEvent()` in a loop, and at some point invokes `process.Kill()` from within that loop to terminate the session, a race condition can occur if `process.Kill()` is called around the same time a breakpoint is hit. ### Race Condition Details The issue arises when the following sequence of events happens: 1. The process's **private state** transitions to `stopped` when the breakpoint is hit. 2. `process.Kill()` calls `Process::Destroy()`, which invokes `Process::WaitForProcessToStop()`. At this point: - `private_state = stopped` - `public_state = running` (the public state has not yet been updated) 3. The **public stop event** is broadcast **before** the hijack listener is installed. 4. As a result, the stop event is delivered to the **non-hijack listener**. 5. The interrupt request sent by `Process::StopForDestroyOrDetach()` is ignored because the process is already stopped (`private_state = stopped`). 6. No public stop event reaches the hijack listener. 7. `Process::WaitForProcessToStop()` hangs waiting for a public stop event, but the event is never received. 8. `process.Kill()` times out after 20 seconds ### Fix Summary This patch modifies `Process::WaitForProcessToStop()` to ensure that any pending events in the non-hijack listener queue are processed before checking the hijack listener. This guarantees that any missed public state change events are handled, preventing the hang. ### Additional Context A discussion of this issue, including a script to reproduce the bug, can be found here: [LLDB hangs when killing process at the same time a breakpoint is hit](https://discourse.llvm.org/t/lldb-hangs-when-killing-process-at-the-same-time-a-breakpoint-is-hit) Added: Modified: lldb/include/lldb/Utility/Listener.h lldb/source/Utility/Broadcaster.cpp lldb/source/Utility/Listener.cpp lldb/unittests/Utility/ListenerTest.cpp Removed: ################################################################################ diff --git a/lldb/include/lldb/Utility/Listener.h b/lldb/include/lldb/Utility/Listener.h index d48816ec0ea4d..393169091390c 100644 --- a/lldb/include/lldb/Utility/Listener.h +++ b/lldb/include/lldb/Utility/Listener.h @@ -53,6 +53,13 @@ class Listener : public std::enable_shared_from_this<Listener> { void AddEvent(lldb::EventSP &event); + /// Transfers all events matching the specified broadcaster and type to the + /// destination listener. This can be useful when setting up a hijack listener, + /// to ensure that no relevant events are missed. + void MoveEvents(lldb::ListenerSP destination, + Broadcaster *broadcaster, // nullptr for any broadcaster + uint32_t event_type_mask); + void Clear(); const char *GetName() { return m_name.c_str(); } diff --git a/lldb/source/Utility/Broadcaster.cpp b/lldb/source/Utility/Broadcaster.cpp index c6b2606afe0c8..41b782fee55fa 100644 --- a/lldb/source/Utility/Broadcaster.cpp +++ b/lldb/source/Utility/Broadcaster.cpp @@ -335,6 +335,18 @@ bool Broadcaster::BroadcasterImpl::HijackBroadcaster( "{0} Broadcaster(\"{1}\")::HijackBroadcaster (listener(\"{2}\")={3})", static_cast<void *>(this), GetBroadcasterName(), listener_sp->m_name.c_str(), static_cast<void *>(listener_sp.get())); + + // Move pending events from the previous listener to the hijack listener. + // This ensures that no relevant event queued before the transition is missed + // by the hijack listener. + ListenerSP prev_listener; + if (!m_hijacking_listeners.empty()) + prev_listener = m_hijacking_listeners.back(); + else if (m_primary_listener_sp) + prev_listener = m_primary_listener_sp; + if (prev_listener && listener_sp) + prev_listener->MoveEvents(listener_sp, &m_broadcaster, event_mask); + m_hijacking_listeners.push_back(listener_sp); m_hijacking_masks.push_back(event_mask); return true; @@ -367,6 +379,19 @@ void Broadcaster::BroadcasterImpl::RestoreBroadcaster() { static_cast<void *>(this), GetBroadcasterName(), listener_sp->m_name.c_str(), static_cast<void *>(listener_sp.get())); + + // Move any remaining events from the hijack listener back to + // the previous listener. This ensures that no events are dropped when + // restoring the original listener. + ListenerSP prev_listener; + if (m_hijacking_listeners.size() > 1) + prev_listener = m_hijacking_listeners[m_hijacking_listeners.size() - 2]; + else if (m_primary_listener_sp) + prev_listener = m_primary_listener_sp; + if (listener_sp && prev_listener && !m_hijacking_masks.empty()) + listener_sp->MoveEvents(prev_listener, &m_broadcaster, + m_hijacking_masks.back()); + m_hijacking_listeners.pop_back(); } if (!m_hijacking_masks.empty()) diff --git a/lldb/source/Utility/Listener.cpp b/lldb/source/Utility/Listener.cpp index d4ce3bf25ec5a..a9b733579d2f7 100644 --- a/lldb/source/Utility/Listener.cpp +++ b/lldb/source/Utility/Listener.cpp @@ -176,6 +176,33 @@ void Listener::AddEvent(EventSP &event_sp) { m_events_condition.notify_all(); } +void Listener::MoveEvents( + ListenerSP destination, + Broadcaster *broadcaster, // nullptr for any broadcaster + uint32_t event_type_mask) { + Log *log = GetLog(LLDBLog::Events); + + std::lock_guard<std::mutex> guard(m_events_mutex); + auto pos = m_events.begin(); + while (pos != m_events.end()) { + EventSP &event_sp = *pos; + if (event_sp && + ((broadcaster == nullptr) || event_sp->BroadcasterIs(broadcaster)) && + (event_type_mask == 0 || event_type_mask & event_sp->GetType())) { + LLDB_LOGF( + log, "%p Listener('%s')::MoveEvents moving event %p to %p('%s')", + static_cast<void *>(this), m_name.c_str(), + static_cast<void *>(event_sp.get()), + static_cast<void *>(destination.get()), destination->GetName()); + + destination->AddEvent(event_sp); + pos = m_events.erase(pos); + } else { + ++pos; + } + } +} + bool Listener::FindNextEventInternal( std::unique_lock<std::mutex> &lock, Broadcaster *broadcaster, // nullptr for any broadcaster diff --git a/lldb/unittests/Utility/ListenerTest.cpp b/lldb/unittests/Utility/ListenerTest.cpp index f7aa0f59d1848..a980a9f01c488 100644 --- a/lldb/unittests/Utility/ListenerTest.cpp +++ b/lldb/unittests/Utility/ListenerTest.cpp @@ -150,3 +150,84 @@ TEST(ListenerTest, StartStopListeningForEventSpec) { ASSERT_EQ(event_sp->GetBroadcaster(), &broadcaster1); ASSERT_FALSE(listener_sp->GetEvent(event_sp, std::chrono::seconds(0))); } + +TEST(ListenerTest, MoveEventsOnHijackAndRestore) { + Broadcaster broadcaster(nullptr, "test-broadcaster"); + const uint32_t event_mask = 1; + EventSP event_sp; + + // Create the original listener and start listening. + ListenerSP original_listener = Listener::MakeListener("original-listener"); + ASSERT_EQ(event_mask, original_listener->StartListeningForEvents(&broadcaster, + event_mask)); + broadcaster.SetPrimaryListener(original_listener); + + // Queue two events to original listener, but do not consume them yet. + broadcaster.BroadcastEvent(event_mask, nullptr); + broadcaster.BroadcastEvent(event_mask, nullptr); + + // Hijack. + ListenerSP hijack_listener = Listener::MakeListener("hijack-listener"); + broadcaster.HijackBroadcaster(hijack_listener, event_mask); + + // The events should have been moved to the hijack listener. + EXPECT_FALSE(original_listener->GetEvent(event_sp, std::chrono::seconds(0))); + EXPECT_TRUE(hijack_listener->GetEvent(event_sp, std::chrono::seconds(0))); + EXPECT_TRUE(hijack_listener->GetEvent(event_sp, std::chrono::seconds(0))); + + // Queue two events while hijacked, but do not consume them yet. + broadcaster.BroadcastEvent(event_mask, nullptr); + broadcaster.BroadcastEvent(event_mask, nullptr); + + // Restore the original listener. + broadcaster.RestoreBroadcaster(); + + // The events queued while hijacked should have been moved back to the + // original listener. + EXPECT_FALSE(hijack_listener->GetEvent(event_sp, std::chrono::seconds(0))); + EXPECT_TRUE(original_listener->GetEvent(event_sp, std::chrono::seconds(0))); + EXPECT_TRUE(original_listener->GetEvent(event_sp, std::chrono::seconds(0))); +} + +TEST(ListenerTest, MoveEventsBetweenHijackers) { + Broadcaster broadcaster(nullptr, "test-broadcaster"); + const uint32_t event_mask = 1; + EventSP event_sp; + + // Create the original listener and start listening. + ListenerSP original_listener = Listener::MakeListener("original-listener"); + ASSERT_EQ(event_mask, original_listener->StartListeningForEvents(&broadcaster, + event_mask)); + broadcaster.SetPrimaryListener(original_listener); + + // First hijack. + ListenerSP hijack_listener1 = Listener::MakeListener("hijack-listener1"); + broadcaster.HijackBroadcaster(hijack_listener1, event_mask); + + // Queue two events while hijacked, but do not consume + // them yet. + broadcaster.BroadcastEvent(event_mask, nullptr); + broadcaster.BroadcastEvent(event_mask, nullptr); + + // Second hijack. + ListenerSP hijack_listener2 = Listener::MakeListener("hijack-listener2"); + broadcaster.HijackBroadcaster(hijack_listener2, event_mask); + + // The second hijacker should have the events now. + EXPECT_FALSE(hijack_listener1->GetEvent(event_sp, std::chrono::seconds(0))); + EXPECT_TRUE(hijack_listener2->GetEvent(event_sp, std::chrono::seconds(0))); + EXPECT_TRUE(hijack_listener2->GetEvent(event_sp, std::chrono::seconds(0))); + + // Queue two events while hijacked with second hijacker, but do not consume + // them yet. + broadcaster.BroadcastEvent(event_mask, nullptr); + broadcaster.BroadcastEvent(event_mask, nullptr); + + // Restore the previous hijacker. + broadcaster.RestoreBroadcaster(); + + // The first hijacker should now have the events. + EXPECT_FALSE(hijack_listener2->GetEvent(event_sp, std::chrono::seconds(0))); + EXPECT_TRUE(hijack_listener1->GetEvent(event_sp, std::chrono::seconds(0))); + EXPECT_TRUE(hijack_listener1->GetEvent(event_sp, std::chrono::seconds(0))); +} _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
