From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16561 invoked by alias); 21 Nov 2007 23:33:35 -0000 Received: (qmail 16539 invoked by uid 22791); 21 Nov 2007 23:33:30 -0000 X-Spam-Check-By: sourceware.org Received: from ug-out-1314.google.com (HELO ug-out-1314.google.com) (66.249.92.171) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 21 Nov 2007 23:33:21 +0000 Received: by ug-out-1314.google.com with SMTP id o2so269163uge for ; Wed, 21 Nov 2007 15:33:18 -0800 (PST) Received: by 10.67.88.10 with SMTP id q10mr1131876ugl.1195687998114; Wed, 21 Nov 2007 15:33:18 -0800 (PST) Received: from ?88.210.76.67? ( [88.210.76.67]) by mx.google.com with ESMTPS id 28sm1600002ugs.2007.11.21.15.33.15 (version=TLSv1/SSLv3 cipher=RC4-MD5); Wed, 21 Nov 2007 15:33:17 -0800 (PST) Message-ID: <4744BCCE.60705@portugalmail.pt> Date: Wed, 21 Nov 2007 23:33: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: Pierre Muller CC: 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> In-Reply-To: <4053daab0711211019r15f3a862g677080b65b4d8e71@mail.gmail.com> Content-Type: multipart/mixed; boundary="------------060207030604080606020201" 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/msg00402.txt.bz2 This is a multi-part message in MIME format. --------------060207030604080606020201 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-length: 3620 Pedro Alves wrote: > Pedro Alves wrote: >> On Nov 21, 2007 2:13 PM, Pierre Muller wrote: >> That's not what I see here. Can you show me a run where you get 4 >> only this patch applied? >> > > I did try that, but posted a log of not doing it :-). > > I've just tried about 30 times, and only once I did see > a 4 coming out ... oh, well, one of those things. > OK. Back at my home laptop, I can reproduce that with no problems. Let me clarify what the 4 problem really is. It's a race between gdb and the inferior. Take this slightly changed test case. The only difference to the original version is the extra Sleep call. #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; } Sleep (300); 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; } If you do the "break at ...", "run", "thread 3", "continue" sequence, and "..." is the "Sleep" line, you'll get 3, but if you put the break at the /* set breakpoint here */ line, you'll get 4 (if you're (un)lucky). The race happens due to the fact that gdb is doing something similar to this: win32_continue() { ContinueDebugEvent (...); /* Resumes all non suspended threads of the process. */ /* At this point, gdb is running concurrently with the inferior threads that were not suspended - which included the main thread of the testcase. */ foreach t in threads do if t is suspended ResumeThread t fi done } If you break at the Sleep call, when we resume, gdb will have a bit of time to call ResumeThread on the suspended thread of the testcase. If you instead break at the ResumeThread line, you'll have a good chance that the inferior wins the race, hence the "4" result (remember that ResumeThread returns the previous suspend count). If we put something like this after the ResumeThread call: (...) suspend_count = ResumeThread (h); /* set breakpoint here */ + Sleep (300); + SuspendThread (h); + suspend_count = ResumeThread (h); printf ("%lu\n", suspend_count); /* should be 3 */ (...) ... you'll see that eventually gdb will bring the suspend count back to 3. (A SuspendThread, ResumeThread pair is the way to get at the suspend count.) > Since the watchpoint patch should fix this, what shall I do? Shall I merge > the two and resubmit, or leave it at that ? They've already been tested > together without regressions. > Here is the merge from the patch I posted at the start of the thread with this patch: [win32] Fix watchpoint support http://sourceware.org/ml/gdb-patches/2007-11/msg00390.html This patch fixes both the suspend_count handling, and the watchpoint support. Thanks Pierre, for looking at it. OK ? -- Pedro Alves --------------060207030604080606020201 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: 5177 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_add_thread): Set Dr6 to 0xffff0ff0. (win32_resume): Likewise. (win32_continue): Set Dr6 to 0xffff0ff0. Update usage of the `suspended' flag. Do ContinueDebugEvent after resuming the suspended threads, not before. Set threads' contexts before resuming them, not after. --- 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-11 23:13:04.000000000 +0000 +++ src/gdb/win32-nat.c 2007-11-21 22:39:56.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; @@ -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; 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 = 0xffff0ff0; + 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 = 0xffff0ff0; th->context.Dr7 = dr[7]; } CHECK (SetThreadContext (th->h, &th->context)); --------------060207030604080606020201--