fix: Target Process may crash or freezes on detaching process on windows (#115712)

Fixes #67825 Fixes #89077

Fixes
[RIDER-99436](https://youtrack.jetbrains.com/issue/RIDER-99436/Unity-Editor-will-be-crashed-when-detaching-LLDB-debugger-in-Rider),
which is upstream issue of #67825.

This PR changes the timing of calling `DebugActiveProcessStop` to after
calling `ContinueDebugEvent` for last debugger exception.

I confirmed the crashing behavior is because we call
`DebugActiveProcessStop` before `ContinueDebugEvent` for last debugger
exception with https://github.com/anatawa12/debug-api-test.

Author: anatawa12

lldb/source/Plugins/Process/Windows/Common/DebuggerThread.cpp

@@ -239,12 +239,13 @@ void DebuggerThread::DebugLoop() {
     BOOL wait_result = WaitForDebugEvent(&dbe, INFINITE);
     if (wait_result) {
       DWORD continue_status = DBG_CONTINUE;
+      bool shutting_down = m_is_shutting_down;
       switch (dbe.dwDebugEventCode) {
       default:
         llvm_unreachable("Unhandle debug event code!");
       case EXCEPTION_DEBUG_EVENT: {
-        ExceptionResult status =
-            HandleExceptionEvent(dbe.u.Exception, dbe.dwThreadId);
+        ExceptionResult status = HandleExceptionEvent(
+            dbe.u.Exception, dbe.dwThreadId, shutting_down);
 
         if (status == ExceptionResult::MaskException)
           continue_status = DBG_CONTINUE;
@@ -292,6 +293,45 @@ void DebuggerThread::DebugLoop() {
 
       ::ContinueDebugEvent(dbe.dwProcessId, dbe.dwThreadId, continue_status);
 
+      // We have to DebugActiveProcessStop after ContinueDebugEvent, otherwise
+      // the target process will crash
+      if (shutting_down) {
+        // A breakpoint that occurs while `m_pid_to_detach` is non-zero is a
+        // magic exception that we use simply to wake up the DebuggerThread so
+        // that we can close out the debug loop.
+        if (m_pid_to_detach != 0 &&
+            (dbe.u.Exception.ExceptionRecord.ExceptionCode ==
+                 EXCEPTION_BREAKPOINT ||
+             dbe.u.Exception.ExceptionRecord.ExceptionCode ==
+                 STATUS_WX86_BREAKPOINT)) {
+          LLDB_LOG(log,
+                   "Breakpoint exception is cue to detach from process {0:x}",
+                   m_pid_to_detach.load());
+
+          // detaching with leaving breakpoint exception event on the queue may
+          // cause target process to crash so process events as possible since
+          // target threads are running at this time, there is possibility to
+          // have some breakpoint exception between last WaitForDebugEvent and
+          // DebugActiveProcessStop but ignore for now.
+          while (WaitForDebugEvent(&dbe, 0)) {
+            continue_status = DBG_CONTINUE;
+            if (dbe.dwDebugEventCode == EXCEPTION_DEBUG_EVENT &&
+                !(dbe.u.Exception.ExceptionRecord.ExceptionCode ==
+                      EXCEPTION_BREAKPOINT ||
+                  dbe.u.Exception.ExceptionRecord.ExceptionCode ==
+                      STATUS_WX86_BREAKPOINT ||
+                  dbe.u.Exception.ExceptionRecord.ExceptionCode ==
+                      EXCEPTION_SINGLE_STEP))
+              continue_status = DBG_EXCEPTION_NOT_HANDLED;
+            ::ContinueDebugEvent(dbe.dwProcessId, dbe.dwThreadId,
+                                 continue_status);
+          }
+
+          ::DebugActiveProcessStop(m_pid_to_detach);
+          m_detached = true;
+        }
+      }
+
       if (m_detached) {
         should_debug = false;
       }
@@ -310,25 +350,18 @@ void DebuggerThread::DebugLoop() {
 
 ExceptionResult
 DebuggerThread::HandleExceptionEvent(const EXCEPTION_DEBUG_INFO &info,
-                                     DWORD thread_id) {
+                                     DWORD thread_id, bool shutting_down) {
   Log *log = GetLog(WindowsLog::Event | WindowsLog::Exception);
-  if (m_is_shutting_down) {
-    // A breakpoint that occurs while `m_pid_to_detach` is non-zero is a magic
-    // exception that
-    // we use simply to wake up the DebuggerThread so that we can close out the
-    // debug loop.
-    if (m_pid_to_detach != 0 &&
+  if (shutting_down) {
+    bool is_breakpoint =
         (info.ExceptionRecord.ExceptionCode == EXCEPTION_BREAKPOINT ||
-         info.ExceptionRecord.ExceptionCode == STATUS_WX86_BREAKPOINT)) {
-      LLDB_LOG(log, "Breakpoint exception is cue to detach from process {0:x}",
-               m_pid_to_detach.load());
-      ::DebugActiveProcessStop(m_pid_to_detach);
-      m_detached = true;
-    }
+         info.ExceptionRecord.ExceptionCode == STATUS_WX86_BREAKPOINT);
 
     // Don't perform any blocking operations while we're shutting down.  That
     // will cause TerminateProcess -> WaitForSingleObject to time out.
-    return ExceptionResult::SendToApplication;
+    // We should not send breakpoint exceptions to the application.
+    return is_breakpoint ? ExceptionResult::MaskException
+                         : ExceptionResult::SendToApplication;
   }
 
   bool first_chance = (info.dwFirstChance != 0);

lldb/source/Plugins/Process/Windows/Common/DebuggerThread.h

@@ -46,7 +46,7 @@ class DebuggerThread : public std::enable_shared_from_this<DebuggerThread> {
   void FreeProcessHandles();
   void DebugLoop();
   ExceptionResult HandleExceptionEvent(const EXCEPTION_DEBUG_INFO &info,
-                                       DWORD thread_id);
+                                       DWORD thread_id, bool shutting_down);
   DWORD HandleCreateThreadEvent(const CREATE_THREAD_DEBUG_INFO &info,
                                 DWORD thread_id);
   DWORD HandleCreateProcessEvent(const CREATE_PROCESS_DEBUG_INFO &info,

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

@@ -169,6 +169,38 @@ Status ProcessWindows::DoDetach(bool keep_stopped) {
   Log *log = GetLog(WindowsLog::Process);
   StateType private_state = GetPrivateState();
   if (private_state != eStateExited && private_state != eStateDetached) {
+    if (!keep_stopped) {
+      // if the thread is suspended by lldb, we have to resume threads before
+      // detaching process. When we do after DetachProcess(), thread handles
+      // become invalid so we do before detach.
+      if (private_state == eStateStopped || private_state == eStateCrashed) {
+        LLDB_LOG(log, "process {0} is in state {1}.  Resuming for detach...",
+                 m_session_data->m_debugger->GetProcess().GetProcessId(),
+                 GetPrivateState());
+
+        LLDB_LOG(log, "resuming {0} threads for detach.",
+                 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 = Status::FromErrorString("Resuming Threads for Detach failed");
+        }
+      }
+    }
+
     error = DetachProcess();
     if (error.Success())
       SetPrivateState(eStateDetached);

lldb/test/API/commands/process/detach-resumes/TestDetachResumes.py

@@ -12,7 +12,6 @@
 class DetachResumesTestCase(TestBase):
     NO_DEBUG_INFO_TESTCASE = True
 
-    @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr89077")
     def test_detach_resumes(self):
         self.build()
         exe = self.getBuildArtifact()