[gdbserver/win32] (3/11) Fix suspend count handling

[gdbserver/win32] (3/11) Fix suspend count handling

[Pedro Alves]

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

Hi,

As Leo reported earlier, the suspend_count handling breaks
applications that use SuspendThread on their own threads.
SuspendThread returns the previous suspend count of the
thread, and gdbserver is calling ResumeThread that many times.
If the Thread was already suspended, say, 3 times, gdbserver will
do one SuspendThread call (in thread_rec), and 4 ResumeThread
calls when resuming.

This patch fixes it by turning the suspend_count counter into
a suspended flag.

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 read "set breakpoint here", switch to the
other thread with "thread 2", effectivelly forcing a thread_rec
call, which calls SuspendThread, and sets suspend_count.  Since the
win32 support can't resume with a different thread, so go back to
thread 1, and step.  The suspend_count will be 0, instead of 3.

I have yet to turn this into a proper gdb test.  Is that wanted?  Where
shall I put it?  gdb.base?  gdb.thread?  other?

Note: The native (gdb/win32-nat.c) support also has the same bug.

Regtested on a local i686-pc-cygwin gdbserver, and against
a arm-wince remote.

Cheers,
Pedro Alves




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

2007-11-12  Leo Zayas  <lerele@champenstudios@com>
	    Pedro Alves  <pedro_alves@portugalmail.pt>

	* win32-low.h (win32_thread_info): Add descriptions to the
	structure members .  Replace `suspend_count' counter by a
	`suspended' flag.

	* win32-low.c (thread_rec): Update condition of when to get the
	context from the inferior.  Rely on ContextFlags being set if it
	has already been retrieved.  Only suspend the inferior thread if
	we haven't already.  Warn if that fails.
	(continue_one_thread): s/suspend_count/suspended/.  Only call
	ResumeThread once.  Warn if that fails.

---
 gdb/gdbserver/win32-low.c |   28 ++++++++++++++++++++--------
 gdb/gdbserver/win32-low.h |    9 ++++++++-
 2 files changed, 28 insertions(+), 9 deletions(-)

Index: src/gdb/gdbserver/win32-low.c
===================================================================
--- src.orig/gdb/gdbserver/win32-low.c	2007-11-04 23:50:36.000000000 +0000
+++ src/gdb/gdbserver/win32-low.c	2007-11-04 23:51:44.000000000 +0000
@@ -105,10 +105,19 @@ thread_rec (DWORD id, int get_context)
     return NULL;
 
   th = inferior_target_data (thread);
-  if (!th->suspend_count && get_context)
+  if (get_context && th->context.ContextFlags == 0)
     {
-      if (id != current_event.dwThreadId)
-	th->suspend_count = SuspendThread (th->h) + 1;
+      if (!th->suspended)
+	{
+	  if (SuspendThread (th->h) == (DWORD) -1)
+	    {
+	      DWORD err = GetLastError ();
+	      OUTMSG (("warning: SuspendThread failed in thread_rec, "
+		       "(error %d): %s\n", (int) err, strwinerror (err)));
+	    }
+	  else
+	    th->suspended = 1;
+	}
 
       (*the_low_target.get_thread_context) (th, &current_event);
     }
