From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 24905 invoked by alias); 12 Nov 2007 02:07:06 -0000 Received: (qmail 24648 invoked by uid 22791); 12 Nov 2007 02:07:03 -0000 X-Spam-Check-By: sourceware.org Received: from nf-out-0910.google.com (HELO nf-out-0910.google.com) (64.233.182.190) by sourceware.org (qpsmtpd/0.31) with ESMTP; Mon, 12 Nov 2007 02:07:01 +0000 Received: by nf-out-0910.google.com with SMTP id b11so888416nfh for ; Sun, 11 Nov 2007 18:06:58 -0800 (PST) Received: by 10.86.54.3 with SMTP id c3mr4083521fga.1194833217991; Sun, 11 Nov 2007 18:06:57 -0800 (PST) Received: from ?192.168.0.4? ( [62.169.107.97]) by mx.google.com with ESMTPS id h1sm5824116nfh.2007.11.11.18.06.54 (version=TLSv1/SSLv3 cipher=RC4-MD5); Sun, 11 Nov 2007 18:06:56 -0800 (PST) Message-ID: <4737B543.80607@portugalmail.pt> Date: Mon, 12 Nov 2007 02:07:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; pt-BR; rv:1.8.1.6) Gecko/20070728 Thunderbird/2.0.0.6 Mnenhy/0.7.5.0 MIME-Version: 1.0 To: gdb-patches@sourceware.org, Lerele Subject: [gdbserver/win32] (3/11) Fix suspend count handling Content-Type: multipart/mixed; boundary="------------000701070508020807030509" X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2007-11/txt/msg00216.txt.bz2 This is a multi-part message in MIME format. --------------000701070508020807030509 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-length: 2259 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 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 --------------000701070508020807030509 Content-Type: text/x-diff; name="suspend_count_fix.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="suspend_count_fix.diff" Content-length: 3163 2007-11-12 Leo Zayas Pedro Alves * 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, ¤t_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; --------------000701070508020807030509--