diff mbox series

[20/34] Windows gdbserver: Eliminate soft-interrupt mechanism

Message ID 20240507234233.371123-21-pedro@palves.net
State New
Headers
Series Windows non-stop mode |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed

Commit Message

Pedro Alves May 7, 2024, 11:42 p.m. UTC 
  I noticed that faked_breakpoint is write only.  And then I hacked
win32_process_target::request_interrupt to force it to stop threads
using the soft_interrupt_requested mechanism (which suspends threads,
and then fakes a breakpoint event in the main thread), and saw that it
no longer works -- gdbserver crashes accessing a NULL current_thread,
because fake_breakpoint_event does not switch to a thread.

This code was originally added for Windows CE, as neither
GenerateConsoleCtrlEvent nor DebugBreakProcess worked there.

We nowadays require Windows XP or later, and XP has DebugBreakProcess.

The soft_interrupt_requested mechanism has other problems, like for
example faking the event in the main thread, even if that thread was
previously stopped, due to scheduler-locking.

A following patch will add a similar mechanism stopping all threads
with SuspendThread to native GDB, for non-stop mode, which doesn't
have these problems.  It's different enough from this old code that I
think we should just rip the old code out, and reimplement it from
scratch (based on gdb's version) when we need it.

Change-Id: I89e98233a9c40c6dcba7c8e1dacee08603843fb1
---
 gdbserver/win32-low.cc | 32 +-------------------------------
 gdbserver/win32-low.h  |  8 --------
 2 files changed, 1 insertion(+), 39 deletions(-)

Comments

Tom Tromey May 8, 2024, 3:26 p.m. UTC | #1 
>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> A following patch will add a similar mechanism stopping all threads
Pedro> with SuspendThread to native GDB, for non-stop mode, which doesn't
Pedro> have these problems.  It's different enough from this old code that I
Pedro> think we should just rip the old code out, and reimplement it from
Pedro> scratch (based on gdb's version) when we need it.

Thank you.
Approved-By: Tom Tromey <tom@tromey.com>

Tom
diff mbox series

Patch

diff --git a/gdbserver/win32-low.cc b/gdbserver/win32-low.cc
index 7d37068ab91..99a6150599e 100644
--- a/gdbserver/win32-low.cc
+++ b/gdbserver/win32-low.cc
@@ -307,8 +307,6 @@  do_initial_child_stuff (HANDLE proch, DWORD pid, int attached)
   windows_process.process_id = pid;
   windows_process.main_thread_id = 0;
 
-  windows_process.soft_interrupt_requested = 0;
-  windows_process.faked_breakpoint = 0;
   windows_process.open_process_used = true;
 
   memset (&windows_process.current_event, 0,
@@ -433,7 +431,6 @@  child_continue_for_kill (DWORD continue_status, int thread_id)
     {
       continue_one_thread (thread, thread_id);
     });
-  windows_process.faked_breakpoint = 0;
 
   return continue_last_debug_event (continue_status, debug_threads);
 }
@@ -941,23 +938,6 @@  suspend_one_thread (thread_info *thread)
   th->suspend ();
 }
 
-static void
-fake_breakpoint_event (void)
-{
-  OUTMSG2(("fake_breakpoint_event\n"));
-
-  windows_process.faked_breakpoint = 1;
-
-  memset (&windows_process.current_event, 0,
-	  sizeof (windows_process.current_event));
-  windows_process.current_event.dwThreadId = windows_process.main_thread_id;
-  windows_process.current_event.dwDebugEventCode = EXCEPTION_DEBUG_EVENT;
-  windows_process.current_event.u.Exception.ExceptionRecord.ExceptionCode
-    = EXCEPTION_BREAKPOINT;
-
-  for_each_thread (suspend_one_thread);
-}
-
 /* See nat/windows-nat.h.  */
 
 bool
@@ -1012,13 +992,6 @@  get_child_debug_event (DWORD *continue_status,
 
   DEBUG_EVENT *current_event = &windows_process.current_event;
 
-  if (windows_process.soft_interrupt_requested)
-    {
-      windows_process.soft_interrupt_requested = 0;
-      fake_breakpoint_event ();
-      goto gotevent;
-    }
-
   windows_process.attaching = 0;
   {
     for (thread_info *thread : all_threads)
@@ -1058,8 +1031,6 @@  get_child_debug_event (DWORD *continue_status,
       }
   }
 
- gotevent:
-
   switch (current_event->dwDebugEventCode)
     {
     case CREATE_THREAD_DEBUG_EVENT:
@@ -1313,8 +1284,7 @@  win32_process_target::request_interrupt ()
   if (DebugBreakProcess (windows_process.handle))
     return;
 
-  /* Last resort, suspend all threads manually.  */
-  windows_process.soft_interrupt_requested = 1;
+  OUTMSG2 (("Could not interrupt.\n"));
 }
 
 bool
diff --git a/gdbserver/win32-low.h b/gdbserver/win32-low.h
index 7ee4de4b37f..4d26b87d73d 100644
--- a/gdbserver/win32-low.h
+++ b/gdbserver/win32-low.h
@@ -187,14 +187,6 @@  struct gdbserver_windows_process : public windows_nat::windows_process_info
      debug event off the win32 API.  */
   struct target_waitstatus cached_status;
 
-  /* Non zero if an interrupt request is to be satisfied by suspending
-     all threads.  */
-  int soft_interrupt_requested = 0;
-
-  /* Non zero if the inferior is stopped in a simulated breakpoint done
-     by suspending all the threads.  */
-  int faked_breakpoint = 0;
-
   /* True if current_process_handle needs to be closed.  */
   bool open_process_used = false;