[win32] Fix suspend count handling

[win32] Fix suspend count handling

[Pedro Alves]

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

Hi,

This patch fixes a problem that was originally reported against
gdbserver/win32, which is also present on a native win32 debugger.

The gdbserver patch is here:
   [gdbserver/win32] (3/11) Fix suspend count handling
   http://sourceware.org/ml/gdb-patches/2007-11/msg00216.html

Let me re-describe the problem:

The current suspend_count handling breaks applications that use
SuspendThread on their own threads.  The SuspendThread function
suspends a thread and returns its previous suspend count.
If a thread was already suspended by the debuggee, say, 3
times, gdb will do one SuspendThread call more (in thread_rec),
bringing the suspend_count to 4.  Later, when the inferior
is being resumed, gdb will call ResumeThread suspend_count
times, in this case 4 times, which will leave a thread
running, while the debuggee wanted it to be suspended (with
a suspend count of 3).

This patch fixes it by turning the suspend_count counter
into a suspended flag that records if gdb itself has
suspended the thread.  Gdb will then only call SuspendThread
on a thread if it hasn't already, and will only call Resume
thread if it has called SuspendThread itself.

Here is a test that shows the problem:

      >cat ~/suspendfix/main.c
#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;
            }

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

Set a breakpoint where it reads "set breakpoint here", switch to the
other thread with "thread 2", effectivelly forcing a thread_rec
call, which calls SuspendThread, and sets suspend_count.  Go back to
thread 1, and advance to the printf call.  The suspend_count will be
0, instead of the expected 3.

I've asked this on the gdbserver mail, but let me re-ask it,
because I haven't done that work yet :-):
    I have yet to turn this into a proper gdb test.  Is that
    wanted?  Where shall I put it?  gdb.base?  gdb.thread?  other?

No regressions on i686-pc-cygwin.

Cheers,
Pedro Alves




