Fix race condition when detaching/killing an inferior.

llvm-svn: 237945

3 files changed, 49 insertions, 32 deletions

diff --git a/lldb/source/Plugins/Process/Windows/DebuggerThread.cpp b/lldb/source/Plugins/Process/Windows/DebuggerThread.cpp
index d4659a5faf2..43afbec7513 100644
--- a/lldb/source/Plugins/Process/Windows/DebuggerThread.cpp
+++ b/lldb/source/Plugins/Process/Windows/DebuggerThread.cpp
@@ -62,11 +62,14 @@ struct DebugAttachContext
 DebuggerThread::DebuggerThread(DebugDelegateSP debug_delegate)
     : m_debug_delegate(debug_delegate)
     , m_image_file(nullptr)
+    , m_debugging_ended_event(nullptr)
 {
+    m_debugging_ended_event = ::CreateEvent(nullptr, TRUE, FALSE, nullptr);
 }
 
 DebuggerThread::~DebuggerThread()
 {
+    ::CloseHandle(m_debugging_ended_event);
 }
 
 Error
@@ -150,6 +153,7 @@ DebuggerThread::DebuggerThreadLaunchRoutine(const ProcessLaunchInfo &launch_info
     else
         m_debug_delegate->OnDebuggerError(error, 0);
 
+    SetEvent(m_debugging_ended_event);
     return 0;
 }
 
@@ -203,10 +207,9 @@ DebuggerThread::StopDebugging(bool terminate)
             "StopDebugging called TerminateProcess(0x%p, 0) (inferior=%I64u), success='%s'",
             handle, pid, (terminate_suceeded ? "true" : "false"));
 
-
-        // If we're stuck waiting for an exception to continue, continue it now.  But only
-        // AFTER setting the termination event, to make sure that we don't race and enter
-        // another wait for another debug event.
+        // If we're stuck waiting for an exception to continue (e.g. the user is at a breakpoint
+        // messing around in the debugger), continue it now.  But only AFTER calling TerminateProcess
+        // to make sure that the very next call to WaitForDebugEvent is an exit process event.
         if (m_active_exception.get())
         {
             WINLOG_IFANY(WINDOWS_LOG_PROCESS|WINDOWS_LOG_EXCEPTION,
@@ -215,23 +218,19 @@ DebuggerThread::StopDebugging(bool terminate)
             ContinueAsyncException(ExceptionResult::MaskException);
         }
 
-        // Don't return until the process has exited.
-        if (terminate_suceeded)
-        {
-            WINLOG_IFALL(WINDOWS_LOG_PROCESS,
-                "StopDebugging waiting for termination of process %u to complete.", pid);
-
-            DWORD wait_result = ::WaitForSingleObject(handle, 5000);
-            if (wait_result != WAIT_OBJECT_0)
-                terminate_suceeded = false;
+        WINLOG_IFALL(WINDOWS_LOG_PROCESS, "StopDebugging waiting for detach from process %u to complete.", pid);
 
-            WINLOG_IFALL(WINDOWS_LOG_PROCESS,
-                "StopDebugging WaitForSingleObject(0x%p, 5000) returned %u",
-                handle, wait_result);
-        }
-
-        if (!terminate_suceeded)
+        DWORD wait_result = WaitForSingleObject(m_debugging_ended_event, 5000);
+        if (wait_result != WAIT_OBJECT_0)
+        {
             error.SetError(GetLastError(), eErrorTypeWin32);
+            WINERR_IFALL(WINDOWS_LOG_PROCESS, "StopDebugging WaitForSingleObject(0x%p, 5000) returned %u",
+                         m_debugging_ended_event, wait_result);
+        }
+        else
+        {
+            WINLOG_IFALL(WINDOWS_LOG_PROCESS, "StopDebugging detach from process %u completed successfully.", pid);
+        }
     }
     else
     {
@@ -343,6 +342,8 @@ DebuggerThread::DebugLoop()
         }
     }
     FreeProcessHandles();
