[lldb, process] Fix occasional hang when launching a process in LLDB

[stella.stamenova]

Summary

Occasionally, when launching a process in lldb (especially on windows, but not limited to), lldb will hang before the process is launched and it will never recover. This happens because the timing of the processing of the state changes can be slightly different. The state changes that are issued are:

1. SetPublicState(eStateLaunching)
2. SetPrivateState(eStateLaunching)
3. SetPublicState(eStateStopped)
4. SetPrivateState(eStateStopped)

What we expect to see is:
public state: launching -> launching -> stopped
private state: launching -> stopped

What we see is:
public state: launching -> stopped -> launching
private state: launching -> stopped

The second launching change to the public state is issued when WaitForProcessStopPrivate calls HandlePrivateEvent on the event which was created when the private state was set to launching. HandlePrivateEvent has logic to determine whether to broadcase the event and a launching event is *always* broadcast. At the same time, when the stopped event is processed by WaitForProcessStopPrivate next, the function exists and that event is never broadcast, so the public state remains as launching.

HandlePrivateEvent does two things: determine whether there's a next action as well as determine whether to broadcast the event that was processed. There's only ever a next action set if we are trying to attach to a process, but WaitForProcessStopPrivate is only ever called when we are launching a process or connecting remotely, so the first part of HandlePrivateEvent (handling the next action) is irrelevant for WaitForProcessStopPrivate. As far as broadcasting the event is concerned, since we are handling state changes that already occurred to the public state (and are now duplicated in the private state), I believe the broadcast step is unnecessary also (and in fact, it causes the hang).

This change removes the call to HandlePrivateEvent from inside WaitForProcessStopPrivate.

Incidentally, there was also a bug filed recently that is the same issue: https://bugs.llvm.org/show_bug.cgi?id=37496

[jingham]

Launch and attaching are weird because they happen before you have a private state thread (or if there is one it is explicitly paused), so the events don't flow as they normally would. So neither launch nor attach are relying on a private eStateLaunching event.

Looking at the code, only FreeBSD and Windows call SetPrivateState(eStateLaunching). When I do "run" on macOS I see:

(lldb) log enable lldb state
(lldb) run

Process::SetPublicState (state = launching, restarted = 0)
Process::SetPrivateState (stopped)
Process::SetPrivateState (stopped) stop_id = 1
Process::SetPublicState (state = stopped, restarted = 0)
Process::SetPublicState (stopped) -- unlocking run lock
Process::SetPrivateState (running)
Process::SetPublicState (state = running, restarted = 0)

The private eStateLaunching event shouldn't be needed on Windows either, and I think that it is just causing trouble. What happens if you just remove the SetPrivateState(eStateLaunching) call? If that fixes the issue, I'd rather do it that way, since that makes all the platforms regular.

Then we need to find some convenient place to document these expectations...

BTW I'm not disagreeing with your analysis of the current uses of WaitForProcessStopPrivate. However, if you change it the way that you are suggesting, it will no longer be a generally useable function, it could only be used in in the narrow cases you describe. So if we have to change it as you are suggesting, we should change its name to something more descriptive of its reduced functionality. But again, if removing the SetPrivateState(eStateLaunching) fixes the problem, I'd rather do it that way.

As an aside, the comment on line 2691 seems to have wandered out of place - presumably when this code got refactored? It doesn't make sense here...

[labath]

In D47609#1118414, @jingham wrote:

Launch and attaching are weird because they happen before you have a private state thread (or if there is one it is explicitly paused), so the events don't flow as they normally would. So neither launch nor attach are relying on a private eStateLaunching event.

Looking at the code, only FreeBSD and Windows call SetPrivateState(eStateLaunching).

I don't know if this matters, but windows and freebsd are also the only platforms using an in-process debugging plugin. (But I agree that unifying the event sequences across different platforms is the best approach.)

[stella.stamenova]

