This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
[win32] Fix suspend count handling
- From: Pedro Alves <pedro_alves at portugalmail dot pt>
- To: gdb-patches at sourceware dot org
- Date: Wed, 21 Nov 2007 00:35:06 +0000
- 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
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 */