Re: [win32] Fix suspend count handling

[Pedro Alves <pedro_alves@portugalmail.pt>]

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

Christopher Faylor wrote:

>> -/* 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.  */
> 
> I don't see any reason to capitalize get_context in the comment.
> 

That's what the GNU coding standards prefer:

[5.2 Commenting Your Work]
"The comment on a function is much clearer if you use the argument
names to speak about the argument values.  The variable name
itself should be lower case, but write it in upper case when
you are speaking about the value rather than the variable
itself.  Thus, “the node number NODE_NUM” rather than “an inode”."

In fact, I shouldn't be capitalizing ID.  Fixed that.

>> @@ -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;
> 
> This, and similar cases, needs to use a #define with an explanatory comment.
> 
> With the above minor changes, this looks ok.
> 

Added a #define and checked in (see attached).

> I have to ask, however, if the SuspendThread's are even needed at all.
> When I was writing this code, I wasn't entirely sure that gdb needed to
> do anything like this but I erred on the side of caution.  So, I'm
> wondering if things would still work ok if the
> SuspendThread/ResumeThread stuff was gone.
> 

I wonder that too.  The docs say that we must not retrieve a thread
context while the thread is running.  When the inferior is stopped due
to a debug event the whole process is frozen, so in that case it should
safe.  I can give it a try later on, but it may take me a while to
confirm if it really is safe, especially because I currently only
have access to Windows XP SP2 and WinCE  (from the archives, I
get the impression Win2000 is pickier.)

OTOH, we'll still need to suspend threads if we want
to implement support for debugging one thread while the
others are running, or to step just one thread while
the others are stopped, or if we want to support stopping the
inferior without relying on a console available to handle
GenerateConsoleCtrlEvent; but these are different stories.

-- 
Pedro Alves



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

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

	* win32-nat.c (DR6_CLEAR_VALUE): New define.
	(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 DR6_CLEAR_VALUE.
	(win32_continue): Set Dr6 to DR6_CLEAR_VALUE.  Update usage of the
	`suspended' flag.  Do ContinueDebugEvent after resuming the
	suspended threads, not before.  Set threads' contexts before
	resuming them, not after.
	(win32_resume): Set Dr6 to DR6_CLEAR_VALUE.

---
 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-24 11:44:14.000000000 +0000
+++ src/gdb/win32-nat.c	2007-11-24 11:44:56.000000000 +0000
@@ -91,6 +91,7 @@ enum
 static unsigned dr[8];
 static int debug_registers_changed;
 static int debug_registers_used;
+#define DR6_CLEAR_VALUE 0xffff0ff0
 
 /* The string sent by cygwin when it processes a signal.
    FIXME: This should be in a cygwin include file. */
@@ -112,14 +113,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 +245,9 @@ 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.  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 = DR6_CLEAR_VALUE;
       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 = DR6_CLEAR_VALUE;
+	    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 = DR6_CLEAR_VALUE;
 	      th->context.Dr7 = dr[7];
 	    }
 	  CHECK (SetThreadContext (th->h, &th->context));