Removing the SetPrivateState(eStateLaunching) from Windows does resolve the issue of the hang. I considered that as well and it seemed that the more generic solution would be better since it would fix the problem for other platform such as FreeBSD as well. I am going to run the full suite of tests on Windows without the private state change and then update the review (I'll also fix the comment that has wandered out of place) but I am not going to touch the FreeBSD code path.

[stella.stamenova]

Rather than going with a generic solution, remove the change to private state to launching. This change also updates a comment that had ended up in the wrong place.

[zturner]

I trust that this doesn't regress the test suite on Windows. You guys are more active in the Windows stuff for the time being, so if this works for you and doesn't break anything, I'm good with it.

[jingham]

Can we add a comment to lldb-enumerations.h after eStateLaunching and eStateAttaching are defined saying that these state changes are all sent while the private state thread is either not yet started or paused, so they should only be signaled as Public state changes, and not Private state changes? I think that's the clearest place we can document this behavior.

Other than that, this change is fine.

[jingham]

Thanks for digging into this, BTW, this sort of thing isn't easy to track down!

[stella.stamenova]

In D47609#1119235, @zturner wrote:

I trust that this doesn't regress the test suite on Windows. You guys are more active in the Windows stuff for the time being, so if this works for you and doesn't break anything, I'm good with it.

Without this change the test suite on Windows will not always finish as some tests will hit the bug and then hang...

include/lldb/lldb-enumerations.h

 //----------------------------------------------------------------------
 enum StateType {
   eStateInvalid = 0,
   eStateUnloaded,  ///< Process is object is valid, but not currently loaded
   eStateConnected, ///< Process is connected to remote debug services, but not
                    ///launched or attached to anything yet
   eStateAttaching, ///< Process is currently trying to attach
   eStateLaunching, ///< Process is in the process of launching
+  // The state changes eStateAttaching and eStateLaunching are both sent while the
+  // private state thread is either not yet started or paused. For that reason, they
+  // should only be signaled as public state changes, and not private state changes.
   eStateStopped,   ///< Process or thread is stopped and can be examined.
   eStateRunning,   ///< Process or thread is running and can't be examined.
   eStateStepping,  ///< Process or thread is in the process of stepping and can
                    ///not be examined.
   eStateCrashed,   ///< Process or thread has crashed and can be examined.
   eStateDetached,  ///< Process has been detached and can't be examined.
   eStateExited,    ///< Process has exited and can't be examined.
   eStateSuspended  ///< Process or thread is in a suspended state as far

source/Plugins/Process/Windows/Common/ProcessWindows.cpp

if (!launch_info.GetFlags().Test(eLaunchFlagDebug)) {
 
     LLDB_LOG(log, "error: {0}", message);
     return result;
   }
 
   bool stop_at_entry = launch_info.GetFlags().Test(eLaunchFlagStopAtEntry);
   m_session_data.reset(new ProcessWindowsData(stop_at_entry));
 
-  SetPrivateState(eStateLaunching);
   DebugDelegateSP delegate(new LocalDebugDelegate(shared_from_this()));
   m_session_data->m_debugger.reset(new DebuggerThread(delegate));
   DebuggerThreadSP debugger = m_session_data->m_debugger;
 
   // Kick off the DebugLaunch asynchronously and wait for it to complete.
   result = debugger->DebugLaunch(launch_info);
   if (result.Fail()) {
     LLDB_LOG(log, "failed launching '{0}'. {1}",

source/Target/Process.cpp

Status Process::DisableWatchpoint(Watchpoint *watchpoint, bool notify) {
   error.SetErrorString("watchpoints are not supported");
   return error;
 }
 
 StateType
 Process::WaitForProcessStopPrivate(EventSP &event_sp,
                                    const Timeout<std::micro> &timeout) {
   StateType state;
-  // Now wait for the process to launch and return control to us, and then call
-  // DidLaunch:
+
   while (true) {
     event_sp.reset();
     state = GetStateChangedEventsPrivate(event_sp, timeout);
 
     if (StateIsStoppedState(state, false))
       break;
 
     // If state is invalid, then we timed out
if (exe_module->GetFileSpec().Exists()) {
             SetID(LLDB_INVALID_PROCESS_ID);
             const char *error_string = error.AsCString();
             if (error_string == nullptr)
               error_string = "launch failed";
             SetExitStatus(-1, error_string);
           }
         } else {
           EventSP event_sp;
+
+          // Now wait for the process to launch and return control to us, and then call
+          // DidLaunch:
           StateType state = WaitForProcessStopPrivate(event_sp, seconds(10));
 
           if (state == eStateInvalid || !event_sp) {
             // We were able to launch the process, but we failed to catch the
             // initial stop.
             error.SetErrorString("failed to catch stop after launch");
             SetExitStatus(0, "failed to catch stop after launch");
             Destroy(false);