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.

Author: jimingham

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

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;

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)

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 -

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