@@ -259,10 +268,9 @@ continue_one_thread (struct inferior_lis
   struct thread_info *thread = (struct thread_info *) this_thread;
   int thread_id = * (int *) id_ptr;
   win32_thread_info *th = inferior_target_data (thread);
-  int i;
 
   if ((thread_id == -1 || thread_id == th->tid)
-      && th->suspend_count)
+      && th->suspended)
     {
       if (th->context.ContextFlags)
 	{
@@ -270,9 +278,13 @@ continue_one_thread (struct inferior_lis
 	  th->context.ContextFlags = 0;
 	}
 
-      for (i = 0; i < th->suspend_count; i++)
-	(void) ResumeThread (th->h);
-      th->suspend_count = 0;
+      if (ResumeThread (th->h) == (DWORD) -1)
+	{
+	  DWORD err = GetLastError ();
+	  OUTMSG (("warning: ResumeThread failed in continue_one_thread, "
+		   "(error %d): %s\n", (int) err, strwinerror (err)));
+	}
+      th->suspended = 0;
     }
 
   return 0;
Index: src/gdb/gdbserver/win32-low.h
===================================================================
--- src.orig/gdb/gdbserver/win32-low.h	2007-11-04 23:50:36.000000000 +0000
+++ src/gdb/gdbserver/win32-low.h	2007-11-04 23:51:16.000000000 +0000
@@ -22,9 +22,16 @@
    each thread.  */
 typedef struct win32_thread_info
 {
+  /* The Win32 thread identifier.  */
   DWORD tid;
+
+  /* The handle to the thread.  */
   HANDLE h;
-  int suspend_count;
+
+  /* Non zero if SuspendThread was called on this thread.  */
+  int suspended;
+
+  /* The context of the thread.  */
   CONTEXT context;
 } win32_thread_info;
 

Re: [gdbserver/win32] (3/11) Fix suspend count handling

[Daniel Jacobowitz]

On Mon, Nov 12, 2007 at 02:06:59AM +0000, Pedro Alves wrote:
> I have yet to turn this into a proper gdb test.  Is that wanted?  Where
> shall I put it?  gdb.base?  gdb.thread?  other?

gdb.threads would be good, I think.

> 2007-11-12  Leo Zayas  <lerele@champenstudios@com>
> 	    Pedro Alves  <pedro_alves@portugalmail.pt>
> 
> 	* win32-low.h (win32_thread_info): Add descriptions to the
> 	structure members .  Replace `suspend_count' counter by a
> 	`suspended' flag.
> 
> 	* win32-low.c (thread_rec): Update condition of when to get the
> 	context from the inferior.  Rely on ContextFlags being set if it
> 	has already been retrieved.  Only suspend the inferior thread if
> 	we haven't already.  Warn if that fails.
> 	(continue_one_thread): s/suspend_count/suspended/.  Only call
> 	ResumeThread once.  Warn if that fails.

OK.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [gdbserver/win32] (3/11) Fix suspend count handling

[Lerele]

Just a couple small tiny details:

Not that it's super important, but in all the series of patches where 
both our names appear, I think my name should be listed second (if at 
all) in the changelog, as Pedro did the vast majority of the work for 
these patches, and merged with the one I sent.
What's fair, is fair, I think. :)

Also, is it necessary to include the eMail?
I'd prefer not to include mine if this is possible, if this can lower 
spam getting into my mail box.




Daniel Jacobowitz escribió:
> On Mon, Nov 12, 2007 at 02:06:59AM +0000, Pedro Alves wrote:
>   
>> I have yet to turn this into a proper gdb test.  Is that wanted?  Where
>> shall I put it?  gdb.base?  gdb.thread?  other?
>>     
>
> gdb.threads would be good, I think.
>
>   
>> 2007-11-12  Leo Zayas  <lerele@champenstudios@com>
>> 	    Pedro Alves  <pedro_alves@portugalmail.pt>
>>
>> 	* win32-low.h (win32_thread_info): Add descriptions to the
>> 	structure members .  Replace `suspend_count' counter by a
>> 	`suspended' flag.
>>
>> 	* win32-low.c (thread_rec): Update condition of when to get the
>> 	context from the inferior.  Rely on ContextFlags being set if it
>> 	has already been retrieved.  Only suspend the inferior thread if
>> 	we haven't already.  Warn if that fails.
>> 	(continue_one_thread): s/suspend_count/suspended/.  Only call
>> 	ResumeThread once.  Warn if that fails.
>>     
>
> OK.
>
>   

Re: [gdbserver/win32] (3/11) Fix suspend count handling

[Pedro Alves]

Lerele wrote:
> 
> Just a couple small tiny details:
> 
> Not that it's super important, but in all the series of patches where 
> both our names appear, I think my name should be listed second (if at 
> all) in the changelog, as Pedro did the vast majority of the work for 
> these patches, and merged with the one I sent.
> What's fair, is fair, I think. :)
> 

I don't think there's any written rule about the order
of the names, but, if were to write one, I'd say that the
chronological order in which people touched the patch
would be the most objective.  Saying who's done more than
who is very subjective, and honestly, I'm certainly not
losing my sleep over that.

> Also, is it necessary to include the eMail?

This will need an answer from someone who knows the
rules better than me.

It is costumary to post the raw email address on the body
of the patch submission messages, on the ChangeLog entry.
I'll try to remember not putting yours raw the next time
I submit a patch with your name on it.  Please understand
that you'd be the exception to the rule, so it is
susceptible to being forgotten or overlooked.

Looking over the ChangeLog files of gcc/binutils/gdb [1],
I see a few entries setting precedent of using mangled
addresses.

[1] grep "^[0-9]" ChangeLog*| grep -v "@"

> I'd prefer not to include mine if this is possible, if this can lower 
> spam getting into my mail box.
> 

If you're already being spammed, then I doubt this
can make a difference.

I'd like to move forward, so I've removed your email
address from the pending entries, making them like like
so for now:

2007-12-03  Leo Zayas
	    Pedro Alves  <pedro_alves@portugalmail.pt>

Changing it to some other form if needed can be easilly
done later.

Patch checked in.

-- 
Pedro Alves

Re: [gdbserver/win32] (3/11) Fix suspend count handling

[Pedro Alves]

Daniel Jacobowitz wrote:
> 
> gdb.threads would be good, I think.
> 

Will do that.

>> 2007-11-12  Leo Zayas
>> 	    Pedro Alves  <pedro_alves@portugalmail.pt>
>>
>> 	* win32-low.h (win32_thread_info): Add descriptions to the
>> 	structure members .  Replace `suspend_count' counter by a
>> 	`suspended' flag.
>>
>> 	* win32-low.c (thread_rec): Update condition of when to get the
>> 	context from the inferior.  Rely on ContextFlags being set if it
>> 	has already been retrieved.  Only suspend the inferior thread if
>> 	we haven't already.  Warn if that fails.
>> 	(continue_one_thread): s/suspend_count/suspended/.  Only call
>> 	ResumeThread once.  Warn if that fails.
> 
> OK.
> 

Thanks, checked in.

-- 
Pedro Alves