[gdbserver/win32] (4/11) New interrupting method

[gdbserver/win32] (4/11) New interrupting method

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 720 bytes --]

Hi,

Here is the new method to stop the inferior, based on a mix of
Leo's and mine patches.  The idea is simple.  Loop through all
the inferior threads suspending them.

This version doesn't contain the gdbserver priority handling,
which means that there is a higher chance of the inferior
breaking the method, since the operation is not atomic.  For
example by tweaking its own thread's priorities or if the
inferior is calling ResumeThread in its own threads while
gdbserver is stopping them.

The next patch will minimize those chances, by elevating
gdbserver's priority while stopping the inferior with
this method.

Leo, did I miss anything?

Regtested on a local i686-pc-cygwin gdbserver.

Cheers,
Pedro Alves





[-- Attachment #2: soft_interrupt.diff --]
[-- Type: text/x-diff, Size: 5704 bytes --]

2007-11-12  Leo Zayas  <lerele@champenstudios@com>
	    Pedro Alves  <pedro_alves@portugalmail.pt>

	* win32-low.c (soft_interrupt_requested, faked_breakpoint): New
	global variables.
	(child_add_thread): Minor cleanup.
	(child_continue): Resume artificially suspended threads before
	calling ContinueDebugEvent.
	(suspend_one_thread): New.
	(fake_breakpoint_event): New.
	(get_child_debug_event): Change return type to int.  Check here if
	gdb sent an interrupt request.  If a soft interrupt was
	requested, fake a breakpoint event.  Return 0 if there is no event
	to handle, and 1 otherwise.
	(win32_wait): Don't check here if gdb sent an interrupt request.
	Ensure there is a valid event to handle.
	(win32_request_interrupt): Add soft interruption method as last resort.

---
 gdb/gdbserver/win32-low.c |  100 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 79 insertions(+), 21 deletions(-)

Index: src/gdb/gdbserver/win32-low.c
===================================================================
--- src.orig/gdb/gdbserver/win32-low.c	2007-11-11 23:15:40.000000000 +0000
+++ src/gdb/gdbserver/win32-low.c	2007-11-11 23:15:44.000000000 +0000
@@ -72,6 +72,14 @@ static enum target_signal last_sig = TAR
 /* The current debug event from WaitForDebugEvent.  */
 static DEBUG_EVENT current_event;
 
+/* Non zero if an interrupt request is to be satisfied by suspending
+   all threads.  */
+static int soft_interrupt_requested = 0;
+
+/* Non zero if the inferior is stopped in a simulated breakpoint done
+   by suspending all the threads.  */
+static int faked_breakpoint = 0;
+
 #define NUM_REGS (the_low_target.num_regs)
 
 typedef BOOL WINAPI (*winapi_DebugActiveProcessStop) (DWORD dwProcessId);
@@ -134,8 +142,7 @@ child_add_thread (DWORD tid, HANDLE h)
   if ((th = thread_rec (tid, FALSE)))
     return th;
 
-  th = (win32_thread_info *) malloc (sizeof (*th));
-  memset (th, 0, sizeof (*th));
+  th = calloc (1, sizeof (*th));
   th->tid = tid;
   th->h = h;
 
@@ -293,14 +300,17 @@ continue_one_thread (struct inferior_lis
 static BOOL
 child_continue (DWORD continue_status, int thread_id)
 {
-  BOOL res;
-
-  res = ContinueDebugEvent (current_event.dwProcessId,
-			    current_event.dwThreadId, continue_status);
-  if (res)
-    find_inferior (&all_threads, continue_one_thread, &thread_id);
+  /* The inferior will only continue after the ContinueDebugEvent
+     call.  */
+  find_inferior (&all_threads, continue_one_thread, &thread_id);
+  faked_breakpoint = 0;
+
+  if (!ContinueDebugEvent (current_event.dwProcessId,
+			   current_event.dwThreadId,
+			   continue_status))
+    return FALSE;
 
-  return res;
+  return TRUE;
 }
 
 /* Fetch register(s) from the current thread context.  */
@@ -1247,19 +1257,67 @@ handle_exception (struct target_waitstat
   last_sig = ourstatus->value.sig;
 }
 
-/* Get the next event from the child.  */
+
 static void
-get_child_debug_event (struct target_waitstatus *ourstatus)
+suspend_one_thread (struct inferior_list_entry *entry)
+{
+  struct thread_info *thread = (struct thread_info *) entry;
+  win32_thread_info *th = inferior_target_data (thread);
+
+  if (!th->suspended)
+    {
+      if (SuspendThread (th->h) == (DWORD) -1)
+	{
+	  DWORD err = GetLastError ();
+	  OUTMSG (("warning: SuspendThread failed in suspend_one_thread, "
+		   "(error %d): %s\n", (int) err, strwinerror (err)));
+	}
+      else
+	th->suspended = 1;
+    }
+}
+
+static void
+fake_breakpoint_event (void)
 {
-  BOOL debug_event;
+  OUTMSG2(("fake_breakpoint_event\n"));
 
+  faked_breakpoint = 1;
+
+  memset (&current_event, 0, sizeof (current_event));
+  current_event.dwThreadId = main_thread_id;
+  current_event.dwDebugEventCode = EXCEPTION_DEBUG_EVENT;
+  current_event.u.Exception.ExceptionRecord.ExceptionCode =
+    EXCEPTION_BREAKPOINT;
+
+  for_each_inferior (&all_threads, suspend_one_thread);
+}
+
+/* Get the next event from the child.  */
+
+static int
+get_child_debug_event (struct target_waitstatus *ourstatus)
+{
   last_sig = TARGET_SIGNAL_0;
   ourstatus->kind = TARGET_WAITKIND_SPURIOUS;
 
-  /* Keep the wait time low enough for confortable remote interruption,
-     but high enough so gdbserver doesn't become a bottleneck.  */
-  if (!(debug_event = WaitForDebugEvent (&current_event, 250)))
-    return;
+  /* Check if GDB sent us an interrupt request.  */
+  check_remote_input_interrupt_request ();
+
+  if (soft_interrupt_requested)
+    {
+      soft_interrupt_requested = 0;
+      fake_breakpoint_event ();
+      goto gotevent;
+    }
+
+  /* Keep the wait time low enough for confortable remote
+     interruption, but high enough so gdbserver doesn't become a
+     bottleneck.  */
+  if (!WaitForDebugEvent (&current_event, 250))
+    return 0;
+
+ gotevent:
 
   current_inferior =
     (struct thread_info *) find_inferior_id (&all_threads,
@@ -1376,6 +1434,7 @@ get_child_debug_event (struct target_wai
   current_inferior =
     (struct thread_info *) find_inferior_id (&all_threads,
 					     current_event.dwThreadId);
+  return 1;
 }
 
 /* Wait for the inferior process to change state.
@@ -1390,10 +1449,8 @@ win32_wait (char *status)
 
   while (1)
     {
-      /* Check if GDB sent us an interrupt request.  */
-      check_remote_input_interrupt_request ();
-
-      get_child_debug_event (&our_status);
+      if (!get_child_debug_event (&our_status))
+	continue;
 
       switch (our_status.kind)
 	{
@@ -1500,7 +1557,8 @@ win32_request_interrupt (void)
       && DebugBreakProcess (current_process_handle))
     return;
 
-  OUTMSG (("Could not interrupt process.\n"));
+  /* Last resort, suspend all threads manually.  */
+  soft_interrupt_requested = 1;
 }
 
 static const char *

Re: [gdbserver/win32] (4/11) New interrupting method

[Lerele]

Pedro Alves escribió:
> Hi,
>
> Here is the new method to stop the inferior, based on a mix of
> Leo's and mine patches.  The idea is simple.  Loop through all
> the inferior threads suspending them.
>
> This version doesn't contain the gdbserver priority handling,
> which means that there is a higher chance of the inferior
> breaking the method, since the operation is not atomic.  For
> example by tweaking its own thread's priorities or if the
> inferior is calling ResumeThread in its own threads while
> gdbserver is stopping them.
>
> The next patch will minimize those chances, by elevating
> gdbserver's priority while stopping the inferior with
> this method.
>
> Leo, did I miss anything?
>
> Regtested on a local i686-pc-cygwin gdbserver.
>
> Cheers,
> Pedro Alves
>
>
>
>
I think this patch breaks interrupt functionality.
The function suspend_one_thread in the new patch does not seem to 
retreive the thread context and sets the thread suspend count to 1, so 
the next call to thread_rec that should happen in gdbserver will get 
incorrect thread context.

My interrupt patch I sent initially used thread_rec to pause the child 
threads, so get context should work here.
I think it's more correct anyway to use thread_rec everywhere so that 
SuspendThread is centralized in one unique function.

Is this correct, or is there something I may have slipped reading your 
patch?

Leo.

Re: [gdbserver/win32] (4/11) New interrupting method

[Pedro Alves]

Lerele wrote:
> I think this patch breaks interrupt functionality.
> The function suspend_one_thread in the new patch does not seem to 
> retreive the thread context and sets the thread suspend count to 1, so 
> the next call to thread_rec that should happen in gdbserver will get 
> incorrect thread context.
> 
> My interrupt patch I sent initially used thread_rec to pause the child 
> threads, so get context should work here.
> I think it's more correct anyway to use thread_rec everywhere so that 
> SuspendThread is centralized in one unique function.
> 
> Is this correct, or is there something I may have slipped reading your 
> patch?
> 

You slipped reading the previous patch in the series  :-)

The previous patch in the series [1] has this hunk:

@@ -105,10 +105,19 @@ thread_rec (DWORD id, int get_context)
      return NULL;

    th = inferior_target_data (thread);
-  if (!th->suspend_count && get_context)
+  if (get_context && th->context.ContextFlags == 0)
      {
-      if (id != current_event.dwThreadId)
-       th->suspend_count = SuspendThread (th->h) + 1;
+      if (!th->suspended)
+       {
+         if (SuspendThread (th->h) == (DWORD) -1)
+           {
+             DWORD err = GetLastError ();
+             OUTMSG (("warning: SuspendThread failed in thread_rec, "
+                      "(error %d): %s\n", (int) err, strwinerror (err)));
+           }
+         else
+           th->suspended = 1;
+       }

        (*the_low_target.get_thread_context) (th, &current_event);
      }

Which means that thread_rec will fetch the register context
if it hasn't already (th->context.ContextFlags == 0),
not if the thread wasn't suspended, which makes more sense:
"If you want the registers, and you don't have them already",
as opposed to:
"If you want the registers and the thread is not suspended".

[1] - [gdbserver/win32] (3/11) Fix suspend count handling
       http://sourceware.org/ml/gdb-patches/2007-11/msg00216.html

-- 
Pedro Alves

Re: [gdbserver/win32] (4/11) New interrupting method

[Lerele]

My mistake, I did not read the entire patch chain.


Pedro Alves escribió:
> Lerele wrote:
>> I think this patch breaks interrupt functionality.
>> The function suspend_one_thread in the new patch does not seem to 
>> retreive the thread context and sets the thread suspend count to 1, 
>> so the next call to thread_rec that should happen in gdbserver will 
>> get incorrect thread context.
>>
>> My interrupt patch I sent initially used thread_rec to pause the 
>> child threads, so get context should work here.
>> I think it's more correct anyway to use thread_rec everywhere so that 
>> SuspendThread is centralized in one unique function.
>>
>> Is this correct, or is there something I may have slipped reading 
>> your patch?
>>
>
> You slipped reading the previous patch in the series  :-)
>
> The previous patch in the series [1] has this hunk:
>
> @@ -105,10 +105,19 @@ thread_rec (DWORD id, int get_context)
>      return NULL;
>
>    th = inferior_target_data (thread);
> -  if (!th->suspend_count && get_context)
> +  if (get_context && th->context.ContextFlags == 0)
>      {
> -      if (id != current_event.dwThreadId)
> -       th->suspend_count = SuspendThread (th->h) + 1;
> +      if (!th->suspended)
> +       {
> +         if (SuspendThread (th->h) == (DWORD) -1)
> +           {
> +             DWORD err = GetLastError ();
> +             OUTMSG (("warning: SuspendThread failed in thread_rec, "
> +                      "(error %d): %s\n", (int) err, strwinerror 
> (err)));
> +           }
> +         else
> +           th->suspended = 1;
> +       }
>
>        (*the_low_target.get_thread_context) (th, &current_event);
>      }
>
> Which means that thread_rec will fetch the register context
> if it hasn't already (th->context.ContextFlags == 0),
> not if the thread wasn't suspended, which makes more sense:
> "If you want the registers, and you don't have them already",
> as opposed to:
> "If you want the registers and the thread is not suspended".
>
> [1] - [gdbserver/win32] (3/11) Fix suspend count handling
>       http://sourceware.org/ml/gdb-patches/2007-11/msg00216.html
>

Re: [gdbserver/win32] (4/11) New interrupting method

[Daniel Jacobowitz]

On Mon, Nov 12, 2007 at 02:07:15AM +0000, Pedro Alves wrote:
> 2007-11-12  Leo Zayas  <lerele@champenstudios@com>
> 	    Pedro Alves  <pedro_alves@portugalmail.pt>
> 
> 	* win32-low.c (soft_interrupt_requested, faked_breakpoint): New
> 	global variables.
> 	(child_add_thread): Minor cleanup.
> 	(child_continue): Resume artificially suspended threads before
> 	calling ContinueDebugEvent.
> 	(suspend_one_thread): New.
> 	(fake_breakpoint_event): New.
> 	(get_child_debug_event): Change return type to int.  Check here if
> 	gdb sent an interrupt request.  If a soft interrupt was
> 	requested, fake a breakpoint event.  Return 0 if there is no event
> 	to handle, and 1 otherwise.
> 	(win32_wait): Don't check here if gdb sent an interrupt request.
> 	Ensure there is a valid event to handle.
> 	(win32_request_interrupt): Add soft interruption method as last resort.

This looks OK to me.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [gdbserver/win32] (4/11) New interrupting method

[Pedro Alves]

Daniel Jacobowitz wrote:
> On Mon, Nov 12, 2007 at 02:07:15AM +0000, Pedro Alves wrote:
>> 2007-11-12  Leo Zayas
>> 	    Pedro Alves  <pedro_alves@portugalmail.pt>
>>
>> 	* win32-low.c (soft_interrupt_requested, faked_breakpoint): New
>> 	global variables.
>> 	(child_add_thread): Minor cleanup.
>> 	(child_continue): Resume artificially suspended threads before
>> 	calling ContinueDebugEvent.
>> 	(suspend_one_thread): New.
>> 	(fake_breakpoint_event): New.
>> 	(get_child_debug_event): Change return type to int.  Check here if
>> 	gdb sent an interrupt request.  If a soft interrupt was
>> 	requested, fake a breakpoint event.  Return 0 if there is no event
>> 	to handle, and 1 otherwise.
>> 	(win32_wait): Don't check here if gdb sent an interrupt request.
>> 	Ensure there is a valid event to handle.
>> 	(win32_request_interrupt): Add soft interruption method as last resort.
> 
> This looks OK to me.
> 

Thanks, checked in.

-- 
Pedro Alves