jingham created this revision.
jingham added reviewers: mib, bulbazord, clayborg, JDevlieghere.
Herald added a project: All.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
Allow a Broadcaster to have one "Primary" listener, and potentially many
secondary Listeners. The primary listener is guaranteed to run the DoOnRemoval
first, and only when that is complete will secondary Listeners receive the
event.
Prior to Ismail's addition of the notion of a "Secondary" listener, you could
only safely observe process events from the Listener you passed in to the
Process creation. That was because the act of pulling the process event off
the event queue triggered the change of the public state of the process (thus
keeping the event stream and the state in sync).
Ismail added a privileged "shadow listener" that could also be informed of the
event. That wasn't quite the right model, however, because the real singleton
is the primary listener.
That's what this patch implements.
This isn't quite the full solution for the Process Listener. The goal is that
the primary Process Listener not only gets to drive the event stream, but that
the primary listener gets to react to each event before any other Listener can
hear about it. That will allow the process execution logic to run before any
other agent gets a chance to change the process state out from under it. For
that, I'll need to add a receipt mechanism to the event delivery, and have the
forwarding to the pending listeners happen only after the receipt is
acknowledged. But that will be a follow on patch.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D157556
Files:
lldb/include/lldb/Target/Process.h
lldb/include/lldb/Utility/Broadcaster.h
lldb/include/lldb/Utility/Event.h
lldb/source/Target/Process.cpp
lldb/source/Utility/Broadcaster.cpp
lldb/source/Utility/Event.cpp
lldb/source/Utility/Listener.cpp
lldb/test/API/functionalities/interactive_scripted_process/TestInteractiveScriptedProcess.py
lldb/test/API/python_api/event/TestEvents.py
Index: lldb/test/API/python_api/event/TestEvents.py
===================================================================
--- lldb/test/API/python_api/event/TestEvents.py
+++ lldb/test/API/python_api/event/TestEvents.py
@@ -313,3 +313,121 @@
self.assertEqual(
self.state, "stopped", "Both expected state changed events received"
)
+
+ 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."""
+ # Waiting on the shadow listener shouldn't have events yet because
+ # we haven't fetched them for the primary listener yet:
+ event = lldb.SBEvent()
+
+ if test_shadow:
+ success = self.shadow_listener.WaitForEvent(1, event)
+ self.assertFalse(success, "Shadow listener doesn't pull events")
+
+ # 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)
+ restart = False
+ if state == lldb.eStateStopped:
+ restart = lldb.SBProcess.GetRestartedFromEvent(event)
+
+ if expected_state != None:
+ self.assertEqual(state, expected_state, "Primary thread got the correct event")
+
+ # And after pulling that one there should be an equivalent event for the shadow
+ # listener:
+ success = self.shadow_listener.WaitForEvent(5, event)
+ self.assertTrue(success, "Shadow listener got event too")
+ self.assertEqual(state, lldb.SBProcess.GetStateFromEvent(event), "It was the same event")
+ self.assertEqual(restart, lldb.SBProcess.GetRestartedFromEvent(event), "It was the same restarted")
+
+ return state, restart
+
+ def test_shadow_listener(self):
+ self.build()
+ exe = self.getBuildArtifact("a.out")
+
+ # Create a target by the debugger.
+ target = self.dbg.CreateTarget(exe)
+ self.assertTrue(target, VALID_TARGET)
+
+ # Now create a breakpoint on main.c by name 'c'.
+ bkpt1 = target.BreakpointCreateByName("c", "a.out")
+ self.trace("breakpoint:", bkpt1)
+ self.assertTrue(bkpt1.GetNumLocations() == 1, VALID_BREAKPOINT)
+
+ self.primary_listener = lldb.SBListener("my listener")
+ self.shadow_listener = lldb.SBListener("shadow listener")
+
+ self.cur_thread = None
+
+ error = lldb.SBError()
+ launch_info = target.GetLaunchInfo()
+ launch_info.SetListener(self.primary_listener)
+ launch_info.SetShadowListener(self.shadow_listener)
+
+ self.runCmd("settings set target.process.extra-startup-command QSetLogging:bitmask=LOG_PROCESS|LOG_EXCEPTIONS|LOG_RNB_PACKETS|LOG_STEP;")
+ self.dbg.SetAsync(True)
+
+ self.process = target.Launch(launch_info, error)
+ self.assertSuccess(error, "Process launched successfully")
+
+ # Keep fetching events from the primary to trigger the do on removal and
+ # then from the shadow listener, and make sure they match:
+
+ # 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)
+
+ # Okay, we're now at a good stop, so try a next:
+ self.cur_thread = self.process.threads[0]
+
+ # Make sure we're at our expected breakpoint:
+ self.assertTrue(self.cur_thread.IsValid(), "Got a zeroth thread")
+ self.assertEqual(self.cur_thread.stop_reason, lldb.eStopReasonBreakpoint)
+ self.assertEqual(self.cur_thread.GetStopReasonDataCount(), 2, "Only one breakpoint/loc here")
+ self.assertEqual(bkpt1.GetID(), 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
+ # for the first couple rounds, then we'll skip the 1 second pause...
+ self.wait_for_next_event(lldb.eStateRunning, True)
+ self.wait_for_next_event(lldb.eStateStopped, True)
+
+ # Next try an auto-continue breakpoint and make sure the shadow listener got
+ # the resumed info as well. Note that I'm not explicitly counting
+ # running events here. At the point when I wrote this lldb sometimes
+ # emits two running events in a row. Apparently the code to coalesce running
+ # events isn't working. But that's not what this test is testing, we're really
+ # testing that the primary & shadow listeners hear the same thing and in the
+ # right order.
+
+ main_spec = lldb.SBFileSpec("main.c")
+ bkpt2 = target.BreakpointCreateBySourceRegex("b.2. returns %d", main_spec)
+ self.assertTrue(bkpt2.GetNumLocations() > 0, "BP2 worked")
+ bkpt2.SetAutoContinue(True)
+
+ bkpt3 = target.BreakpointCreateBySourceRegex("a.3. returns %d", main_spec)
+ self.assertTrue(bkpt3.GetNumLocations() > 0, "BP3 worked")
+
+ state = lldb.eStateStopped
+ restarted = False
+
+ # Put in a counter to make sure we don't spin forever if there is some
+ # error in the logic.
+ counter = 0
+ while state != lldb.eStateExited:
+ counter += 1
+ self.assertLess(counter, 50, "Took more than 50 events to hit two breakpoints.")
+ if state == lldb.eStateStopped and not restarted:
+ self.process.Continue()
+ state, restarted = self.wait_for_next_event(None, False)
+
Index: lldb/test/API/functionalities/interactive_scripted_process/TestInteractiveScriptedProcess.py
===================================================================
--- lldb/test/API/functionalities/interactive_scripted_process/TestInteractiveScriptedProcess.py
+++ lldb/test/API/functionalities/interactive_scripted_process/TestInteractiveScriptedProcess.py
@@ -23,7 +23,7 @@
self.script_file = self.script_module + ".py"
@skipUnlessDarwin
- @skipIfDarwin
+ #@skipIfDarwin
def test_passthrough_launch(self):
"""Test a simple pass-through process launch"""
self.passthrough_launch()
@@ -34,6 +34,15 @@
self.assertSuccess(error, "Resuming multiplexer scripted process")
self.assertTrue(self.mux_process.IsValid(), "Got a valid process")
+ event = lldbutil.fetch_next_event(
+ self, self.dbg.GetListener(), self.mux_process.GetBroadcaster()
+ )
+ self.assertState(lldb.SBProcess.GetStateFromEvent(event), lldb.eStateRunning)
+ event = lldbutil.fetch_next_event(
+ self, self.dbg.GetListener(), self.mux_process.GetBroadcaster()
+ )
+ self.assertState(lldb.SBProcess.GetStateFromEvent(event), lldb.eStateStopped)
+
event = lldbutil.fetch_next_event(
self, self.mux_process_listener, self.mux_process.GetBroadcaster()
)
@@ -44,7 +53,7 @@
self.assertState(lldb.SBProcess.GetStateFromEvent(event), lldb.eStateStopped)
@skipUnlessDarwin
- @skipIfDarwin
+ #@skipIfDarwin
def test_multiplexed_launch(self):
"""Test a multiple interactive scripted process debugging"""
self.passthrough_launch()
@@ -178,6 +187,13 @@
)
execution_events[event_target_idx][state] = True
+ for _ in range((self.dbg.GetNumTargets() - 1) * 2):
+ fetch_process_event(self, execution_events)
+
+ for target_index, event_states in execution_events.items():
+ for state, is_set in event_states.items():
+ self.assertTrue(is_set, f"Target {target_index} has state {state} set")
+
event = lldbutil.fetch_next_event(
self, self.mux_process_listener, self.mux_process.GetBroadcaster()
)
@@ -188,13 +204,6 @@
)
self.assertState(lldb.SBProcess.GetStateFromEvent(event), lldb.eStateStopped)
- for _ in range((self.dbg.GetNumTargets() - 1) * 2):
- fetch_process_event(self, execution_events)
-
- for target_index, event_states in execution_events.items():
- for state, is_set in event_states.items():
- self.assertTrue(is_set, f"Target {target_index} has state {state} set")
-
def duplicate_target(self, driving_target):
exe = driving_target.executable.fullpath
triple = driving_target.triple
@@ -248,14 +257,14 @@
self.assertSuccess(error, "Launched multiplexer scripted process")
self.assertTrue(self.mux_process.IsValid(), "Got a valid process")
- # Check that the mux process started running
+ # Check that the real process started running
event = lldbutil.fetch_next_event(
- self, self.mux_process_listener, self.mux_process.GetBroadcaster()
+ self, self.dbg.GetListener(), self.mux_process.GetBroadcaster()
)
self.assertState(lldb.SBProcess.GetStateFromEvent(event), lldb.eStateRunning)
- # Check that the real process started running
+ # Check that the mux process started running
event = lldbutil.fetch_next_event(
- self, self.dbg.GetListener(), self.mux_process.GetBroadcaster()
+ self, self.mux_process_listener, self.mux_process.GetBroadcaster()
)
self.assertState(lldb.SBProcess.GetStateFromEvent(event), lldb.eStateRunning)
Index: lldb/source/Utility/Listener.cpp
===================================================================
--- lldb/source/Utility/Listener.cpp
+++ lldb/source/Utility/Listener.cpp
@@ -231,8 +231,7 @@
// to return it so it should be okay to get the next event off the queue
// here - and it might be useful to do that in the "DoOnRemoval".
lock.unlock();
- if (!m_is_shadow)
- event_sp->DoOnRemoval();
+ event_sp->DoOnRemoval();
}
return true;
}
Index: lldb/source/Utility/Event.cpp
===================================================================
--- lldb/source/Utility/Event.cpp
+++ lldb/source/Utility/Event.cpp
@@ -11,6 +11,7 @@
#include "lldb/Utility/Broadcaster.h"
#include "lldb/Utility/DataExtractor.h"
#include "lldb/Utility/Endian.h"
+#include "lldb/Utility/Listener.h"
#include "lldb/Utility/Stream.h"
#include "lldb/Utility/StreamString.h"
#include "lldb/lldb-enumerations.h"
@@ -80,8 +81,16 @@
}
void Event::DoOnRemoval() {
+ std::lock_guard<std::mutex> guard(m_listeners_mutex);
+
if (m_data_sp)
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();
}
#pragma mark -
Index: lldb/source/Utility/Broadcaster.cpp
===================================================================
--- lldb/source/Utility/Broadcaster.cpp
+++ lldb/source/Utility/Broadcaster.cpp
@@ -151,6 +151,10 @@
if (!m_hijacking_listeners.empty() && event_type & m_hijacking_masks.back())
return true;
+ // The primary listener listens for all event bits:
+ if (m_primary_listener_sp)
+ return true;
+
for (auto &pair : GetListeners()) {
if (pair.second & event_type)
return true;
@@ -222,14 +226,36 @@
event_description.GetData(), unique,
static_cast<void *>(hijacking_listener_sp.get()));
}
-
- if (hijacking_listener_sp) {
- if (unique && hijacking_listener_sp->PeekAtNextEventForBroadcasterWithType(
+ ListenerSP primary_listener_sp = hijacking_listener_sp;
+ bool is_hijack = false;
+
+ if (primary_listener_sp)
+ is_hijack = true;
+ else
+ primary_listener_sp = m_primary_listener_sp;
+
+ if (primary_listener_sp) {
+ if (unique && primary_listener_sp->PeekAtNextEventForBroadcasterWithType(
&m_broadcaster, event_type))
return;
- hijacking_listener_sp->AddEvent(event_sp);
- if (m_shadow_listener)
- m_shadow_listener->AddEvent(event_sp);
+ // Add the pending listeners but not if the event is hijacked, since that
+ // is given sole access to the event stream it is hijacking.
+ // Make sure to do this before adding the event to the primary or it might
+ // start handling the event before we're done adding all the pending
+ // listeners.
+ if (!is_hijack) {
+ for (auto &pair : GetListeners()) {
+ if (!(pair.second & event_type))
+ continue;
+ if (unique && pair.first->PeekAtNextEventForBroadcasterWithType(
+ &m_broadcaster, event_type))
+ continue;
+ if (pair.first != hijacking_listener_sp
+ && pair.first != m_primary_listener_sp)
+ event_sp->AddPendingListener(pair.first);
+ }
+ }
+ primary_listener_sp->AddEvent(event_sp);
} else {
for (auto &pair : GetListeners()) {
if (!(pair.second & event_type))
@@ -239,8 +265,6 @@
continue;
pair.first->AddEvent(event_sp);
- if (m_shadow_listener)
- m_shadow_listener->AddEvent(event_sp);
}
}
}
Index: lldb/source/Target/Process.cpp
===================================================================
--- lldb/source/Target/Process.cpp
+++ lldb/source/Target/Process.cpp
@@ -369,6 +369,12 @@
g_process_properties[idx].default_uint_value));
}
+const int Process::g_all_event_bits = eBroadcastBitStateChanged
+ | eBroadcastBitInterrupt
+ | eBroadcastBitSTDOUT | eBroadcastBitSTDERR
+ | eBroadcastBitProfileData
+ | eBroadcastBitStructuredData;
+
ProcessSP Process::FindPlugin(lldb::TargetSP target_sp,
llvm::StringRef plugin_name,
ListenerSP listener_sp,
@@ -475,10 +481,9 @@
m_private_state_control_broadcaster.SetEventName(
eBroadcastInternalStateControlResume, "control-resume");
- m_listener_sp->StartListeningForEvents(
- this, eBroadcastBitStateChanged | eBroadcastBitInterrupt |
- eBroadcastBitSTDOUT | eBroadcastBitSTDERR |
- eBroadcastBitProfileData | eBroadcastBitStructuredData);
+ // The listener passed into process creation is the primary listener:
+ // It always listens for all the event bits for Process:
+ SetPrimaryListener(m_listener_sp);
m_private_state_listener_sp->StartListeningForEvents(
&m_private_state_broadcaster,
@@ -4114,8 +4119,8 @@
if (!still_should_stop && does_anybody_have_an_opinion) {
// We've been asked to continue, so do that here.
SetRestarted(true);
- // Use the public resume method here, since this is just extending a
- // public resume.
+ // Use the private resume method here, since we aren't changing the run
+ // lock state.
process_sp->PrivateResume();
} else {
bool hijacked = process_sp->IsHijackedForEvent(eBroadcastBitStateChanged) &&
Index: lldb/include/lldb/Utility/Event.h
===================================================================
--- lldb/include/lldb/Utility/Event.h
+++ lldb/include/lldb/Utility/Event.h
@@ -176,7 +176,7 @@
};
// lldb::Event
-class Event {
+class Event : public std::enable_shared_from_this<Event> {
friend class Listener;
friend class EventData;
friend class Broadcaster::BroadcasterImpl;
@@ -225,6 +225,12 @@
}
void Clear() { m_data_sp.reset(); }
+
+ /// This is used by Broadcasters with Primary Listeners to store the other
+ /// Listeners till after the Event's DoOnRemoval has completed.
+ void AddPendingListener(lldb::ListenerSP pending_listener_sp) {
+ m_pending_listeners.push_back(pending_listener_sp);
+ };
private:
// This is only called by Listener when it pops an event off the queue for
@@ -244,6 +250,8 @@
m_broadcaster_wp; // The broadcaster that sent this event
uint32_t m_type; // The bit describing this event
lldb::EventDataSP m_data_sp; // User specific data for this event
+ std::vector<lldb::ListenerSP> m_pending_listeners;
+ std::mutex m_listeners_mutex;
Event(const Event &) = delete;
const Event &operator=(const Event &) = delete;
Index: lldb/include/lldb/Utility/Broadcaster.h
===================================================================
--- lldb/include/lldb/Utility/Broadcaster.h
+++ lldb/include/lldb/Utility/Broadcaster.h
@@ -312,8 +312,8 @@
lldb::BroadcasterManagerSP GetManager();
- virtual void SetShadowListener(lldb::ListenerSP listener_sp) {
- m_broadcaster_sp->m_shadow_listener = listener_sp;
+ virtual void SetPrimaryListener(lldb::ListenerSP listener_sp) {
+ m_broadcaster_sp->m_primary_listener_sp = listener_sp;
}
protected:
@@ -409,6 +409,27 @@
/// event bit.
event_names_map m_event_names;
+ /// A Broadcaster can have zero, one or many listeners. A Broadcaster with
+ /// zero listeners is a no-op, with one Listener is trivial.
+ /// In most cases of multiple Listeners,the Broadcaster treats all its
+ /// Listeners as equal, sending each event to all of the Listeners in no
+ /// guaranteed order.
+ /// However, some Broadcasters - in particular the Process broadcaster, can
+ /// designate one Listener to be the "Primary Listener". In the case of
+ /// the Process Broadcaster, the Listener passed to the Process constructor
+ /// will be the Primary Listener.
+ /// If the broadcaster has a Primary Listener, then the event gets
+ /// sent first to the Primary Listener, and then when the Primary Listener
+ /// pulls the event and the the event's DoOnRemoval finishes running,
+ /// the event is forwarded to all the other Listeners.
+ /// The other wrinkle is that a Broadcaster may be serving a Hijack
+ /// Listener. If the Hijack Listener is present, events are only sent to
+ /// the Hijack Listener. We use that, for instance, to absorb all the
+ /// events generated by running an expression so that they don't show up to
+ /// the driver or UI as starts and stops.
+ /// If a Broadcaster has both a Primary and a Hijack Listener, the top-most
+ /// Hijack Listener is treated as the current Primary Listener.
+
/// A list of Listener / event_mask pairs that are listening to this
/// broadcaster.
collection m_listeners;
@@ -416,6 +437,9 @@
/// A mutex that protects \a m_listeners.
std::recursive_mutex m_listeners_mutex;
+ /// See the discussion of Broadcasters and Listeners above.
+ lldb::ListenerSP m_primary_listener_sp;
+
/// A simple mechanism to intercept events from a broadcaster
std::vector<lldb::ListenerSP> m_hijacking_listeners;
@@ -423,10 +447,6 @@
/// for now this is just for private hijacking.
std::vector<uint32_t> m_hijacking_masks;
- /// A optional listener that all private events get also broadcasted to,
- /// on top the hijacked / default listeners.
- lldb::ListenerSP m_shadow_listener = nullptr;
-
private:
BroadcasterImpl(const BroadcasterImpl &) = delete;
const BroadcasterImpl &operator=(const BroadcasterImpl &) = delete;
Index: lldb/include/lldb/Target/Process.h
===================================================================
--- lldb/include/lldb/Target/Process.h
+++ lldb/include/lldb/Target/Process.h
@@ -352,6 +352,9 @@
eBroadcastBitProfileData = (1 << 4),
eBroadcastBitStructuredData = (1 << 5),
};
+ // This is all the event bits the public process broadcaster broadcasts.
+ // The process shadow listener signs up for all these bits...
+ static const int g_all_event_bits;
enum {
eBroadcastInternalStateControlStop = (1 << 0),
@@ -383,10 +386,6 @@
return GetStaticBroadcasterClass();
}
- void SetShadowListener(lldb::ListenerSP listener_sp) override {
- Broadcaster::SetShadowListener(listener_sp);
- }
-
/// A notification structure that can be used by clients to listen
/// for changes in a process's lifetime.
///
@@ -610,6 +609,15 @@
return error;
}
+ /// The "ShadowListener" for a process is just an ordinary Listener that
+ /// listens for all the Process event bits. It's convenient because you can
+ /// specify it in the LaunchInfo or AttachInfo, so it will get events from
+ /// the very start of the process.
+ void SetShadowListener(lldb::ListenerSP shadow_listener_sp) {
+ if (shadow_listener_sp)
+ AddListener(shadow_listener_sp, g_all_event_bits);
+ }
+
// FUTURE WORK: GetLoadImageUtilityFunction are the first use we've
// had of having other plugins cache data in the Process. This is handy for
// long-living plugins - like the Platform - which manage interactions whose
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
