[lldb] Fix ProcessEventData::DoOnRemoval to support multiple listeners

[ilya-nozhkin]

Summary

Process does something with its state and threads when an event is
removed from some public listener's event queue. The comment at the
beginning of DoOnRemoval and the condition checking m_update_state
tell that all these actions on the process state and threads must
be executed only once and after the private listener has already
handled an event. However, there is no any modification of
m_update_state except for invocation of SetUpdateStateOnRemoval
in HandlePrivateEvent. It means that m_update_state becomes 1 after an
event is handled by the private listener and is not changed anymore.
So, if more than one listener listen for process events
(for example, if someone add a listener following the example [1])
then the code in DoOnRemoval may be executed more than once. Moreover,
since events are handled asynchronously, DoOnRemoval may be executed by
two threads simultaneously that leads to data race.

This commit replaces m_update_state-based mechanism with a check of the
event broadcaster at the beginning of DoOnRemoval and a boolean flag
telling whether DoOnRemoval's logic has been already executed.
Also, there added a mutex which is locked during the whole execution of
DoOnRemoval. It makes DoOnRemoval thread-safe and ensures that the
public state of a process will be correct for all threads invoking
DoOnRemoval.

[1] https://lldb.llvm.org/python_reference/lldb.SBEvent-class.html

[jingham]

This change is fine for what it does, but I don't think the model that it allows is really supportable. If you have multiple listeners on the process events, and Listener A wants to do a "step" when notified that the process has stopped, but Listener B wants to do a "Continue", then there's no way to coordinate these and we're just allowing race conditions in controlling the process. I think you really need to have one agent that controls the process, and then other threads that are passive listeners.

My intention for this was to have the primary Listener (the one that was registered with the process when it was created) be the "controlling listener". We would ensure that that listener gets notified first, and add some kind of handshake where the controlling listener relinquishes control, after which the other listeners will be informed.

Anyway, my feeling is we should think a bit more about policy w.r.t. fetching process events from multiple Process listeners before we allow them.

There should also be some test cases for handling a process with multiple listeners, since that's not something we've supported before.

[ilya-nozhkin]

Yeah, I understand the problem of two listeners trying to change process state simultaneously and I agree that informing other listeners once the primary one finished its job would be a great solution. But the problem is that primary listener just provides an event to some user code (via GetEvent) and there is no way to find out when this code has finished without requiring it to invoke a handshake explicitly.

