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