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


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

RE: [win32] Fix suspend count handling


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




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