One of the possible solutions is to send a specifically marked event to a primary listener first. Then, make this event to invoke a handshake in its destructor (which would mean that primary listener doesn't need the event anymore). And handshake can be implemented as just a rebroadcasting a new instance of the event to other listeners (or via unlocking some additional mutex / notifying some conditional variable). However, destructor may be invoked not immediately after event processing is finished. For example, if a primary listener's thread defines an EventSP before the listening cycle and doesn't reset it until GetEvent returns a new one (which may not happen until the next stop, for example).

Anyway, the concept of primary and secondary listeners requires to forbid secondary listeners to do "step" or "continue" at all. So, why can't we do the same without any additional code? I mean, I think that the most common case of multiple listeners is a Python extension adding a listener to read/write some memory/registers on each stop taking the RunLock. So, it does not interfere with a primary listener except for simultaneous DoOnRemoval invocation (which is fixed by this patch). Moreover, as I understand, there is no any way to implement such extension without adding a new listener (because if one would try to use Debugger's default listener then all other users such as lldb-vscode, for example, couldn't handle process events anymore).

[jingham]

This still seems to me a pretty fragile way to program lldb. For instance, if the controlling thread gets the stop event first, and decides it wants to continue, it will set the process running again and by the time the memory reading thread gets the event, the process will have already proceeded and the memory read will fail. But it it happens the other way around, it will succeed. That will make for subtle bugs in your implementation.

Having a bunch of event listeners hanging out doing ancillary stop tasks also makes it hard to do cancellation. For instance, if the user presses "step" again while you are still doing the work to present the current stop, you want to discard all the work you were doing for the first stop and just step again. People like to drive from an interesting place to another interesting place by whaling on the "next" button and want that to be very responsive and don't care if all the data isn't updated in getting there. As long as the UI knows what work it cancelled and doesn't mark data as updated if it wasn't, this does seem to better model how people told us they want GUI debuggers to behave.

You could also implement this sort of thing with stop hooks that get run before any of the other actions are taken by then controlling thread (or by implementing this yourself.) There isn't currently a way to write stop hooks in Python but it would be simple to add that. The stop hooks don't solve the cancellation problem, however.

Anyway, "I wouldn't do it that way" isn't a good reason not to make a thing possible. If you feel like you can have success with this added ability, I can't see how it would do harm.

You still need to add some tests for this new behavior. We have been telling people not to do it this way pretty much forever, and so there are no tests for multiple process listeners. It would be good to write a not entirely trivial test that tries to do a little simultaneous work in your different Listeners, and deal with DoOnRemoval restarting the target, as well as one of the Listeners doing so, to ensure we don't fall over for other reasons when we actually try to use the feature.

[labath]

This is going to derail this patch somewhat (sorry), but since this is an important long-standing open issue, I think we should discuss it in more detail.

The part that's bothering me about this for quite some time is... Why do we even have this DoOnRemoval stuff in the first place?

I mean instead of doing this work in DoOnRemoval, why don't we do this work /before/ enqueueing the relevant event. Then the event would be of purely informative character and it would not matter if its received by one, two, ten, or zero listeners. Of course, if a user wanted to program lldb in a correct and race-free manner, it would /have to/ listen to these events (as otherwise it would not know when is it safe to run some actions), but that would be something that's entirely up to him.

Back when I was debugging some races in tests with asynchronous listeners, I remember it was pretty hard to reason about things and prove they are correct, since the dequeueing of the event (and the resulting flurry of actions within lldb) could come at any moment. If those actions were done internally to lldb, they would be more sequenced more deterministically and be easier to reason about than if they are executed whenever the user chooses to dequeue an event. (I remember things got particularly messy when a test finished before dequeuing an event, at which point the processing of the event was executing concurrently with the process.Kill() statement we execute at the end of each test.)

In D86652#2242770, @ilya-nozhkin wrote:

One of the possible solutions is to send a specifically marked event to a primary listener first. Then, make this event to invoke a handshake in its destructor (which would mean that primary listener doesn't need the event anymore). And handshake can be implemented as just a rebroadcasting a new instance of the event to other listeners (or via unlocking some additional mutex / notifying some conditional variable). However, destructor may be invoked not immediately after event processing is finished. For example, if a primary listener's thread defines an EventSP before the listening cycle and doesn't reset it until GetEvent returns a new one (which may not happen until the next stop, for example).

This primary listener technique could be implemented purely in your own code, could it not? You could have one listener, which listens to lldb events, and then rebroadcasts it (using arbitrary techniques, not necessarily SBListener) to any number of interested threads that want to act on it. That might make it easier to ensure there are no races in your code, and would avoid stepping on some of the internal lldb races that I am sure we have... Otherwise I (as Jim) expect that you will still need to have some sort of communication between the various listeners listening to process events, as they'd need to coordinate

Anyway, the concept of primary and secondary listeners requires to forbid secondary listeners to do "step" or "continue" at all. So, why can't we do the same without any additional code? I mean, I think that the most common case of multiple listeners is a Python extension adding a listener to read/write some memory/registers on each stop taking the RunLock.

Yes, you could take the run lock, which would guarantee that the other listeners don't get to resume the process. But how do you guarantee that you _can_ take the run lock -- some other thread (the primary listener) could beat you to it resume the process, so you'd miss the chance to read/write those registers. Seems very fragile. A stop hook does look like a better way to achieve these things.

[jingham]

In D86652#2244043, @labath wrote:

This is going to derail this patch somewhat (sorry), but since this is an important long-standing open issue, I think we should discuss it in more detail.

The part that's bothering me about this for quite some time is... Why do we even have this DoOnRemoval stuff in the first place?

I mean instead of doing this work in DoOnRemoval, why don't we do this work /before/ enqueueing the relevant event. Then the event would be of purely informative character and it would not matter if its received by one, two, ten, or zero listeners. Of course, if a user wanted to program lldb in a correct and race-free manner, it would /have to/ listen to these events (as otherwise it would not know when is it safe to run some actions), but that would be something that's entirely up to him.

Back when I was debugging some races in tests with asynchronous listeners, I remember it was pretty hard to reason about things and prove they are correct, since the dequeueing of the event (and the resulting flurry of actions within lldb) could come at any moment. If those actions were done internally to lldb, they would be more sequenced more deterministically and be easier to reason about than if they are executed whenever the user chooses to dequeue an event. (I remember things got particularly messy when a test finished before dequeuing an event, at which point the processing of the event was executing concurrently with the process.Kill() statement we execute at the end of each test.)

It would certainly be worth rethinking this, these were decisions made very early in the development of lldb...

I did it the way I did for several reasons:

First of all was to get the work that was done by the DoOnRemoval - which is basically anything lldb can do since it's where the breakpoint callbacks are triggered - off the private state thread. The private state thread needs to be alive and getting private events and running the thread plans to do the work required by breakpoint callbacks, and while I do have a system to spin up another private state threads if you need to use one while doing some piece of work on the private state thread, that seemed fragile to me and I didn't want to lean on it too heavily. It is important to make explicit the transition from lldb doing internal work with the incoming event stream to giving over control to the user. Internally, we have to let ourselves do things based off the private state, but external users are gated by the public state instead, Having it happen when the event gets handed to the user was a convenient place to mark this transition.

The other reason I did it this way was that I didn't want the process state and the event stream to get out of sync. I we do all the work on an event including setting the public state to stopped before posting the event, then you will easily get into the situation where the public state said "stopped" when you haven't yet fetched the stopped event off of the event queue. This inconsistencies seemed likely to result in confusing errors in use of the API's.

If we can come up with another way to achieve these goals but do the user-facing work of processing the event somewhere else, I don't have any objections.

In D86652#2242770, @ilya-nozhkin wrote:

One of the possible solutions is to send a specifically marked event to a primary listener first. Then, make this event to invoke a handshake in its destructor (which would mean that primary listener doesn't need the event anymore). And handshake can be implemented as just a rebroadcasting a new instance of the event to other listeners (or via unlocking some additional mutex / notifying some conditional variable). However, destructor may be invoked not immediately after event processing is finished. For example, if a primary listener's thread defines an EventSP before the listening cycle and doesn't reset it until GetEvent returns a new one (which may not happen until the next stop, for example).

This primary listener technique could be implemented purely in your own code, could it not? You could have one listener, which listens to lldb events, and then rebroadcasts it (using arbitrary techniques, not necessarily SBListener) to any number of interested threads that want to act on it. That might make it easier to ensure there are no races in your code, and would avoid stepping on some of the internal lldb races that I am sure we have... Otherwise I (as Jim) expect that you will still need to have some sort of communication between the various listeners listening to process events, as they'd need to coordinate

Anyway, the concept of primary and secondary listeners requires to forbid secondary listeners to do "step" or "continue" at all. So, why can't we do the same without any additional code? I mean, I think that the most common case of multiple listeners is a Python extension adding a listener to read/write some memory/registers on each stop taking the RunLock.

Yes, you could take the run lock, which would guarantee that the other listeners don't get to resume the process. But how do you guarantee that you _can_ take the run lock -- some other thread (the primary listener) could beat you to it resume the process, so you'd miss the chance to read/write those registers. Seems very fragile. A stop hook does look like a better way to achieve these things.

[ilya-nozhkin]

I've realized that I just didn't know about stop hooks... So, sorry and thanks, it solves almost all my problems except for notifying about "resume" events which are important for me too. But I can use synchronous Process::Notifications to receive them (I couldn't use Notifications to receive stop events because they are sent before the public state is changed by DoOnRemoval). But then I thought why don't we just move SynchronouslyNotifyStateChanged invocation from ShouldBroadcastEvent to DoOnRemoval? It would provide the most convenient way of receiving process events without intersections with the primary listener. I can make these changes in another patch instead of this one.

In D86652#2244043, @labath wrote:

This primary listener technique could be implemented purely in your own code, could it not?

I am trying to write an extension that can work with such things as lldb-vscode and lldb-mi without disturbing them. They use default Debugger's listener. So, I can't change the code of this listener on my side thus I can't force them to notify my listener.

lldb/include/lldb/Target/Process.h

 //===-- Process.h -----------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_TARGET_PROCESS_H
 #define LLDB_TARGET_PROCESS_H
 
 #include "lldb/Host/Config.h"
 
 #include <limits.h>
 
 #include <chrono>
 #include <list>
 #include <memory>
 #include <mutex>
 #include <string>
public:
     static void AddRestartedReason(Event *event_ptr, const char *reason);
 
     static void SetRestartedInEvent(Event *event_ptr, bool new_value);
 
     static bool GetInterruptedFromEvent(const Event *event_ptr);
 
     static void SetInterruptedInEvent(Event *event_ptr, bool new_value);
 
-    static bool SetUpdateStateOnRemoval(Event *event_ptr);
-
   private:
-    void SetUpdateStateOnRemoval() { m_update_state++; }
-
     void SetRestarted(bool new_value) { m_restarted = new_value; }
 
     void SetInterrupted(bool new_value) { m_interrupted = new_value; }
 
     void AddRestartedReason(const char *reason) {
       m_restarted_reasons.push_back(reason);
     }
 
     lldb::ProcessWP m_process_wp;
     lldb::StateType m_state;
     std::vector<std::string> m_restarted_reasons;
     bool m_restarted; // For "eStateStopped" events, this is true if the target
                       // was automatically restarted.
-    int m_update_state;
+    std::mutex m_do_on_removal_mutex;
+    bool m_do_on_removal_executed;
     bool m_interrupted;
 
     ProcessEventData(const ProcessEventData &) = delete;
     const ProcessEventData &operator=(const ProcessEventData &) = delete;
   };
 
   /// Construct with a shared pointer to a target, and the Process listener.
   /// Uses the Host UnixSignalsSP by default.

lldb/source/Target/Process.cpp

 //===-- Process.cpp -------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include <atomic>
 #include <memory>
 #include <mutex>
 
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/Threading.h"
 
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Breakpoint/StoppointCallbackContext.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Expression/DiagnosticManager.h"
 #include "lldb/Expression/DynamicCheckerFunctions.h"
if (should_broadcast) {
     if (log) {
       LLDB_LOGF(log,
                 "Process::%s (pid = %" PRIu64
                 ") broadcasting new state %s (old state %s) to %s",
                 __FUNCTION__, GetID(), StateAsCString(new_state),
                 StateAsCString(GetState()),
                 is_hijacked ? "hijacked" : "public");
     }
-    Process::ProcessEventData::SetUpdateStateOnRemoval(event_sp.get());
     if (StateIsRunningState(new_state)) {
       // Only push the input handler if we aren't fowarding events, as this
       // means the curses GUI is in use... Or don't push it if we are launching
       // since it will come up stopped.
       if (!GetTarget().GetDebugger().IsForwardingEvents() &&
           new_state != eStateLaunching && new_state != eStateAttaching) {
         PushProcessIOHandler();
         m_iohandler_sync.SetValue(m_iohandler_sync.GetValue() + 1,
if (!is_secondary_thread)
     m_public_run_lock.SetStopped();
   return {};
 }
 
 // Process Event Data
 
 Process::ProcessEventData::ProcessEventData()
     : EventData(), m_process_wp(), m_state(eStateInvalid), m_restarted(false),
-      m_update_state(0), m_interrupted(false) {}
+      m_do_on_removal_executed(false), m_interrupted(false) {}
 
 Process::ProcessEventData::ProcessEventData(const ProcessSP &process_sp,
                                             StateType state)
     : EventData(), m_process_wp(), m_state(state), m_restarted(false),
-      m_update_state(0), m_interrupted(false) {
+      m_do_on_removal_executed(false), m_interrupted(false) {
   if (process_sp)
     m_process_wp = process_sp;
 }
 
 Process::ProcessEventData::~ProcessEventData() = default;
 
 ConstString Process::ProcessEventData::GetFlavorString() {
   static ConstString g_flavor("Process::ProcessEventData");
if (stop_info_sp && stop_info_sp->IsValid()) {
         still_should_stop = this_thread_wants_to_stop;
     }
   }
 
   return still_should_stop;
 }
 
 void Process::ProcessEventData::DoOnRemoval(Event *event_ptr) {
-  ProcessSP process_sp(m_process_wp.lock());
+  // This function gets called each time an event gets pulled off of some
+  // listener's event queue. However, all the code below must be executed only
+  // once and after an event is already handled by the private state listener.
+  // So, here are two guards: the first of them checks that this event has been
+  // broadcasted by the public broadcaster and the second checks that this code
+  // hasn't been executed yet.
+
+  Broadcaster *broadcaster = event_ptr->GetBroadcaster();
+  if (!broadcaster ||
+      broadcaster->GetBroadcasterName() != Process::GetStaticBroadcasterClass())
+    return;
 
-  if (!process_sp)
+  // Since different listeners are usually monitored by different threads, two
+  // threads may enter this function simultaneously. This lock ensures that
+  // only one thread executes all the code below in this case. This lock must
+  // be taken during the whole DoOnRemoval execution to ensure that other
+  // threads (which don't execute the code below) won't resume while DoOnRemoval
+  // does all the stuff about public process state and thus until the public
+  // state becomes correct.
+  std::lock_guard<std::mutex> do_on_removal_lock(m_do_on_removal_mutex);
+  if (m_do_on_removal_executed)
     return;
 
-  // This function gets called twice for each event, once when the event gets
-  // pulled off of the private process event queue, and then any number of
-  // times, first when it gets pulled off of the public event queue, then other
-  // times when we're pretending that this is where we stopped at the end of
-  // expression evaluation.  m_update_state is used to distinguish these three
-  // cases; it is 0 when we're just pulling it off for private handling, and >
-  // 1 for expression evaluation, and we don't want to do the breakpoint
-  // command handling then.
-  if (m_update_state != 1)
+  m_do_on_removal_executed = true;
+
+  ProcessSP process_sp(m_process_wp.lock());
+
+  if (!process_sp)
     return;
 
   process_sp->SetPublicState(
       m_state, Process::ProcessEventData::GetRestartedFromEvent(event_ptr));
 
   if (m_state == eStateStopped && !m_restarted) {
     // Let process subclasses know we are about to do a public stop and do
     // anything they might need to in order to speed up register and memory

 void Process::ProcessEventData::SetInterruptedInEvent(Event *event_ptr,
                                                       bool new_value) {
   ProcessEventData *data =
       const_cast<ProcessEventData *>(GetEventDataFromEvent(event_ptr));
   if (data != nullptr)
     data->SetInterrupted(new_value);
 }
 
-bool Process::ProcessEventData::SetUpdateStateOnRemoval(Event *event_ptr) {
-  ProcessEventData *data =
-      const_cast<ProcessEventData *>(GetEventDataFromEvent(event_ptr));
-  if (data) {
-    data->SetUpdateStateOnRemoval();
-    return true;
-  }
-  return false;
-}
-
 lldb::TargetSP Process::CalculateTarget() { return m_target_wp.lock(); }
 
 void Process::CalculateExecutionContext(ExecutionContext &exe_ctx) {
   exe_ctx.SetTargetPtr(&GetTarget());
   exe_ctx.SetProcessPtr(this);
   exe_ctx.SetThreadPtr(nullptr);
   exe_ctx.SetFramePtr(nullptr);
 }

lldb/unittests/Process/ProcessEventDataTest.cpp

 //===-- ProcessEventDataTest.cpp ------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Plugins/Platform/MacOSX/PlatformMacOSX.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Target/Process.h"
 #include "lldb/Target/StopInfo.h"
 #include "lldb/Target/Thread.h"
 #include "lldb/Utility/ArchSpec.h"
 #include "lldb/Utility/Event.h"
TEST_F(ProcessEventDataTest, DoOnRemoval) {
   ASSERT_TRUE(process_sp);
 
   /*
    Should hit ShouldStop if state is eStateStopped
    */
   ProcessEventDataSP event_data_sp =
       std::make_shared<DummyProcessEventData>(process_sp, eStateStopped);
   EventSP event_sp = std::make_shared<Event>(0, event_data_sp);
-  event_data_sp->SetUpdateStateOnRemoval(event_sp.get());
+  process_sp->BroadcastEvent(event_sp);
   event_data_sp->DoOnRemoval(event_sp.get());
   bool result = static_cast<DummyProcessEventData *>(event_data_sp.get())
                     ->m_should_stop_hit_count == 1;
   ASSERT_TRUE(result);
 
   /*
    Should not hit ShouldStop if state is not eStateStopped
    */
   event_data_sp =
       std::make_shared<DummyProcessEventData>(process_sp, eStateStepping);
   event_sp = std::make_shared<Event>(0, event_data_sp);
-  event_data_sp->SetUpdateStateOnRemoval(event_sp.get());
+  process_sp->BroadcastEvent(event_sp);
   event_data_sp->DoOnRemoval(event_sp.get());
   result = static_cast<DummyProcessEventData *>(event_data_sp.get())
                ->m_should_stop_hit_count == 0;
   ASSERT_TRUE(result);
 }
 
 TEST_F(ProcessEventDataTest, ShouldStop) {
   ArchSpec arch("x86_64-apple-macosx-");