[Lldb-commits] [PATCH] D157556: Replace the singleton "ShadowListener" with a primary and N secondary listeners

[Jim Ingham via Phabricator via lldb-commits]

jingham marked 4 inline comments as done.
jingham added a comment.

I implemented Alex's suggestion for a GetListeners filter, but doing that triggered some bugs due to the fact that we weren't really pruning the m_listeners correctly as listeners come and go.  I fixed some bugs there along with implementing this suggestion.



================
Comment at: lldb/source/Utility/Broadcaster.cpp:154-157
+  // The primary listener listens for all event bits:
+  if (m_primary_listener_sp)
+    return true;
+
----------------
clayborg wrote:
> Does the primary listener need to listen to all event bits?
I don't see a use case for saying "I want to be the one that gets first crack at this broadcaster" but only for some bits.  Plus, if we do it that way, we'd really have to have a map of "primary listener -> event mask" and add a way to make yourself the primary listener for only some bits.

We have this ability for HijackListeners, but we never use it, we always just hijack all bits as well...

There's no reason you can't do it that way, but until we see a need for multiple primary listeners, I don't think the extra complexity is warranted.


================
Comment at: lldb/source/Utility/Broadcaster.cpp:253
+          continue;
+        if (pair.first != hijacking_listener_sp 
+            && pair.first != m_primary_listener_sp)
----------------
bulbazord wrote:
> nit: Checking to see if `pair.first` is `hijacking_listener_sp` is redundant. If you've gotten to this point, `hijacking_listener_sp` must be pointing to `nullptr`.
Interestingly enough, that was effectively a check on empty listeners in m_listeners.  There shouldn't have been any, but we weren't being very careful about pruning the list...


================
Comment at: lldb/source/Utility/Broadcaster.cpp:254
+        if (pair.first != hijacking_listener_sp 
+            && pair.first != m_primary_listener_sp)
+          event_sp->AddPendingListener(pair.first);
----------------
bulbazord wrote:
> Why might `pair.first` be `m_primary_listener_sp`? I would assume that if a primary listener is set, it wouldn't be among the other listeners in a broadcaster.... But I suppose nothing stops you from doing that.
That's really an oversight in GetListeners.  That correctly doesn't return the Hijack listener, since that is a hidden agent, but the primary listener only differs from the secondary listeners in how dispatch gets done.  I fixed that in this patch.


================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/TestInteractiveScriptedProcess.py:38-44
+            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)
----------------
bulbazord wrote:
> This is to make sure that the primary listener is getting the run/stop events before the `mux_process_listener` right?
correct.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157556/new/

https://reviews.llvm.org/D157556