+
+    WINLOG_IFALL(WINDOWS_LOG_EVENT, "WaitForDebugEvent loop completed, exiting.");
 }
 
 ExceptionResult
diff --git a/lldb/source/Plugins/Process/Windows/DebuggerThread.h b/lldb/source/Plugins/Process/Windows/DebuggerThread.h
index 0bfb6486208..08997776d63 100644
--- a/lldb/source/Plugins/Process/Windows/DebuggerThread.h
+++ b/lldb/source/Plugins/Process/Windows/DebuggerThread.h
@@ -81,6 +81,9 @@ class DebuggerThread : public std::enable_shared_from_this<DebuggerThread>
                                                  // is finished processing and the debug loop can be
                                                  // continued.
 
+    HANDLE m_debugging_ended_event; // An event which gets signalled by the debugger thread when it
+                                    // exits the debugger loop and is detached from the inferior.
+
     static lldb::thread_result_t DebuggerThreadLaunchRoutine(void *data);
     lldb::thread_result_t DebuggerThreadLaunchRoutine(const ProcessLaunchInfo &launch_info);
     static lldb::thread_result_t DebuggerThreadAttachRoutine(void *data);
diff --git a/lldb/source/Plugins/Process/Windows/ProcessWindows.cpp b/lldb/source/Plugins/Process/Windows/ProcessWindows.cpp
index 716dabbf128..8364e7770de 100644
--- a/lldb/source/Plugins/Process/Windows/ProcessWindows.cpp
+++ b/lldb/source/Plugins/Process/Windows/ProcessWindows.cpp
@@ -424,30 +424,43 @@ ProcessWindows::DoDetach(bool keep_stopped)
 Error
 ProcessWindows::DoDestroy()
 {
-    llvm::sys::ScopedLock lock(m_mutex);
-
-    Error error;
-    StateType private_state = GetPrivateState();
-    if (!m_session_data)
+    DebuggerThreadSP debugger_thread;
+    StateType private_state;
     {
-        WINWARN_IFALL(WINDOWS_LOG_PROCESS, "DoDestroy called while state = %u, but there is no active session.",
-                      private_state);
-        return error;
+        // Acquire this lock inside an inner scope, only long enough to get the DebuggerThread.
+        // StopDebugging() will trigger a call back into ProcessWindows which will acquire the lock
+        // again, so we need to not deadlock.
+        llvm::sys::ScopedLock lock(m_mutex);
+
+        private_state = GetPrivateState();
+
+        if (!m_session_data)
+        {
+            WINWARN_IFALL(WINDOWS_LOG_PROCESS, "DoDestroy called while state = %u, but there is no active session.",
+                          private_state);
+            return Error();
+        }
+
+        debugger_thread = m_session_data->m_debugger;
     }
 
-    DebuggerThread &debugger = *m_session_data->m_debugger;
+    Error error;
     if (private_state != eStateExited && private_state != eStateDetached)
     {
-        WINLOG_IFALL(WINDOWS_LOG_PROCESS, "DoDestroy called for process 0x%I64u while state = %u.  Shutting down...",
-                     debugger.GetProcess().GetNativeProcess().GetSystemHandle(), private_state);
-        error = debugger.StopDebugging(true);
+        WINLOG_IFALL(WINDOWS_LOG_PROCESS, "DoDestroy called for process %I64u while state = %u.  Shutting down...",
+                     debugger_thread->GetProcess().GetNativeProcess().GetSystemHandle(), private_state);
+        error = debugger_thread->StopDebugging(true);
+
+        // By the time StopDebugging returns, there is no more debugger thread, so we can be assured that no other
+        // thread
+        // will race for the session data.  So it's safe to reset it without holding a lock.
         m_session_data.reset();
     }
     else
     {
         WINERR_IFALL(WINDOWS_LOG_PROCESS,
                      "DoDestroy called for process %I64u while state = %u, but cannot destroy in this state.",
-                     debugger.GetProcess().GetNativeProcess().GetSystemHandle(), private_state);
+                     debugger_thread->GetProcess().GetNativeProcess().GetSystemHandle(), private_state);
     }
 
     return error;