[gdbserver/win32] (3/11) Fix suspend count handling

[Pedro Alves <pedro_alves@portugalmail.pt>]

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

Hi,

As Leo reported earlier, the suspend_count handling breaks
applications that use SuspendThread on their own threads.
SuspendThread returns the previous suspend count of the
thread, and gdbserver is calling ResumeThread that many times.
If the Thread was already suspended, say, 3 times, gdbserver will
do one SuspendThread call (in thread_rec), and 4 ResumeThread
calls when resuming.

This patch fixes it by turning the suspend_count counter into
a suspended flag.

Here is a test that shows the problem:

     >cat ~/suspendfix/main.c
#include <windows.h>

HANDLE started;
HANDLE stop;

DWORD WINAPI
thread_start (void *arg)
{
       SetEvent (started);
       WaitForSingleObject (stop, INFINITE);
       return 0;
}

int
main (int argc, char **argv)
{
       int i;
       DWORD suspend_count;
       started = CreateEvent (NULL, TRUE, FALSE, NULL);
       stop = CreateEvent (NULL, TRUE, FALSE, NULL);

       HANDLE h = CreateThread (NULL, 0, thread_start, NULL,
                                0, NULL);

       WaitForSingleObject (started, INFINITE);

       for (i = 0; i < 3; i++)
         if (SuspendThread (h) == (DWORD) -1)
           {
             printf ("SuspendThreadFailed\n");
             return 1;
           }

       suspend_count = ResumeThread (h); /* set breakpoint here */

       printf ("%lu\n", suspend_count); /* should be 3 */

       while ((suspend_count = ResumeThread (h)) != 0
              && suspend_count != -1)
         ;
       SetEvent (stop);
       WaitForSingleObject (h, INFINITE);
       CloseHandle (h);
       CloseHandle (started);
       CloseHandle (stop);
       return 0;
}

Set a breakpoint where it read "set breakpoint here", switch to the
other thread with "thread 2", effectivelly forcing a thread_rec
call, which calls SuspendThread, and sets suspend_count.  Since the
win32 support can't resume with a different thread, so go back to
thread 1, and step.  The suspend_count will be 0, instead of 3.

I have yet to turn this into a proper gdb test.  Is that wanted?  Where
shall I put it?  gdb.base?  gdb.thread?  other?

Note: The native (gdb/win32-nat.c) support also has the same bug.

Regtested on a local i686-pc-cygwin gdbserver, and against
a arm-wince remote.

Cheers,
Pedro Alves




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

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

	* win32-low.h (win32_thread_info): Add descriptions to the
	structure members .  Replace `suspend_count' counter by a
	`suspended' flag.

	* win32-low.c (thread_rec): Update condition of when to get the
	context from the inferior.  Rely on ContextFlags being set if it
	has already been retrieved.  Only suspend the inferior thread if
	we haven't already.  Warn if that fails.
	(continue_one_thread): s/suspend_count/suspended/.  Only call
	ResumeThread once.  Warn if that fails.

---
 gdb/gdbserver/win32-low.c |   28 ++++++++++++++++++++--------
 gdb/gdbserver/win32-low.h |    9 ++++++++-
 2 files changed, 28 insertions(+), 9 deletions(-)

Index: src/gdb/gdbserver/win32-low.c
===================================================================
--- src.orig/gdb/gdbserver/win32-low.c	2007-11-04 23:50:36.000000000 +0000
+++ src/gdb/gdbserver/win32-low.c	2007-11-04 23:51:44.000000000 +0000
@@ -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);
     }
@@ -259,10 +268,9 @@ continue_one_thread (struct inferior_lis
   struct thread_info *thread = (struct thread_info *) this_thread;
   int thread_id = * (int *) id_ptr;
   win32_thread_info *th = inferior_target_data (thread);
-  int i;
 
   if ((thread_id == -1 || thread_id == th->tid)
-      && th->suspend_count)
+      && th->suspended)
     {
       if (th->context.ContextFlags)
 	{
@@ -270,9 +278,13 @@ continue_one_thread (struct inferior_lis
 	  th->context.ContextFlags = 0;
 	}
 
-      for (i = 0; i < th->suspend_count; i++)
-	(void) ResumeThread (th->h);
-      th->suspend_count = 0;
+      if (ResumeThread (th->h) == (DWORD) -1)
+	{
+	  DWORD err = GetLastError ();
+	  OUTMSG (("warning: ResumeThread failed in continue_one_thread, "
+		   "(error %d): %s\n", (int) err, strwinerror (err)));
+	}
+      th->suspended = 0;
     }
 
   return 0;
Index: src/gdb/gdbserver/win32-low.h
===================================================================
--- src.orig/gdb/gdbserver/win32-low.h	2007-11-04 23:50:36.000000000 +0000
+++ src/gdb/gdbserver/win32-low.h	2007-11-04 23:51:16.000000000 +0000
@@ -22,9 +22,16 @@
    each thread.  */
 typedef struct win32_thread_info
 {
+  /* The Win32 thread identifier.  */
   DWORD tid;
+
+  /* The handle to the thread.  */
   HANDLE h;
-  int suspend_count;
+
+  /* Non zero if SuspendThread was called on this thread.  */
+  int suspended;
+
+  /* The context of the thread.  */
   CONTEXT context;
 } win32_thread_info;