From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6969 invoked by alias); 24 Nov 2007 12:16:54 -0000 Received: (qmail 6960 invoked by uid 22791); 24 Nov 2007 12:16:52 -0000 X-Spam-Check-By: sourceware.org Received: from nf-out-0910.google.com (HELO nf-out-0910.google.com) (64.233.182.188) by sourceware.org (qpsmtpd/0.31) with ESMTP; Sat, 24 Nov 2007 12:16:42 +0000 Received: by nf-out-0910.google.com with SMTP id b11so96281nfh for ; Sat, 24 Nov 2007 04:16:39 -0800 (PST) Received: by 10.78.149.15 with SMTP id w15mr208802hud.1195906598373; Sat, 24 Nov 2007 04:16:38 -0800 (PST) Received: from ?88.210.72.146? ( [88.210.72.146]) by mx.google.com with ESMTPS id c9sm3008938nfi.2007.11.24.04.16.36 (version=TLSv1/SSLv3 cipher=RC4-MD5); Sat, 24 Nov 2007 04:16:37 -0800 (PST) Message-ID: <47481615.1070307@portugalmail.pt> Date: Sat, 24 Nov 2007 12:16: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: Re: [win32] Fix suspend count handling References: <47437D3A.3000107@portugalmail.pt> <000001c82c31$4a57b220$df071660$@u-strasbg.fr> <4053daab0711210543w4b241e1ek2371e887f3c4f7d2@mail.gmail.com> <000401c82c48$a450df10$ecf29d30$@u-strasbg.fr> <4053daab0711210708o607018b9n8b63147a8498a207@mail.gmail.com> <4053daab0711211019r15f3a862g677080b65b4d8e71@mail.gmail.com> <4744BCCE.60705@portugalmail.pt> <20071123010744.GA31180@ednor.casa.cgf.cx> In-Reply-To: <20071123010744.GA31180@ednor.casa.cgf.cx> Content-Type: multipart/mixed; boundary="------------010208000109030100060300" 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/msg00447.txt.bz2 This is a multi-part message in MIME format. --------------010208000109030100060300 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Content-length: 2483 Christopher Faylor wrote: >> -/* 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. */ > > I don't see any reason to capitalize get_context in the comment. > That's what the GNU coding standards prefer: [5.2 Commenting Your Work] "The comment on a function is much clearer if you use the argument names to speak about the argument values. The variable name itself should be lower case, but write it in upper case when you are speaking about the value rather than the variable itself. Thus, “the node number NODE_NUM” rather than “an inode”." In fact, I shouldn't be capitalizing ID. Fixed that. >> @@ -294,8 +304,7 @@ win32_add_thread (DWORD id, HANDLE h) >> th->context.Dr1 = dr[1]; >> th->context.Dr2 = dr[2]; >> th->context.Dr3 = dr[3]; >> - /* th->context.Dr6 = dr[6]; >> - FIXME: should we set dr6 also ?? */ >> + th->context.Dr6 = 0xffff0ff0; > > This, and similar cases, needs to use a #define with an explanatory comment. > > With the above minor changes, this looks ok. > Added a #define and checked in (see attached). > I have to ask, however, if the SuspendThread's are even needed at all. > When I was writing this code, I wasn't entirely sure that gdb needed to > do anything like this but I erred on the side of caution. So, I'm > wondering if things would still work ok if the > SuspendThread/ResumeThread stuff was gone. > I wonder that too. The docs say that we must not retrieve a thread context while the thread is running. When the inferior is stopped due to a debug event the whole process is frozen, so in that case it should safe. I can give it a try later on, but it may take me a while to confirm if it really is safe, especially because I currently only have access to Windows XP SP2 and WinCE (from the archives, I get the impression Win2000 is pickier.) OTOH, we'll still need to suspend threads if we want to implement support for debugging one thread while the others are running, or to step just one thread while the others are stopped, or if we want to support stopping the inferior without relying on a console available to handle GenerateConsoleCtrlEvent; but these are different stories. -- Pedro Alves --------------010208000109030100060300 Content-Type: text/x-diff; name="suspend_count_fix_plus_watchpoints.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="suspend_count_fix_plus_watchpoints.diff" Content-length: 5503 2007-11-24 Pedro Alves * win32-nat.c (DR6_CLEAR_VALUE): New define. (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_add_thread): Set Dr6 to DR6_CLEAR_VALUE. (win32_continue): Set Dr6 to DR6_CLEAR_VALUE. Update usage of the `suspended' flag. Do ContinueDebugEvent after resuming the suspended threads, not before. Set threads' contexts before resuming them, not after. (win32_resume): Set Dr6 to DR6_CLEAR_VALUE. --- gdb/win32-nat.c | 80 +++++++++++++++++++++++++++++++------------------------- 1 file changed, 45 insertions(+), 35 deletions(-) Index: src/gdb/win32-nat.c =================================================================== --- src.orig/gdb/win32-nat.c 2007-11-24 11:44:14.000000000 +0000 +++ src/gdb/win32-nat.c 2007-11-24 11:44:56.000000000 +0000 @@ -91,6 +91,7 @@ enum static unsigned dr[8]; static int debug_registers_changed; static int debug_registers_used; +#define DR6_CLEAR_VALUE 0xffff0ff0 /* The string sent by cygwin when it processes a signal. FIXME: This should be in a cygwin include file. */ @@ -112,14 +113,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 +245,9 @@ 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. 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; @@ -294,8 +304,7 @@ win32_add_thread (DWORD id, HANDLE h) th->context.Dr1 = dr[1]; th->context.Dr2 = dr[2]; th->context.Dr3 = dr[3]; - /* th->context.Dr6 = dr[6]; - FIXME: should we set dr6 also ?? */ + th->context.Dr6 = DR6_CLEAR_VALUE; th->context.Dr7 = dr[7]; CHECK (SetThreadContext (th->h, &th->context)); th->context.ContextFlags = 0; @@ -1122,32 +1131,34 @@ win32_continue (DWORD continue_status, i current_event.dwProcessId, current_event.dwThreadId, continue_status == DBG_CONTINUE ? "DBG_CONTINUE" : "DBG_EXCEPTION_NOT_HANDLED")); + + for (th = &thread_head; (th = th->next) != NULL;) + if ((id == -1 || id == (int) th->id) + && th->suspended) + { + if (debug_registers_changed) + { + th->context.ContextFlags |= CONTEXT_DEBUG_REGISTERS; + th->context.Dr0 = dr[0]; + th->context.Dr1 = dr[1]; + th->context.Dr2 = dr[2]; + th->context.Dr3 = dr[3]; + th->context.Dr6 = DR6_CLEAR_VALUE; + th->context.Dr7 = dr[7]; + } + if (th->context.ContextFlags) + { + CHECK (SetThreadContext (th->h, &th->context)); + th->context.ContextFlags = 0; + } + if (th->suspended > 0) + (void) ResumeThread (th->h); + th->suspended = 0; + } + res = ContinueDebugEvent (current_event.dwProcessId, current_event.dwThreadId, continue_status); - if (res) - for (th = &thread_head; (th = th->next) != NULL;) - if (((id == -1) || (id == (int) th->id)) && th->suspend_count) - { - - for (i = 0; i < th->suspend_count; i++) - (void) ResumeThread (th->h); - th->suspend_count = 0; - if (debug_registers_changed) - { - /* Only change the value of the debug registers */ - th->context.ContextFlags = CONTEXT_DEBUG_REGISTERS; - th->context.Dr0 = dr[0]; - th->context.Dr1 = dr[1]; - th->context.Dr2 = dr[2]; - th->context.Dr3 = dr[3]; - /* th->context.Dr6 = dr[6]; - FIXME: should we set dr6 also ?? */ - th->context.Dr7 = dr[7]; - CHECK (SetThreadContext (th->h, &th->context)); - th->context.ContextFlags = 0; - } - } debug_registers_changed = 0; return res; @@ -1233,8 +1244,7 @@ win32_resume (ptid_t ptid, int step, enu th->context.Dr1 = dr[1]; th->context.Dr2 = dr[2]; th->context.Dr3 = dr[3]; - /* th->context.Dr6 = dr[6]; - FIXME: should we set dr6 also ?? */ + th->context.Dr6 = DR6_CLEAR_VALUE; th->context.Dr7 = dr[7]; } CHECK (SetThreadContext (th->h, &th->context)); --------------010208000109030100060300--