[-- Attachment #2: nat_suspend_count_fix.diff --]
[-- Type: text/x-diff, Size: 2936 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_continue): Update usage of the `suspended' flag.

---
 gdb/win32-nat.c |   33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 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-12 22:35:26.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;
@@ -1127,12 +1137,11 @@ win32_continue (DWORD continue_status, i
 			    continue_status);
   if (res)
     for (th = &thread_head; (th = th->next) != NULL;)
-      if (((id == -1) || (id == (int) th->id)) && th->suspend_count)
+      if (((id == -1) || (id == (int) th->id)) && th->suspended)
 	{
-
-	  for (i = 0; i < th->suspend_count; i++)
+	  if (th->suspended > 0)
 	    (void) ResumeThread (th->h);
-	  th->suspend_count = 0;
+	  th->suspended = 0;
 	  if (debug_registers_changed)
 	    {
 	      /* Only change the value of the debug registers */

RE: [win32] Fix suspend count handling

[Pierre Muller]

  Hi Pedro,

  I agree with you that the ResumeThread should be called
only once, even if the suspend_count is > 1, but when I tested your test
application,
I obtained 4 as a result with the current gdb head.
  This is because in fact the thread suspend_count are
only restored after ContinueDebugEvent is called.
  Thus the debugged event can possibly called 
ResumeThread before gdb does so, thus obtaining a
Thread count of 4 instead of 0...
  The first answer that comes to mind would be to
called ResumeThread before calling ContinueDebugEvent,
but that would mean that other threads would be restarted before
the thread that caused the debug event, which is
probably also not good.

  I would propose to do it this way:
  -for threads that where already suspended, and for which gdb
increased the suspend count, we should restore the suspend count before
calling ContinueDebugEvent, while for the thread that were
not suspended before, we should resume them after the call
to ContinueDebugEvent.
  This means that we should keep the suspend_count variable as an int
and loop once before ContinueDebugEvent call (restoring
the suspend count of debuggee suspended threads,
identified by the fact that their suspend_count is > 1,
calling ResumeThread only once, as you suggest in your patch), and
loop again after the call to resume all other threads that
were running at the time of the debug event (those which
have a suspend_count of 1).

Pierre

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Pedro Alves
> Sent: Wednesday, November 21, 2007 1:35 AM
> To: gdb-patches@sourceware.org
> Subject: [win32] Fix suspend count handling
> 
> Hi,
> 
> This patch fixes a problem that was originally reported against
> gdbserver/win32, which is also present on a native win32 debugger.
> 
> The gdbserver patch is here:
>    [gdbserver/win32] (3/11) Fix suspend count handling
>    http://sourceware.org/ml/gdb-patches/2007-11/msg00216.html
> 
> Let me re-describe the problem:
> 
> The current suspend_count handling breaks applications that use
> SuspendThread on their own threads.  The SuspendThread function
> suspends a thread and returns its previous suspend count.
> If a thread was already suspended by the debuggee, say, 3 times, gdb
> will do one SuspendThread call more (in thread_rec), bringing the
> suspend_count to 4.  Later, when the inferior is being resumed, gdb
> will call ResumeThread suspend_count times, in this case 4 times, which
> will leave a thread running, while the debuggee wanted it to be
> suspended (with a suspend count of 3).
> 
> This patch fixes it by turning the suspend_count counter into a
> suspended flag that records if gdb itself has suspended the thread.
> Gdb will then only call SuspendThread on a thread if it hasn't already,
> and will only call Resume thread if it has called SuspendThread itself.
> 
> Here is a test that shows the problem:
> 
>       >cat ~/suspendfix/main.c
> #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;
>             }
> 
>         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;
> }
> 
> Set a breakpoint where it reads "set breakpoint here", switch to the
> other thread with "thread 2", effectivelly forcing a thread_rec call,
> which calls SuspendThread, and sets suspend_count.  Go back to thread
> 1, and advance to the printf call.  The suspend_count will be 0,
> instead of the expected 3.
> 
> I've asked this on the gdbserver mail, but let me re-ask it, because I
> haven't done that work yet :-):
>     I have yet to turn this into a proper gdb test.  Is that
>     wanted?  Where shall I put it?  gdb.base?  gdb.thread?  other?
> 
> No regressions on i686-pc-cygwin.
> 
> Cheers,
> Pedro Alves
> 
> 

Re: [win32] Fix suspend count handling

[Pedro Alves]

On Nov 21, 2007 11:25 AM, Pierre Muller wrote:
>   Hi Pedro,
>
>   I agree with you that the ResumeThread should be called
> only once, even if the suspend_count is > 1, but when I tested your test
> application,
> I obtained 4 as a result with the current gdb head.
>   This is because in fact the thread suspend_count are
> only restored after ContinueDebugEvent is called.

I suspect you didn't reproduce the exact steps I described, probably
because I didn't describe it sufficiently.  Here's a test run:

$ gdb/gdb.exe main.exe
GNU gdb 6.7.50.20071121
Copyright (C) 2007 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-pc-cygwin"...
(gdb) list 37
32              printf ("SuspendThreadFailed\n");
33              return 1;
34            }
35
36
37        suspend_count = ResumeThread (h); /* set breakpoint here */
38
39        printf ("%lu\n", suspend_count); /* should be 3 */
40
41        while ((suspend_count = ResumeThread (h)) != 0
(gdb) b 37
Breakpoint 1 at 0x40119a: file main.c, line 37.
(gdb) r
Starting program: /d/gdb/build/main.exe

Breakpoint 1, main (argc=1, argv=0x6b3270) at main.c:37
37        suspend_count = ResumeThread (h); /* set breakpoint here */
(gdb) info threads
  3 thread 11344.0x2e54  0x7c90eb94 in ntdll!LdrAccessResource () from
/cygdrive/c/WINNT/system32/ntdll.dll
  2 thread 11344.0x1e4c  0x7c90eb94 in ntdll!LdrAccessResource () from
/cygdrive/c/WINNT/system32/ntdll.dll
* 1 thread 11344.0x2d00  main (argc=1, argv=0x6b3270) at main.c:37
(gdb) thread 3
[Switching to thread 3 (thread 11344.0x2e54)]#0  0x7c90eb94 in
ntdll!LdrAccessResource ()
   from /cygdrive/c/WINNT/system32/ntdll.dll
(gdb) thread 1
[Switching to thread 1 (thread 11344.0x2d00)]#0  main (argc=1,
argv=0x6b3270) at main.c:37
37        suspend_count = ResumeThread (h); /* set breakpoint here */
(gdb) n
39        printf ("%lu\n", suspend_count); /* should be 3 */
(gdb) p suspend_count
$1 = 0
(gdb)

You must switch to the suspended thread for gdb to call SuspendThread
on it, otherwise, gdb will not try to ResumeThread it, and the bug isn't
triggered.  I mentioned "thread 2" in the other mail, but as seen on
the "info threads" result, the gdb tid of the suspended thread will vary.
A proper testcase would switch to all threads one by one and then
back to thread 1 to ensure the bug is triggered.

>   Thus the debugged event can possibly called
> ResumeThread before gdb does so, thus obtaining a
> Thread count of 4 instead of 0...
>   The first answer that comes to mind would be to
> called ResumeThread before calling ContinueDebugEvent,
> but that would mean that other threads would be restarted before
> the thread that caused the debug event, which is
> probably also not good.
>

Not true.  See here:
  [The Win32 Debugging Application Programming Interface]
  http://msdn2.microsoft.com/en-us/library/ms809754.aspx

  "When a debug event occurs, execution of that process is suspended until
  the debugger calls the ContinueDebugEvent function. Consequently,
all threads in
  the process are suspended while the debugger is processing the debug event."

In fact my patch at:
 [win32] Fix watchpoint support
 http://sourceware.org/ml/gdb-patches/2007-11/msg00390.html

... ensures that ContinueDebugEvent is always called last.

Cheers,
Pedro Alves

RE: [win32] Fix suspend count handling

[Pierre Muller]

> -----Original Message-----
> From: alves.ped@gmail.com [mailto:alves.ped@gmail.com] On Behalf Of
> Pedro Alves
> Sent: Wednesday, November 21, 2007 2:43 PM
> To: gdb-patches@sourceware.org
> Cc: Pierre Muller
> Subject: Re: [win32] Fix suspend count handling
> 
> On Nov 21, 2007 11:25 AM, Pierre Muller wrote:
> >   Hi Pedro,
> >
> >   I agree with you that the ResumeThread should be called
> > only once, even if the suspend_count is > 1, but when I tested your
> test
> > application,
> > I obtained 4 as a result with the current gdb head.
> >   This is because in fact the thread suspend_count are
> > only restored after ContinueDebugEvent is called.
> 
> I suspect you didn't reproduce the exact steps I described, probably
> because I didn't describe it sufficiently.  Here's a test run:
  You are right indeed (see below) 
> $ gdb/gdb.exe main.exe
> GNU gdb 6.7.50.20071121
> Copyright (C) 2007 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later
> <http://gnu.org/licenses/gpl.html>
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.  Type "show
> copying"
> and "show warranty" for details.
> This GDB was configured as "i686-pc-cygwin"...
> (gdb) list 37
> 32              printf ("SuspendThreadFailed\n");
> 33              return 1;
> 34            }
> 35
> 36
> 37        suspend_count = ResumeThread (h); /* set breakpoint here */
> 38
> 39        printf ("%lu\n", suspend_count); /* should be 3 */
> 40
> 41        while ((suspend_count = ResumeThread (h)) != 0
> (gdb) b 37
> Breakpoint 1 at 0x40119a: file main.c, line 37.
> (gdb) r
> Starting program: /d/gdb/build/main.exe
> 
> Breakpoint 1, main (argc=1, argv=0x6b3270) at main.c:37
> 37        suspend_count = ResumeThread (h); /* set breakpoint here */
> (gdb) info threads
>   3 thread 11344.0x2e54  0x7c90eb94 in ntdll!LdrAccessResource () from
> /cygdrive/c/WINNT/system32/ntdll.dll
>   2 thread 11344.0x1e4c  0x7c90eb94 in ntdll!LdrAccessResource () from
> /cygdrive/c/WINNT/system32/ntdll.dll
> * 1 thread 11344.0x2d00  main (argc=1, argv=0x6b3270) at main.c:37

  This comes from the fact that cygwin startup code generated a thread
dedicated to signal handling,
but I figures that out after a while, while testing your patch...
> (gdb) thread 3
> [Switching to thread 3 (thread 11344.0x2e54)]#0  0x7c90eb94 in
> ntdll!LdrAccessResource ()
>    from /cygdrive/c/WINNT/system32/ntdll.dll
> (gdb) thread 1
> [Switching to thread 1 (thread 11344.0x2d00)]#0  main (argc=1,
> argv=0x6b3270) at main.c:37
> 37        suspend_count = ResumeThread (h); /* set breakpoint here */
> (gdb) n
> 39        printf ("%lu\n", suspend_count); /* should be 3 */
> (gdb) p suspend_count
> $1 = 0
> (gdb)

  Your patch does indeed correct this to 3, which is what is wanted.
But if you use 'continue' instead of 'next' when you are at line
37, you will get '4' printed out instead of three, because
the main thread will have call SuspendThread before gdb does.
This is at least what happens on my computer.

> 
> You must switch to the suspended thread for gdb to call SuspendThread
> on it, otherwise, gdb will not try to ResumeThread it, and the bug
> isn't
> triggered.  I mentioned "thread 2" in the other mail, but as seen on
> the "info threads" result, the gdb tid of the suspended thread will
> vary.
> A proper testcase would switch to all threads one by one and then
> back to thread 1 to ensure the bug is triggered.
> 
> >   Thus the debugged event can possibly called
> > ResumeThread before gdb does so, thus obtaining a
> > Thread count of 4 instead of 0...
> >   The first answer that comes to mind would be to
> > called ResumeThread before calling ContinueDebugEvent,
> > but that would mean that other threads would be restarted before
> > the thread that caused the debug event, which is
> > probably also not good.
> >
> 
> Not true.  See here:
>   [The Win32 Debugging Application Programming Interface]
>   http://msdn2.microsoft.com/en-us/library/ms809754.aspx
> 
>   "When a debug event occurs, execution of that process is suspended
> until
>   the debugger calls the ContinueDebugEvent function. Consequently,
> all threads in
>   the process are suspended while the debugger is processing the debug
> event."
> 
> In fact my patch at:
>  [win32] Fix watchpoint support
>  http://sourceware.org/ml/gdb-patches/2007-11/msg00390.html
> 
> ... ensures that ContinueDebugEvent is always called last.

  OK, in that case it is indeed correct to do all the ResumeThread calls
before ContinueDebugEvent is called, and thus the suspend_count can be
replaced
by suspended Boolean value.
  But only the combination of the two patches will ensure to 
always obtain '3' using 'continue' or 'next'.


Pierre

Re: [win32] Fix suspend count handling

[Pedro Alves]

On Nov 21, 2007 2:13 PM, Pierre Muller wrote:

>   Your patch does indeed correct this to 3, which is what is wanted.
> But if you use 'continue' instead of 'next' when you are at line
> 37, you will get '4' printed out instead of three, because
> the main thread will have call SuspendThread before gdb does.
> This is at least what happens on my computer.
>

Are you sure this was from a gdb with this patch installed?  I get 4
without the patch, but with this patch (*without* the other
watchpoint patch on top) I still get 3 no matter how I try it:

$ gdb/gdb.exe main.exe
GNU gdb 6.7.50.20071121
Copyright (C) 2007 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-pc-cygwin"...
(gdb) b 37
Breakpoint 1 at 0x40119a: file main.c, line 37.
(gdb) r
Starting program: /d/gdb/build/main.exe

Breakpoint 1, main (argc=1, argv=0x6b3270) at main.c:37
37        suspend_count = ResumeThread (h); /* set breakpoint here */
(gdb) c
Continuing.
3

Program exited normally.
(gdb)

>
>   OK, in that case it is indeed correct to do all the ResumeThread calls
> before ContinueDebugEvent is called, and thus the suspend_count can be
> replaced
> by suspended Boolean value.

Not a boolean, its a three state -- suspend_count also accepted -1, and I've
kept that.

>   But only the combination of the two patches will ensure to
> always obtain '3' using 'continue' or 'next'.

That's not what I see here.  Can you show me a run where you get 4
only this patch applied?

Cheers,
Pedro Alves

RE: [win32] Fix suspend count handling

[Pierre Muller]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Pedro Alves
> Sent: Wednesday, November 21, 2007 4:08 PM
> To: Pierre Muller
> Cc: gdb-patches@sourceware.org
> Subject: Re: [win32] Fix suspend count handling
> 
> On Nov 21, 2007 2:13 PM, Pierre Muller wrote:
> 
> >   Your patch does indeed correct this to 3, which is what is wanted.
> > But if you use 'continue' instead of 'next' when you are at line
> > 37, you will get '4' printed out instead of three, because
> > the main thread will have call SuspendThread before gdb does.
> > This is at least what happens on my computer.
> >
> 
> Are you sure this was from a gdb with this patch installed?  I get 4
> without the patch, but with this patch (*without* the other
> watchpoint patch on top) I still get 3 no matter how I try it:
> 
> $ gdb/gdb.exe main.exe
> GNU gdb 6.7.50.20071121
> Copyright (C) 2007 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later
> <http://gnu.org/licenses/gpl.html>
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.  Type "show
> copying"
> and "show warranty" for details.
> This GDB was configured as "i686-pc-cygwin"...
> (gdb) b 37
> Breakpoint 1 at 0x40119a: file main.c, line 37.
> (gdb) r
> Starting program: /d/gdb/build/main.exe
> 
> Breakpoint 1, main (argc=1, argv=0x6b3270) at main.c:37
> 37        suspend_count = ResumeThread (h); /* set breakpoint here */
> (gdb) c
> Continuing.
> 3
  You need to switch to the thread that is suspended to get '4' as a result,
otherwise you get '3'.

> Program exited normally.
> (gdb)
> 
> >
> >   OK, in that case it is indeed correct to do all the ResumeThread
> calls
> > before ContinueDebugEvent is called, and thus the suspend_count can
> be
> > replaced
> > by suspended Boolean value.
> 
> Not a boolean, its a three state -- suspend_count also accepted -1, and
> I've
> kept that.
> 
> >   But only the combination of the two patches will ensure to
> > always obtain '3' using 'continue' or 'next'.
> 
> That's not what I see here.  Can you show me a run where you get 4
> only this patch applied?
Just add a 'thread 3' after the breakpoint is trigged and 'continue'.

Pierre

Re: [win32] Fix suspend count handling

[Pedro Alves]

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.

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.

-- 
Pedro Alves

Re: [win32] Fix suspend count handling

[Pedro Alves]

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

RE: [win32] Fix suspend count handling

[Pierre Muller]

  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

Re: [win32] Fix suspend count handling

[Christopher Faylor]

On Wed, Nov 21, 2007 at 11:18:38PM +0000, Pedro Alves wrote:
> 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.  */

I don't see any reason to capitalize get_context in the comment.

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

This, and similar cases, needs to use a #define with an explanatory comment.

With the above minor changes, this looks ok.

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.

cgf

Re: [win32] Fix suspend count handling

[Lerele]

Christopher Faylor escribió:
> On Wed, Nov 21, 2007 at 11:18:38PM +0000, Pedro Alves wrote:
>   
>> 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.  */
>>     
>
> I don't see any reason to capitalize get_context in the comment.
>
>   
>> 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;
>>     
>
> This, and similar cases, needs to use a #define with an explanatory comment.
>
> With the above minor changes, this looks ok.
>
> 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.
>
> cgf
>
>   

I think they are needed. They were anyway with the new gdbserver based 
(vs. Win32 API based) interrupt code I sent several days ago, and that 
so very kindly Pedro prepared for commitment, but that I still haven't 
found the time to sit down and look at them (however I'm absolutely sure 
they're just fine), I guess his changes must be similar to what I sent 
in the first place.
Apart from this, there's also the case where (at least for gdbserver) 
socket data is received asynchronously while the child is running. This 
socket data could indicate gdbserver to set/enable/disable a breakpoint, 
read thread registers, etc., and this kind of things may require to stop 
the child using SuspendThread.
Right?

Leo.

Re: [win32] Fix suspend count handling

[Lerele]

Lerele escribió:
> Christopher Faylor escribió:
>> On Wed, Nov 21, 2007 at 11:18:38PM +0000, Pedro Alves wrote:
>>  
>>> 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.  */
>>>     
>>
>> I don't see any reason to capitalize get_context in the comment.
>>
>>  
>>> 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;
>>>     
>>
>> This, and similar cases, needs to use a #define with an explanatory 
>> comment.
>>
>> With the above minor changes, this looks ok.
>>
>> 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.
>>
>> cgf
>>
>>   
>
> I think they are needed. They were anyway with the new gdbserver based 
> (vs. Win32 API based) interrupt code I sent several days ago, and that 
> so very kindly Pedro prepared for commitment, but that I still haven't 
> found the time to sit down and look at them (however I'm absolutely 
> sure they're just fine), I guess his changes must be similar to what I 
> sent in the first place.
> Apart from this, there's also the case where (at least for gdbserver) 
> socket data is received asynchronously while the child is running. 
> This socket data could indicate gdbserver to set/enable/disable a 
> breakpoint, read thread registers, etc., and this kind of things may 
> require to stop the child using SuspendThread.
> Right?
>
> Leo.
>
>


I'd also like to ask you a question, concerning a comment from Pedro 
several messages back that has stayed around in my mind since then.

It's not related with this specific thread title, but since it's 
gdbserver/win32 related, I haven't found appropriate to open a new 
thread just for this simple question.

The issue is near the end of:
http://sourceware.org/ml/gdb-patches/2007-11/msg00041.html

It's about the fact of gdb win32-nat.c some time ago having the 
interrupt functionality similar to the one that has been recently 
implemented using SuspendThread (versus using DebugBreak kind of 
functions). Pedro commented back then that win32-nat.c did have sometime 
in the past a similar implementation [that must have been dropped].
Do you know/remember if it was dropped for a specific reason?

My concern is about any possible drawback for this new interrupt 
functionality method.

Regards,
Leo.

Re: [win32] Fix suspend count handling

[Pedro Alves]

Lerele wrote:

> 
> I'd also like to ask you a question, concerning a comment from Pedro 
> several messages back that has stayed around in my mind since then.
> 
> It's not related with this specific thread title, but since it's 
> gdbserver/win32 related, I haven't found appropriate to open a new 
> thread just for this simple question.
> 
> The issue is near the end of:
> http://sourceware.org/ml/gdb-patches/2007-11/msg00041.html
> 
> It's about the fact of gdb win32-nat.c some time ago having the 
> interrupt functionality similar to the one that has been recently 
> implemented using SuspendThread (versus using DebugBreak kind of 
> functions). Pedro commented back then that win32-nat.c did have sometime 
> in the past a similar implementation [that must have been dropped].
> Do you know/remember if it was dropped for a specific reason?
> 

Humm, I guess I mistaked win32-nat.c for the winpdo-nat.c
files on Apple's gdb:

See in:
http://www.opensource.apple.com/darwinsource/tarballs/other/gdb-186.1.tar.gz

In src/gdb-next/winpdo-nat.c, you'll see a SuspendThreads mechanism.
The file header states 1996 as the latest copyright year, but they
may have easilly failed to update it.  I'm not sure if win32-nat
ever had this mechanism, or if it was only added by Apple.  Our
cvs history only goes back till 1999.

I did say: "I *think* that if...".  :-)

-- 
Pedro Alves

Re: [win32] Fix suspend count handling

[Lerele]

Pedro Alves escribió:
> Lerele wrote:
>
>>
>> I'd also like to ask you a question, concerning a comment from Pedro 
>> several messages back that has stayed around in my mind since then.
>>
>> It's not related with this specific thread title, but since it's 
>> gdbserver/win32 related, I haven't found appropriate to open a new 
>> thread just for this simple question.
>>
>> The issue is near the end of:
>> http://sourceware.org/ml/gdb-patches/2007-11/msg00041.html
>>
>> It's about the fact of gdb win32-nat.c some time ago having the 
>> interrupt functionality similar to the one that has been recently 
>> implemented using SuspendThread (versus using DebugBreak kind of 
>> functions). Pedro commented back then that win32-nat.c did have 
>> sometime in the past a similar implementation [that must have been 
>> dropped].
>> Do you know/remember if it was dropped for a specific reason?
>>
>
> Humm, I guess I mistaked win32-nat.c for the winpdo-nat.c
> files on Apple's gdb:
>
> See in:
> http://www.opensource.apple.com/darwinsource/tarballs/other/gdb-186.1.tar.gz 
>
>
> In src/gdb-next/winpdo-nat.c, you'll see a SuspendThreads mechanism.
> The file header states 1996 as the latest copyright year, but they
> may have easilly failed to update it.  I'm not sure if win32-nat
> ever had this mechanism, or if it was only added by Apple.  Our
> cvs history only goes back till 1999.
>
> I did say: "I *think* that if...".  :-)
>

Sorry, I misunderstood "you'll see that once a similar method was used." 
as being an assertion.
Anyway there seems to be a reason not to SuspendThread, whether there 
was or wasn't such implementation in the past.

Leo.

Re: [win32] Fix suspend count handling

[Christopher Faylor]

On Fri, Nov 23, 2007 at 11:19:14AM +0100, Lerele wrote:
> Christopher Faylor escribi?:
>> On Wed, Nov 21, 2007 at 11:18:38PM +0000, Pedro Alves wrote:
>>   
>>> 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.  */
>>>     
>>
>> I don't see any reason to capitalize get_context in the comment.
>>
>>   
>>> 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;
>>>     
>>
>> This, and similar cases, needs to use a #define with an explanatory 
>> comment.
>>
>> With the above minor changes, this looks ok.
>>
>> 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 think they are needed.  They were anyway with the new gdbserver based
>(vs.  Win32 API based) interrupt code I sent several days ago, and that
>so very kindly Pedro prepared for commitment, but that I still haven't
>found the time to sit down and look at them (however I'm absolutely
>sure they're just fine), I guess his changes must be similar to what I
>sent in the first place.

*Why* did you think you needed them in gdbserver?  Did you actually try
not using them or did you just copy my code from win32-nat.c?

> Apart from this, there's also the case where (at least for gdbserver) 
> socket data is received asynchronously while the child is running. This 
> socket data could indicate gdbserver to set/enable/disable a breakpoint, 
> read thread registers, etc., and this kind of things may require to stop 
> the child using SuspendThread.
> Right?

I don't know exactly what you are referring to but if you are proposing
using SuspendThread/ResumeThread to work around non UNIX-like handling
of signals, I've previously mentioned that the use of SuspendThread is
dangerous since, the last I checked, Windows wasn't very good about not
avoiding reentracy problems if you suspend a thread and then use another
funcion from the Windows API.  This used to cause strange random
behavior and even blue screens in older versions of Windows NT.  I guess
it's possible that it's better these days but there is no way to really
know for sure.

cgf

Re: [win32] Fix suspend count handling

[Lerele]

Christopher Faylor escribió:
> On Fri, Nov 23, 2007 at 11:19:14AM +0100, Lerele wrote:
>   
>> Christopher Faylor escribi?:
>>     
>>> On Wed, Nov 21, 2007 at 11:18:38PM +0000, Pedro Alves wrote:
>>>   
>>>       
>>>> 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.  */
>>>>     
>>>>         
>>> I don't see any reason to capitalize get_context in the comment.
>>>
>>>   
>>>       
>>>> 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;
>>>>     
>>>>         
>>> This, and similar cases, needs to use a #define with an explanatory 
>>> comment.
>>>
>>> With the above minor changes, this looks ok.
>>>
>>> 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 think they are needed.  They were anyway with the new gdbserver based
>> (vs.  Win32 API based) interrupt code I sent several days ago, and that
>> so very kindly Pedro prepared for commitment, but that I still haven't
>> found the time to sit down and look at them (however I'm absolutely
>> sure they're just fine), I guess his changes must be similar to what I
>> sent in the first place.
>>     
>
> *Why* did you think you needed them in gdbserver?  Did you actually try
> not using them or did you just copy my code from win32-nat.c?
>
>   

I thought it natural to implement win32 functionality into gdbserver not 
starting from scratch, but from an already working implementation, such 
as win32-nat.c, however *truly* one drawback of this is that original 
code bugs got inherited into new implementation (such as the subject in 
question), and the porting process however was not really as 
straightforward as copy/paste; certain amount of work was involved (a 
lot of debugging gdbserver and gdb themselves), however I did not stop 
to think that SuspendThread was not actually needed, as it was already 
there.

I like your previous statement "I wasn't entirely sure that gdb needed 
to do anything like this but I erred on the side of caution.", except 
for the "erred" part.
I think its not bad to, in order to access thread data, lower memory, 
etc., first make sure it is actually paused, for whatever operation you 
want to do with it, as to not rely on a debug exception being received, 
or gdbserver itself stopping the low.  Also you may even need this 
functionality for any future new requirement.

If it works without problems and it's safer and more flexible, I'd just 
leave it. :-)



>> Apart from this, there's also the case where (at least for gdbserver) 
>> socket data is received asynchronously while the child is running. This 
>> socket data could indicate gdbserver to set/enable/disable a breakpoint, 
>> read thread registers, etc., and this kind of things may require to stop 
>> the child using SuspendThread.
>> Right?
>>     
>
> I don't know exactly what you are referring to but if you are proposing
> using SuspendThread/ResumeThread to work around non UNIX-like handling
> of signals, I've previously mentioned that the use of SuspendThread is
> dangerous since, the last I checked, Windows wasn't very good about not
> avoiding reentracy problems if you suspend a thread and then use another
> funcion from the Windows API.  This used to cause strange random
> behavior and even blue screens in older versions of Windows NT.  I guess
> it's possible that it's better these days but there is no way to really
> know for sure.
>
> cgf
>
>   


So then that's the reason.
Yes, I was referring to what you understood.

I do use win32 gdbserver quite often, and I have not encountered *any* 
kind of problem yet, however I do have to say I do use XP almost 
exclusively to run gdbserver.
If there is a problem with some other version of Windows, maybe you're 
right and SuspendThread should be removed after all.
It is a dilemma.

However, I guess you may have stumbled upon the fact that say for 
example msvcmon inferior *pausing* does actually work quite well, no 
matter what Windows version it's running on.  It's not that I want to 
start a discussion for mentioning that specific debugger nor writing ms 
here, but my opinion is that I think people would expect that kind of 
working from gdb/gdbserver on windows.  I would anyway.  This paragraph 
makes me ask, maybe the problems you talk about were due to some other 
reason, and not just about using SuspendThread only? Or are you 100% 
sure it's because of that specific function?

Leo.

Re: [win32] Fix suspend count handling

[Pedro Alves]

Lerele wrote:

 > This paragraph
> makes me ask, maybe the problems you talk about were due to some other 
> reason, and not just about using SuspendThread only? Or are you 100% 
> sure it's because of that specific function?
> 

Its not the function per se that's the problem.  It's that when we
suspend a thread it may be holding some internal lock, or
some such.  Calling the same function or another function that accesses
the same internal resources from another thread may violate what
was expected internally.  My experience on porting gdbserver to WinCE,
showed me that on WinCE at least, SuspendThread will not immediatelly
suspend the thread -- that probably means that the OS only stops the
threads on safe points.  That a look here [1].  It may be that
earlier versions of Windows had bugs that made that unsafe.
(Windows CE source code is available, but I won't touch it with a
ten-meter [2] pole.)


[1]
http://sourceware.org/ml/gdb-patches/2007-11/msg00220.html
-- Yes not perfect, I know.

[2]
I think metric.

-- 
Pedro Alves

Re: [win32] Fix suspend count handling

[Lerele]

Pedro Alves escribió:
> Lerele wrote:
>
> > This paragraph
>> makes me ask, maybe the problems you talk about were due to some 
>> other reason, and not just about using SuspendThread only? Or are you 
>> 100% sure it's because of that specific function?
>>
>
> Its not the function per se that's the problem.  It's that when we
> suspend a thread it may be holding some internal lock, or
> some such.  Calling the same function or another function that accesses
> the same internal resources from another thread may violate what
> was expected internally.  My experience on porting gdbserver to WinCE,
> showed me that on WinCE at least, SuspendThread will not immediatelly
> suspend the thread -- that probably means that the OS only stops the
> threads on safe points.  That a look here [1].  It may be that
> earlier versions of Windows had bugs that made that unsafe.
> (Windows CE source code is available, but I won't touch it with a
> ten-meter [2] pole.)
>
>
> [1]
> http://sourceware.org/ml/gdb-patches/2007-11/msg00220.html
> -- Yes not perfect, I know.
>
> [2]
> I think metric.
>

It's actually hard to believe those functions do not work as expected, 
especially because MSDN docs specifically state using SuspendThread 
before GetThreadContext, asuming the latter should work after the 
former, but of course anything is possible.

If that is true, it's useless of course to use them functions.

Re: [win32] Fix suspend count handling

[Christopher Faylor]

On Sat, Nov 24, 2007 at 06:50:39PM +0100, Lerele wrote:
> Pedro Alves escribi?:
>> Lerele wrote:
>>
>> > This paragraph
>>> makes me ask, maybe the problems you talk about were due to some other 
>>> reason, and not just about using SuspendThread only? Or are you 100% sure 
>>> it's because of that specific function?
>>>
>>
>> Its not the function per se that's the problem.  It's that when we
>> suspend a thread it may be holding some internal lock, or
>> some such.  Calling the same function or another function that accesses
>> the same internal resources from another thread may violate what
>> was expected internally.  My experience on porting gdbserver to WinCE,
>> showed me that on WinCE at least, SuspendThread will not immediatelly
>> suspend the thread -- that probably means that the OS only stops the
>> threads on safe points.  That a look here [1].  It may be that
>> earlier versions of Windows had bugs that made that unsafe.
>> (Windows CE source code is available, but I won't touch it with a
>> ten-meter [2] pole.)
>>
>>
>> [1]
>> http://sourceware.org/ml/gdb-patches/2007-11/msg00220.html
>> -- Yes not perfect, I know.
>>
>> [2]
>> I think metric.
>>
>
> It's actually hard to believe those functions do not work as expected, 
> especially because MSDN docs specifically state using SuspendThread before 
> GetThreadContext, asuming the latter should work after the former, but of 
> course anything is possible.
>
> If that is true, it's useless of course to use them functions.

It's possible that GetThreadContext may wait for pending SuspendThread's.

cgf

Re: [win32] Fix suspend count handling

[Christopher Faylor]

On Sat, Nov 24, 2007 at 03:18:42PM +0100, Lerele wrote:
>>> Christopher Faylor escribi?:
>>>> 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 think they are needed.  They were anyway with the new gdbserver based
>>> (vs.  Win32 API based) interrupt code I sent several days ago, and that
>>> so very kindly Pedro prepared for commitment, but that I still haven't
>>> found the time to sit down and look at them (however I'm absolutely
>>> sure they're just fine), I guess his changes must be similar to what I
>>> sent in the first place.
>>>     
>>
>> *Why* did you think you needed them in gdbserver?  Did you actually try
>> not using them or did you just copy my code from win32-nat.c?
>
> I thought it natural to implement win32 functionality into gdbserver not 
> starting from scratch, but from an already working implementation, such as 
> win32-nat.c, however *truly* one drawback of this is that original code 
> bugs got inherited into new implementation (such as the subject in 
> question), and the porting process however was not really as 
> straightforward as copy/paste; certain amount of work was involved (a lot 
> of debugging gdbserver and gdb themselves), however I did not stop to think 
> that SuspendThread was not actually needed, as it was already there.

So, in answer to my question about whether something was needed or not,
you asserted that it wasn't needed because you copied the code that I
wrote which was the source of my question.  Sorry but that doesn't really
answer my question.

If Windows already does the SuspendThread on all threads on debug event
then there is no reason to also do it in gdb.  I was asking if anyone
had actually checked if that was the case since it has been quite some
time since I did any experiments.

cgf

Re: [win32] Fix suspend count handling

[Lerele]

Christopher Faylor escribió:
> On Sat, Nov 24, 2007 at 03:18:42PM +0100, Lerele wrote:
>   
>>>> Christopher Faylor escribi?:
>>>>         
>>>>> 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 think they are needed.  They were anyway with the new gdbserver based
>>>> (vs.  Win32 API based) interrupt code I sent several days ago, and that
>>>> so very kindly Pedro prepared for commitment, but that I still haven't
>>>> found the time to sit down and look at them (however I'm absolutely
>>>> sure they're just fine), I guess his changes must be similar to what I
>>>> sent in the first place.
>>>>     
>>>>         
>>> *Why* did you think you needed them in gdbserver?  Did you actually try
>>> not using them or did you just copy my code from win32-nat.c?
>>>       
>> I thought it natural to implement win32 functionality into gdbserver not 
>> starting from scratch, but from an already working implementation, such as 
>> win32-nat.c, however *truly* one drawback of this is that original code 
>> bugs got inherited into new implementation (such as the subject in 
>> question), and the porting process however was not really as 
>> straightforward as copy/paste; certain amount of work was involved (a lot 
>> of debugging gdbserver and gdb themselves), however I did not stop to think 
>> that SuspendThread was not actually needed, as it was already there.
>>     
>
> So, in answer to my question about whether something was needed or not,
> you asserted that it wasn't needed because you copied the code that I
> wrote which was the source of my question.  Sorry but that doesn't really
> answer my question.
>
>   
Sure I did, however the situation seems a bit more complicated.  I'll 
explain below.




> If Windows already does the SuspendThread on all threads on debug event
> then there is no reason to also do it in gdb.  I was asking if anyone
> had actually checked if that was the case since it has been quite some
> time since I did any experiments.
>
>   


What I said basically is that it is safer to leave the SuspendThread 
there as, besides being safer, it may be required for any future 
enhancement.
Also, upon reception of a Win32 debug event, calling SuspendThread 
should not do any harm, as the threads are already paused.
It is also the basis of the new interrupt method, which is SuspendThread 
based.


In my previous message, in the paragraph I wrote:

"Apart from this, there's also the case where (at least for gdbserver) 
socket data is received asynchronously while the child is running. This 
socket data could indicate gdbserver to set/enable/disable a breakpoint, 
read thread registers, etc., and this kind of things may require to stop 
the child using SuspendThread.
Right?"

I was refering a specific gdbserver functionality that for some reason 
is not currently working for current cvs gdbserver.
In fact right now I do not actually know for sure if this functionality 
ever worked for cvs gdb and got broken, or it is based on local 
modifications I have made to gdbserver.

Please let me clarify:

I have a gdbserver.exe version here that lets you set breakpoints into 
the child while the child is running (without having to interrupt the 
child --if you can, set breakpoint and continue the child).
For this functionality to work socket data availability check must be 
done somewhere in the WaitForDebugEvent loop/call trace.
I think however that what I did was have get_child_debug_event return a 
"spurious" signal periodically to have gdbserver check for this socket 
data and act accordingly to this received data.
The really important thing is that in this case, no Win32 debug event 
was received (and the child was still running), so when gdbserver wanted 
to access the thread, the thread_rec function was ultimately called, 
which itself calls SuspendThread.
So in this case calling SuspendThread *is* necessary.

Now as to the reason why this is not working, I think it's pointless to 
figure why I have a local gdbserver that asynchrounously lets you set 
breakpoints, or try figure out if some older official gdbserver version 
ever had this support.

It is not currently working, and I think it is a very nice feature, not 
only because it actually is useful, but also because people used to 
other debugging systems (such as msvcmon) are used to this.

I think it's best to have it working upstream, and for this, 
SuspendThread is mandatory.
What do you think?

Leo.

Re: [win32] Fix suspend count handling

[Christopher Faylor]

On Sun, Nov 25, 2007 at 03:44:31PM +0100, Lerele wrote:
> What do you think?

There is some code in win32-nat.c which was a result of my uncertainty
about the Windows debugging API.  I thought that since we have a couple
more eyes on this now someone might know a bit more about how this
works.  Understanding the foundations is never a bad idea.

I'm not interested in gdbserver or what you think may be happening in
the future.  If the SuspendThread/ResumeThread code can be eliminated
from win32-nat.c along with all of the bookkeeping that is required to
avoid races then it may be a good idea to do so.  Whether it is a good
idea in light of future enhancements is a decision I can make but,
personally, I rarely see a good reason to keep code complication around
for the future unless someone is actually planning to do the work.

We can stop talking about this now since it is apparent that no one
actually knows the answer to my question.

cgf

Re: [win32] Fix suspend count handling

[Pedro Alves]

Christopher Faylor wrote:
> On Sun, Nov 25, 2007 at 03:44:31PM +0100, Lerele wrote:
>> What do you think?
> 
> There is some code in win32-nat.c which was a result of my uncertainty
> about the Windows debugging API.  I thought that since we have a couple
> more eyes on this now someone might know a bit more about how this
> works.  Understanding the foundations is never a bad idea.
> 
> I'm not interested in gdbserver or what you think may be happening in
> the future.  If the SuspendThread/ResumeThread code can be eliminated
> from win32-nat.c along with all of the bookkeeping that is required to
> avoid races then it may be a good idea to do so.  Whether it is a good
> idea in light of future enhancements is a decision I can make but,
> personally, I rarely see a good reason to keep code complication around
> for the future unless someone is actually planning to do the work.
> 
> We can stop talking about this now since it is apparent that no one
> actually knows the answer to my question.
> 

100% Agreed.

Just to let you know that I plan on looking at this.
I'm specifically wanting to look at what we're doing
when/if we need to step over something with all
threads but the current stopped -- if that is supposed to
happen, it looks like we're broken, we should be
suspending all the threads but the current before
resuming.

-- 
Pedro Alves

Re: [win32] Fix suspend count handling

[Christopher Faylor]

On Sun, Nov 25, 2007 at 06:56:04PM +0000, Pedro Alves wrote:
> Christopher Faylor wrote:
>> On Sun, Nov 25, 2007 at 03:44:31PM +0100, Lerele wrote:
>>> What do you think?
>> There is some code in win32-nat.c which was a result of my uncertainty
>> about the Windows debugging API.  I thought that since we have a couple
>> more eyes on this now someone might know a bit more about how this
>> works.  Understanding the foundations is never a bad idea.
>> I'm not interested in gdbserver or what you think may be happening in
>> the future.  If the SuspendThread/ResumeThread code can be eliminated
>> from win32-nat.c along with all of the bookkeeping that is required to
>> avoid races then it may be a good idea to do so.  Whether it is a good
>> idea in light of future enhancements is a decision I can make but,
>> personally, I rarely see a good reason to keep code complication around
>> for the future unless someone is actually planning to do the work.
>> We can stop talking about this now since it is apparent that no one
>> actually knows the answer to my question.
>
> 100% Agreed.
>
> Just to let you know that I plan on looking at this.
> I'm specifically wanting to look at what we're doing
> when/if we need to step over something with all
> threads but the current stopped -- if that is supposed to
> happen, it looks like we're broken, we should be
> suspending all the threads but the current before
> resuming.

I was going to try just temporarily doing this to see what happens:

#define SuspendThread(hThread) 1
#define ResumeThread(hThread) 1

cgf

Re: [win32] Fix suspend count handling

[Lerele]

Christopher Faylor escribió:
> On Sun, Nov 25, 2007 at 06:56:04PM +0000, Pedro Alves wrote:
>   
>> Christopher Faylor wrote:
>>     
>>> On Sun, Nov 25, 2007 at 03:44:31PM +0100, Lerele wrote:
>>>       
>>>> What do you think?
>>>>         
>>> There is some code in win32-nat.c which was a result of my uncertainty
>>> about the Windows debugging API.  I thought that since we have a couple
>>> more eyes on this now someone might know a bit more about how this
>>> works.  Understanding the foundations is never a bad idea.
>>> I'm not interested in gdbserver or what you think may be happening in
>>> the future.  If the SuspendThread/ResumeThread code can be eliminated
>>> from win32-nat.c along with all of the bookkeeping that is required to
>>> avoid races then it may be a good idea to do so.  Whether it is a good
>>> idea in light of future enhancements is a decision I can make but,
>>> personally, I rarely see a good reason to keep code complication around
>>> for the future unless someone is actually planning to do the work.
>>> We can stop talking about this now since it is apparent that no one
>>> actually knows the answer to my question.
>>>       
>> 100% Agreed.
>>
>> Just to let you know that I plan on looking at this.
>> I'm specifically wanting to look at what we're doing
>> when/if we need to step over something with all
>> threads but the current stopped -- if that is supposed to
>> happen, it looks like we're broken, we should be
>> suspending all the threads but the current before
>> resuming.
>>     
>
> I was going to try just temporarily doing this to see what happens:
>
> #define SuspendThread(hThread) 1
> #define ResumeThread(hThread) 1
>
> cgf
>
>   

I did remove SuspendThread from gdbserver, before my last reply.
Did have to remove SuspendThreadInterrupt support too.
Worked Ok.
I guess win32-nat.c should be similar.

Re: [win32] Fix suspend count handling

[Lerele]

Christopher Faylor escribió:
> On Sun, Nov 25, 2007 at 03:44:31PM +0100, Lerele wrote:
>   
>> What do you think?
>>     
>
> There is some code in win32-nat.c which was a result of my uncertainty
> about the Windows debugging API.  I thought that since we have a couple
> more eyes on this now someone might know a bit more about how this
> works.  Understanding the foundations is never a bad idea.
>
> I'm not interested in gdbserver or what you think may be happening in
> the future.  If the SuspendThread/ResumeThread code can be eliminated
> from win32-nat.c along with all of the bookkeeping that is required to
> avoid races then it may be a good idea to do so.  Whether it is a good
> idea in light of future enhancements is a decision I can make but,
> personally, I rarely see a good reason to keep code complication around
> for the future unless someone is actually planning to do the work.
>
> We can stop talking about this now since it is apparent that no one
> actually knows the answer to my question.
>
> cgf
>
>   

Okay.
Derived from my previous messages, which by the way I do not really know 
if you read/undestood, is that for the third time I'm saying 
SuspendThread I think is safer (as you did think once) and does not 
really hurt, and it's not such a complication keeping it around, in my 
opinion.
The only problem I see is that a small bug has been detected, that's all.

If win32-nat.c is going to give control back to gdb in response to a 
Win32 debug event only, it's obvious SuspendThread is not needed.
When WaitForDebugEvent returns an event, all child threads are stopped, 
so yes, SuspendThread is *not* needed.
Is this the short answer you were looking for???

Also, may you be interested in the fact that currently interrupting a 
running program using gdb 6.7 does *not* seem to work?  Either if you 
run gdb alone (not gdbserver) or using a front-end?
To my point, as gdbserver win32 code is win32-nat.c based, funtionality 
is very similar.  One such functionality could be the latest 
SuspendThread interruption that has been added to gdbserver (but not to 
win32-nat.c).  As such, could you be interested in this for win32-nat.c?
If this functionality is of interest, then that's another reason to keep 
thread_rec/SuspendThread.

Re: [win32] Fix suspend count handling

[Pedro Alves]

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

Re: [win32] Fix suspend count handling

[Christopher Faylor]

On Sat, Nov 24, 2007 at 12:16:21PM +0000, Pedro Alves wrote:
> Added a #define and checked in (see attached).

Could you add a comment too at some point?  It's not clear to me
what that constant does.

cgf