[win32] Fix suspend count handling

[Pedro Alves <pedro_alves@portugalmail.pt>]

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

Hi,

This patch fixes a problem that was originally reported against
gdbserver/win32, which is also present on a native win32 debugger.

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

Let me re-describe the problem:

The current suspend_count handling breaks applications that use
SuspendThread on their own threads.  The SuspendThread function
suspends a thread and returns its previous suspend count.
If a thread was already suspended by the debuggee, say, 3
times, gdb will do one SuspendThread call more (in thread_rec),
bringing the suspend_count to 4.  Later, when the inferior
is being resumed, gdb will call ResumeThread suspend_count
times, in this case 4 times, which will leave a thread
running, while the debuggee wanted it to be suspended (with
a suspend count of 3).

This patch fixes it by turning the suspend_count counter
into a suspended flag that records if gdb itself has
suspended the thread.  Gdb will then only call SuspendThread
on a thread if it hasn't already, and will only call Resume
thread if it has called SuspendThread itself.

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 reads "set breakpoint here", switch to the
other thread with "thread 2", effectivelly forcing a thread_rec
call, which calls SuspendThread, and sets suspend_count.  Go back to
thread 1, and advance to the printf call.  The suspend_count will be
0, instead of the expected 3.

I've asked this on the gdbserver mail, but let me re-ask it,
because I haven't done that work yet :-):
    I have yet to turn this into a proper gdb test.  Is that
    wanted?  Where shall I put it?  gdb.base?  gdb.thread?  other?

No regressions on i686-pc-cygwin.

Cheers,
Pedro Alves




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

2007-11-21  Pedro Alves  <pedro_alves@portugalmail.pt>

	* win32-nat.c (thread_info_struct): Rename suspend_count to
	suspended, to be used as a flag.
	(thread_rec): Only suspend the thread if it wasn't suspended by
	gdb before.  Warn if suspending failed.
	(win32_continue): Update usage of the `suspended' flag.

---
 gdb/win32-nat.c |   33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

Index: src/gdb/win32-nat.c
===================================================================
--- src.orig/gdb/win32-nat.c	2007-11-11 23:13:04.000000000 +0000
+++ src/gdb/win32-nat.c	2007-11-12 22:35:26.000000000 +0000
@@ -112,14 +112,14 @@ static enum target_signal last_sig = TAR
 /* Set if a signal was received from the debugged process */
 
 /* Thread information structure used to track information that is
-   not available in gdb's thread structure. */
+   not available in gdb's thread structure.  */
 typedef struct thread_info_struct
   {
     struct thread_info_struct *next;
     DWORD id;
     HANDLE h;
     char *name;
-    int suspend_count;
+    int suspended;
     int reload_context;
     CONTEXT context;
     STACKFRAME sf;
@@ -244,9 +244,10 @@ check (BOOL ok, const char *file, int li
 		     GetLastError ());
 }
 
-/* Find a thread record given a thread id.
-   If get_context then also retrieve the context for this
-   thread. */
+/* Find a thread record given a thread id passed in ID.  If
+   GET_CONTEXT is not 0, then also retrieve the context for this
+   thread.  If GET_CONTEXT is negative, then don't suspend the
+   thread.  */
 static thread_info *
 thread_rec (DWORD id, int get_context)
 {
@@ -255,12 +256,21 @@ thread_rec (DWORD id, int get_context)
   for (th = &thread_head; (th = th->next) != NULL;)
     if (th->id == id)
       {
-	if (!th->suspend_count && get_context)
+	if (!th->suspended && get_context)
 	  {
 	    if (get_context > 0 && id != current_event.dwThreadId)
-	      th->suspend_count = SuspendThread (th->h) + 1;
+	      {
+		if (SuspendThread (th->h) == (DWORD) -1)
+		  {
+		    DWORD err = GetLastError ();
+		    warning (_("SuspendThread failed. (winerr %d)"),
+			     (int) err);
+		    return NULL;
+		  }
+		th->suspended = 1;
+	      }
 	    else if (get_context < 0)
-	      th->suspend_count = -1;
+	      th->suspended = -1;
 	    th->reload_context = 1;
 	  }
 	return th;
@@ -1127,12 +1137,11 @@ win32_continue (DWORD continue_status, i
 			    continue_status);
   if (res)
     for (th = &thread_head; (th = th->next) != NULL;)
-      if (((id == -1) || (id == (int) th->id)) && th->suspend_count)
+      if (((id == -1) || (id == (int) th->id)) && th->suspended)
 	{
-
-	  for (i = 0; i < th->suspend_count; i++)
+	  if (th->suspended > 0)
 	    (void) ResumeThread (th->h);
-	  th->suspend_count = 0;
+	  th->suspended = 0;
 	  if (debug_registers_changed)
 	    {
 	      /* Only change the value of the debug registers */