[Lldb-commits] [lldb] fix: Target Process may crash on detaching process (PR #115712)

[via lldb-commits]

https://github.com/anatawa12 created https://github.com/llvm/llvm-project/pull/115712

I hope this fixes #67825
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.

I have no idea how should I test this so I did not add tests. Could you let me know how should I test for this chages?


>From cabe26fc14cb4a924b2aebe6f3dd080408fed6ce Mon Sep 17 00:00:00 2001
From: anatawa12 <anatawa12 at icloud.com>
Date: Mon, 11 Nov 2024 20:22:33 +0900
Subject: [PATCH] fix: Target Process may crash on detaching process

---
 .../Process/Windows/Common/DebuggerThread.cpp | 45 ++++++++++++-------
 .../Process/Windows/Common/DebuggerThread.h   |  2 +-
 2 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/lldb/source/Plugins/Process/Windows/Common/DebuggerThread.cpp b/lldb/source/Plugins/Process/Windows/Common/DebuggerThread.cpp
index ca8e9c078e1f99..ac63997abe823d 100644
--- a/lldb/source/Plugins/Process/Windows/Common/DebuggerThread.cpp
+++ b/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,25 @@ 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());
+          ::DebugActiveProcessStop(m_pid_to_detach);
+          m_detached = true;
+        }
+      }
+
       if (m_detached) {
         should_debug = false;
       }
@@ -310,25 +330,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);
diff --git a/lldb/source/Plugins/Process/Windows/Common/DebuggerThread.h b/lldb/source/Plugins/Process/Windows/Common/DebuggerThread.h
index e3439ff34584be..5960237b778673 100644
--- a/lldb/source/Plugins/Process/Windows/Common/DebuggerThread.h
+++ b/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,