Re: [win32] Fix suspend count handling

[Pedro Alves <pedro_alves@portugalmail.pt>]

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

Pedro Alves wrote:
> Pedro Alves wrote:
>> On Nov 21, 2007 2:13 PM, Pierre Muller wrote:
>> That's not what I see here.  Can you show me a run where you get 4
>> only this patch applied?
>>
> 
> I did try that, but posted a log of not doing it :-).
> 
> I've just tried about 30 times, and only once I did see
> a 4 coming out ...  oh, well, one of those things.
> 

OK.  Back at my home laptop, I can reproduce that with
no problems.  Let me clarify what the 4 problem really is.
It's a race between gdb and the inferior.

Take this slightly changed test case.  The only difference
to the original version is the extra Sleep call.

#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;
         }

     Sleep (300);

     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;
}

If you do the "break at ...", "run", "thread 3", "continue"
sequence, and "..." is the "Sleep" line, you'll get 3,
but if you put the break at the /* set breakpoint here */
line, you'll get 4 (if you're (un)lucky).

The race happens due to the fact that gdb is
doing something similar to this:

win32_continue()
{
    ContinueDebugEvent (...);  /* Resumes all non suspended
                                  threads of the process.  */

    /* At this point, gdb is running concurrently with
       the inferior threads that were not suspended - which
       included the main thread of the testcase.  */
    foreach t in threads do
       if t is suspended
          ResumeThread t
       fi
    done
}

If you break at the Sleep call, when we resume, gdb will
have a bit of time to call ResumeThread on the suspended
thread of the testcase.  If you instead break at the
ResumeThread line, you'll have a good chance that the
inferior wins the race, hence the "4" result (remember
that ResumeThread returns the previous suspend count).
If we put something like this after the ResumeThread call:

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

+  Sleep (300);
+  SuspendThread (h);
+  suspend_count = ResumeThread (h);

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

... you'll see that eventually gdb will bring the
suspend count back to 3.  (A SuspendThread, ResumeThread
pair is the way to get at the suspend count.)

> Since the watchpoint patch should fix this, what shall I do?  Shall I merge
> the two and resubmit, or leave it at that ?  They've already been tested
> together without regressions.
> 

Here is the merge from the patch I posted at the start of the
thread with this patch:
    [win32] Fix watchpoint support
    http://sourceware.org/ml/gdb-patches/2007-11/msg00390.html

This patch fixes both the suspend_count
handling, and the watchpoint support.

Thanks Pierre, for looking at it.

OK ?

-- 
Pedro Alves

[-- Attachment #2: suspend_count_fix_plus_watchpoints.diff --]
[-- Type: text/x-diff, Size: 5177 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_add_thread): Set Dr6 to 0xffff0ff0.
	(win32_resume): Likewise.
	(win32_continue): Set Dr6 to 0xffff0ff0.  Update usage of the
	`suspended' flag.  Do ContinueDebugEvent after resuming the
	suspended threads, not before.  Set threads' contexts before
	resuming them, not after.

---
 gdb/win32-nat.c |   80 +++++++++++++++++++++++++++++++-------------------------
 1 file changed, 45 insertions(+), 35 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-21 22:39:56.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;
@@ -294,8 +304,7 @@ win32_add_thread (DWORD id, HANDLE h)
       th->context.Dr1 = dr[1];
       th->context.Dr2 = dr[2];
       th->context.Dr3 = dr[3];
-      /* th->context.Dr6 = dr[6];
-      FIXME: should we set dr6 also ?? */
+      th->context.Dr6 = 0xffff0ff0;
       th->context.Dr7 = dr[7];
       CHECK (SetThreadContext (th->h, &th->context));
       th->context.ContextFlags = 0;
@@ -1122,32 +1131,34 @@ win32_continue (DWORD continue_status, i
 		  current_event.dwProcessId, current_event.dwThreadId,
 		  continue_status == DBG_CONTINUE ?
 		  "DBG_CONTINUE" : "DBG_EXCEPTION_NOT_HANDLED"));
+
+  for (th = &thread_head; (th = th->next) != NULL;)
+    if ((id == -1 || id == (int) th->id)
+	&& th->suspended)
+      {
+	if (debug_registers_changed)
+	  {
+	    th->context.ContextFlags |= CONTEXT_DEBUG_REGISTERS;
+	    th->context.Dr0 = dr[0];
+	    th->context.Dr1 = dr[1];
+	    th->context.Dr2 = dr[2];
+	    th->context.Dr3 = dr[3];
+	    th->context.Dr6 = 0xffff0ff0;
+	    th->context.Dr7 = dr[7];
+	  }
+	if (th->context.ContextFlags)
+	  {
+	    CHECK (SetThreadContext (th->h, &th->context));
+	    th->context.ContextFlags = 0;
+	  }
+	if (th->suspended > 0)
+	  (void) ResumeThread (th->h);
+	th->suspended = 0;
+      }
+
   res = ContinueDebugEvent (current_event.dwProcessId,
 			    current_event.dwThreadId,
 			    continue_status);
-  if (res)
-    for (th = &thread_head; (th = th->next) != NULL;)
-      if (((id == -1) || (id == (int) th->id)) && th->suspend_count)
-	{
-
-	  for (i = 0; i < th->suspend_count; i++)
-	    (void) ResumeThread (th->h);
-	  th->suspend_count = 0;
-	  if (debug_registers_changed)
-	    {
-	      /* Only change the value of the debug registers */
-	      th->context.ContextFlags = CONTEXT_DEBUG_REGISTERS;
-	      th->context.Dr0 = dr[0];
-	      th->context.Dr1 = dr[1];
-	      th->context.Dr2 = dr[2];
-	      th->context.Dr3 = dr[3];
-	      /* th->context.Dr6 = dr[6];
-		 FIXME: should we set dr6 also ?? */
-	      th->context.Dr7 = dr[7];
-	      CHECK (SetThreadContext (th->h, &th->context));
-	      th->context.ContextFlags = 0;
-	    }
-	}
 
   debug_registers_changed = 0;
   return res;
@@ -1233,8 +1244,7 @@ win32_resume (ptid_t ptid, int step, enu
 	      th->context.Dr1 = dr[1];
 	      th->context.Dr2 = dr[2];
 	      th->context.Dr3 = dr[3];
-	      /* th->context.Dr6 = dr[6];
-	       FIXME: should we set dr6 also ?? */
+	      th->context.Dr6 = 0xffff0ff0;
 	      th->context.Dr7 = dr[7];
 	    }
 	  CHECK (SetThreadContext (th->h, &th->context));