This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [win32] Fix suspend count handling


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



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]