From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12036 invoked by alias); 21 Nov 2007 00:35:18 -0000 Received: (qmail 12024 invoked by uid 22791); 21 Nov 2007 00:35:17 -0000 X-Spam-Check-By: sourceware.org Received: from ug-out-1314.google.com (HELO ug-out-1314.google.com) (66.249.92.174) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 21 Nov 2007 00:35:06 +0000 Received: by ug-out-1314.google.com with SMTP id o2so1719822uge for ; Tue, 20 Nov 2007 16:35:03 -0800 (PST) Received: by 10.66.242.5 with SMTP id p5mr1206214ugh.1195605303193; Tue, 20 Nov 2007 16:35:03 -0800 (PST) Received: from ?88.210.75.97? ( [88.210.75.97]) by mx.google.com with ESMTPS id i39sm2307454ugd.2007.11.20.16.35.01 (version=TLSv1/SSLv3 cipher=RC4-MD5); Tue, 20 Nov 2007 16:35:02 -0800 (PST) Message-ID: <47437D3A.3000107@portugalmail.pt> Date: Wed, 21 Nov 2007 00:35:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; pt-BR; rv:1.8.1.9) Gecko/20071031 Thunderbird/2.0.0.9 Mnenhy/0.7.5.0 MIME-Version: 1.0 To: gdb-patches@sourceware.org Subject: [win32] Fix suspend count handling Content-Type: multipart/mixed; boundary="------------060707000905070004070904" 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/msg00389.txt.bz2 This is a multi-part message in MIME format. --------------060707000905070004070904 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-length: 2945 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 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 --------------060707000905070004070904 Content-Type: text/x-diff; name="nat_suspend_count_fix.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="nat_suspend_count_fix.diff" Content-length: 2936 2007-11-21 Pedro Alves * 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 */ --------------060707000905070004070904--