RE: [win32] Fix suspend count handling

["Pierre Muller" <muller at ics dot u-strasbg dot fr>]

  I checked again the new patch and
the testsuite results are exactly the
same as I already reported for the hardware watchpoint fix
alone.
  I think that this patch is good,
but only Christopher can approve it.

Pierre

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Pedro Alves
> Sent: Thursday, November 22, 2007 12:19 AM
> To: Pierre Muller
> Cc: gdb-patches@sourceware.org
> Subject: 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