[Lldb-commits] [lldb] 44d9692 - Private process events were being delivered to the secondary listener (#98571)
via lldb-commits
lldb-commits at lists.llvm.org
Mon Jul 15 15:07:06 PDT 2024
Author: jimingham
Date: 2024-07-15T15:07:01-07:00
New Revision: 44d9692e6a657ec46e98e4912ac56417da67cfee
URL: https://github.com/llvm/llvm-project/commit/44d9692e6a657ec46e98e4912ac56417da67cfee
DIFF: https://github.com/llvm/llvm-project/commit/44d9692e6a657ec46e98e4912ac56417da67cfee.diff
LOG: Private process events were being delivered to the secondary listener (#98571)
This fixes a bug where Process events were being delivered to secondary
listeners when the Private state thread listener was processing the
event. That meant the secondary listener could get an event before the
Primary listener did. That in turn meant the state when the secondary
listener got the event wasn't right yet. Plus it meant that the
secondary listener saw more events than the primary (not all events get
forwarded from the private to the public Process listener.)
This bug became much more evident when we had a stop hook that did some
work, since that delays the Primary listener event delivery. So I also
added a stop-hook to the test, and put a little delay in as well.
Added:
Modified:
lldb/include/lldb/Target/Process.h
lldb/include/lldb/Utility/Event.h
lldb/source/Target/Process.cpp
lldb/source/Utility/Event.cpp
lldb/test/API/python_api/event/TestEvents.py
Removed:
################################################################################
diff --git a/lldb/include/lldb/Target/Process.h b/lldb/include/lldb/Target/Process.h
index ceaf547ebddaf..c8475db8ae160 100644
--- a/lldb/include/lldb/Target/Process.h
+++ b/lldb/include/lldb/Target/Process.h
@@ -465,6 +465,8 @@ class Process : public std::enable_shared_from_this<Process>,
static bool SetUpdateStateOnRemoval(Event *event_ptr);
private:
+ bool ForwardEventToPendingListeners(Event *event_ptr) override;
+
void SetUpdateStateOnRemoval() { m_update_state++; }
void SetRestarted(bool new_value) { m_restarted = new_value; }
diff --git a/lldb/include/lldb/Utility/Event.h b/lldb/include/lldb/Utility/Event.h
index 461d711b8c3f2..4f58f257d4a26 100644
--- a/lldb/include/lldb/Utility/Event.h
+++ b/lldb/include/lldb/Utility/Event.h
@@ -48,6 +48,17 @@ class EventData {
virtual void Dump(Stream *s) const;
private:
+ /// This will be queried for a Broadcaster with a primary and some secondary
+ /// listeners after the primary listener pulled the event from the event queue
+ /// and ran its DoOnRemoval, right before the event is delivered.
+ /// If it returns true, the event will also be forwarded to the secondary
+ /// listeners, and if false, event propagation stops at the primary listener.
+ /// Some broadcasters (particularly the Process broadcaster) fetch events on
+ /// a private Listener, and then forward the event to the Public Listeners
+ /// after some processing. The Process broadcaster does not want to forward
+ /// to the secondary listeners at the private processing stage.
+ virtual bool ForwardEventToPendingListeners(Event *event_ptr) { return true; }
+
virtual void DoOnRemoval(Event *event_ptr) {}
EventData(const EventData &) = delete;
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index b91446e1c0e49..7abe5dcb88845 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -4236,7 +4236,22 @@ bool Process::ProcessEventData::ShouldStop(Event *event_ptr,
return still_should_stop;
}
+bool Process::ProcessEventData::ForwardEventToPendingListeners(
+ Event *event_ptr) {
+ // STDIO and the other async event notifications should always be forwarded.
+ if (event_ptr->GetType() != Process::eBroadcastBitStateChanged)
+ return true;
+
+ // For state changed events, if the update state is zero, we are handling
+ // this on the private state thread. We should wait for the public event.
+ return m_update_state == 1;
+}
+
void Process::ProcessEventData::DoOnRemoval(Event *event_ptr) {
+ // We only have work to do for state changed events:
+ if (event_ptr->GetType() != Process::eBroadcastBitStateChanged)
+ return;
+
ProcessSP process_sp(m_process_wp.lock());
if (!process_sp)
diff --git a/lldb/source/Utility/Event.cpp b/lldb/source/Utility/Event.cpp
index 863167e56bce6..5f431c0a6dd89 100644
--- a/lldb/source/Utility/Event.cpp
+++ b/lldb/source/Utility/Event.cpp
@@ -83,14 +83,20 @@ void Event::Dump(Stream *s) const {
void Event::DoOnRemoval() {
std::lock_guard<std::mutex> guard(m_listeners_mutex);
- if (m_data_sp)
- m_data_sp->DoOnRemoval(this);
+ if (!m_data_sp)
+ return;
+
+ m_data_sp->DoOnRemoval(this);
+
// Now that the event has been handled by the primary event Listener, forward
// it to the other Listeners.
+
EventSP me_sp = shared_from_this();
- for (auto listener_sp : m_pending_listeners)
- listener_sp->AddEvent(me_sp);
- m_pending_listeners.clear();
+ if (m_data_sp->ForwardEventToPendingListeners(this)) {
+ for (auto listener_sp : m_pending_listeners)
+ listener_sp->AddEvent(me_sp);
+ m_pending_listeners.clear();
+ }
}
#pragma mark -
diff --git a/lldb/test/API/python_api/event/TestEvents.py b/lldb/test/API/python_api/event/TestEvents.py
index d8d3dd2d2b01b..fb1a7e3bc6d3a 100644
--- a/lldb/test/API/python_api/event/TestEvents.py
+++ b/lldb/test/API/python_api/event/TestEvents.py
@@ -7,7 +7,7 @@
from lldbsuite.test.decorators import *
from lldbsuite.test.lldbtest import *
from lldbsuite.test import lldbutil
-
+import random
@skipIfLinux # llvm.org/pr25924, sometimes generating SIGSEGV
class EventAPITestCase(TestBase):
@@ -20,6 +20,7 @@ def setUp(self):
self.line = line_number(
"main.c", '// Find the line number of function "c" here.'
)
+ random.seed()
@expectedFailureAll(
oslist=["linux"], bugnumber="llvm.org/pr23730 Flaky, fails ~1/10 cases"
@@ -318,6 +319,7 @@ def wait_for_next_event(self, expected_state, test_shadow=False):
"""Wait for an event from self.primary & self.shadow listener.
If test_shadow is true, we also check that the shadow listener only
receives events AFTER the primary listener does."""
+ import stop_hook
# Waiting on the shadow listener shouldn't have events yet because
# we haven't fetched them for the primary listener yet:
event = lldb.SBEvent()
@@ -328,12 +330,23 @@ def wait_for_next_event(self, expected_state, test_shadow=False):
# But there should be an event for the primary listener:
success = self.primary_listener.WaitForEvent(5, event)
+
self.assertTrue(success, "Primary listener got the event")
state = lldb.SBProcess.GetStateFromEvent(event)
+ primary_event_type = event.GetType()
restart = False
if state == lldb.eStateStopped:
restart = lldb.SBProcess.GetRestartedFromEvent(event)
+ # This counter is matching the stop hooks, which don't get run
+ # for auto-restarting stops.
+ if not restart:
+ self.stop_counter += 1
+ self.assertEqual(
+ stop_hook.StopHook.counter[self.instance],
+ self.stop_counter,
+ "matching stop hook",
+ )
if expected_state is not None:
self.assertEqual(
@@ -344,15 +357,18 @@ def wait_for_next_event(self, expected_state, test_shadow=False):
# listener:
success = self.shadow_listener.WaitForEvent(5, event)
self.assertTrue(success, "Shadow listener got event too")
+ shadow_event_type = event.GetType()
+ self.assertEqual(
+ primary_event_type, shadow_event_type, "It was the same event type"
+ )
self.assertEqual(
- state, lldb.SBProcess.GetStateFromEvent(event), "It was the same event"
+ state, lldb.SBProcess.GetStateFromEvent(event), "It was the same state"
)
self.assertEqual(
restart,
lldb.SBProcess.GetRestartedFromEvent(event),
"It was the same restarted",
)
-
return state, restart
@expectedFlakeyLinux("llvm.org/pr23730") # Flaky, fails ~1/100 cases
@@ -386,6 +402,20 @@ def test_shadow_listener(self):
)
self.dbg.SetAsync(True)
+ # Now make our stop hook - we want to ensure it stays up to date with
+ # the events. We can't get our hands on the stop-hook instance directly,
+ # so we'll pass in an instance key, and use that to retrieve the data from
+ # this instance of the stop hook:
+ self.instance = f"Key{random.randint(0,10000)}"
+ stop_hook_path = os.path.join(self.getSourceDir(), "stop_hook.py")
+ self.runCmd(f"command script import {stop_hook_path}")
+ import stop_hook
+
+ self.runCmd(
+ f"target stop-hook add -P stop_hook.StopHook -k instance -v {self.instance}"
+ )
+ self.stop_counter = 0
+
self.process = target.Launch(launch_info, error)
self.assertSuccess(error, "Process launched successfully")
@@ -395,6 +425,7 @@ def test_shadow_listener(self):
# Events in the launch sequence might be platform dependent, so don't
# expect any particular event till we get the stopped:
state = lldb.eStateInvalid
+
while state != lldb.eStateStopped:
state, restart = self.wait_for_next_event(None, False)
@@ -412,8 +443,6 @@ def test_shadow_listener(self):
self.cur_thread.GetStopReasonDataAtIndex(0),
"Hit the right breakpoint",
)
- # Disable the first breakpoint so it doesn't get in the way...
- bkpt1.SetEnabled(False)
self.cur_thread.StepOver()
# We'll run the test for "shadow listener blocked by primary listener
@@ -450,4 +479,9 @@ def test_shadow_listener(self):
)
if state == lldb.eStateStopped and not restarted:
self.process.Continue()
+
state, restarted = self.wait_for_next_event(None, False)
+
+ # Now make sure we agree with the stop hook counter:
+ self.assertEqual(self.stop_counter, stop_hook.StopHook.counter[self.instance])
+ self.assertEqual(stop_hook.StopHook.non_stops[self.instance], 0, "No non stops")
More information about the lldb-commits
mailing list