[Windows] Fix race condition between state changes

[martin]

Summary

If the process is resumed before the state is changed to "running" there is a possibility (when single stepping) that the debugger stops and changes the state to "stopped" before it is first changed to "running". This causes the process to ignore the stop event (since the state did not change) which in turn leads the DebuggerThread to wait indefinitely for the exception predicate in HandleExceptionEvent.

[amccarth]

zturner is not a regular here anymore. I think he pops in from time-to-time, but I wouldn't depend on just him for a review.

Are you still wanting to land this? If you rebase and add me as a reviewer, I'd be happy to take a look.

[martin]

Rebased against master

[martin]

In D62183#1556645, @amccarth wrote:

zturner is not a regular here anymore. I think he pops in from time-to-time, but I wouldn't depend on just him for a review.

Are you still wanting to land this? If you rebase and add me as a reviewer, I'd be happy to take a look.

I unfortunately missed your reply since it was somehow marked as spam. But yes I would like to see this land in some shape or form. I was a bit unsure on how much to change, I thought about also moving the call to LLDB_LOG for instance, but I opted for the least disruptive change I could manage.

[amccarth]

This change looks fine to me, but I wish there was a reliable way to test it. Is it a true race condition (e.g., because the unpredictability of thread scheduling), or is there a way to write a test that would always fail without this fix?

[martin]

In D62183#1589937, @amccarth wrote:

This change looks fine to me, but I wish there was a reliable way to test it. Is it a true race condition (e.g., because the unpredictability of thread scheduling), or is there a way to write a test that would always fail without this fix?

My understanding is that this is a race between the call to SetPrivateState(eStateRunning) in DoResume and the call to SetPrivateState(eStateStopped) in OnDebugException (on line 694). The former is supposed to take place before the latter and it often is, but when the latter is run first it leads the debugger to wait indefinitely. Because the second state change is ignored because the process is already in the stopped state.

I found this issue when I was using Visual Studio Code with the lldb-vscode debug adapter. Sometimes when I single stepped I would hit this issue. I could reliably trigger it by holding down F10 (step) in Visual Studio Code, it would only take a couple of seconds of rapid single stepping (roughly between 10 steps and 100 steps) before I hit it. However, I cannot trigger it with any code. For example, a simple while loop with an integer increment does not trigger it. But if I replace the integer increment with a function call I can trigger it within a couple of seconds. However, I am not familiar enough with LLDB to know if it would be possible to write a test that would reliably trigger it. My hunch is that it would be difficult.

[martin]

What is the next step to get this commited? Can you commit it?

[amccarth]

Yes, I can submit it for you, probably in the next hour or two. Thanks for the patch!

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

Status ProcessWindows::DoResume() {
   Status error;
 
   StateType private_state = GetPrivateState();
   if (private_state == eStateStopped || private_state == eStateCrashed) {
     LLDB_LOG(log, "process {0} is in state {1}.  Resuming...",
              m_session_data->m_debugger->GetProcess().GetProcessId(),
              GetPrivateState());
 
-    ExceptionRecordSP active_exception =
-        m_session_data->m_debugger->GetActiveException().lock();
-    if (active_exception) {
-      // Resume the process and continue processing debug events.  Mask the
-      // exception so that from the process's view, there is no indication that
-      // anything happened.
-      m_session_data->m_debugger->ContinueAsyncException(
-          ExceptionResult::MaskException);
-    }
-
     LLDB_LOG(log, "resuming {0} threads.", m_thread_list.GetSize());
 
     bool failed = false;
     for (uint32_t i = 0; i < m_thread_list.GetSize(); ++i) {
       auto thread = std::static_pointer_cast<TargetThreadWindows>(
           m_thread_list.GetThreadAtIndex(i));
       Status result = thread->DoResume();
       if (result.Fail()) {
         failed = true;
         LLDB_LOG(
             log,
             "Trying to resume thread at index {0}, but failed with error {1}.",
             i, result);
       }
     }
 
     if (failed) {
       error.SetErrorString("ProcessWindows::DoResume failed");
-      return error;
     } else {
       SetPrivateState(eStateRunning);
     }
+
+    ExceptionRecordSP active_exception =
+        m_session_data->m_debugger->GetActiveException().lock();
+    if (active_exception) {
+      // Resume the process and continue processing debug events.  Mask the
+      // exception so that from the process's view, there is no indication that
+      // anything happened.
+      m_session_data->m_debugger->ContinueAsyncException(
+          ExceptionResult::MaskException);
+    }
   } else {
     LLDB_LOG(log, "error: process {0} is in state {1}.  Returning...",
              m_session_data->m_debugger->GetProcess().GetProcessId(),
              GetPrivateState());
   }
   return error;
 }