From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19555 invoked by alias); 23 Nov 2007 18:30:50 -0000 Received: (qmail 19547 invoked by uid 22791); 23 Nov 2007 18:30:49 -0000 X-Spam-Check-By: sourceware.org Received: from gw.sprintaddict.net (HELO champenstudios.com) (80.91.89.73) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 23 Nov 2007 18:30:36 +0000 Received: from [192.168.1.5] (164.Red-80-36-45.staticIP.rima-tde.net [80.36.45.164]) (authenticated bits=0) by champenstudios.com (8.13.8/8.13.8) with ESMTP id lANIN1wp025537 (version=TLSv1/SSLv3 cipher=DHE-DSS-AES256-SHA bits=256 verify=NO) for ; Fri, 23 Nov 2007 19:23:02 +0100 Message-ID: <47471C47.80909@champenstudios.com> Date: Fri, 23 Nov 2007 18:30:00 -0000 From: Lerele User-Agent: Thunderbird 2.0.0.6 (Windows/20070728) 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> <4746A922.30404@champenstudios.com> In-Reply-To: <4746A922.30404@champenstudios.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8bit 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/msg00436.txt.bz2 Lerele escribió: > Christopher Faylor escribió: >> On Wed, Nov 21, 2007 at 11:18:38PM +0000, Pedro Alves wrote: >> >>> 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 >>> >> >> >>> 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. */ >>> >> >> I don't see any reason to capitalize get_context in the comment. >> >> >>> 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; >>> >> >> This, and similar cases, needs to use a #define with an explanatory >> comment. >> >> With the above minor changes, this looks ok. >> >> 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. >> >> cgf >> >> > > I think they are needed. They were anyway with the new gdbserver based > (vs. Win32 API based) interrupt code I sent several days ago, and that > so very kindly Pedro prepared for commitment, but that I still haven't > found the time to sit down and look at them (however I'm absolutely > sure they're just fine), I guess his changes must be similar to what I > sent in the first place. > Apart from this, there's also the case where (at least for gdbserver) > socket data is received asynchronously while the child is running. > This socket data could indicate gdbserver to set/enable/disable a > breakpoint, read thread registers, etc., and this kind of things may > require to stop the child using SuspendThread. > Right? > > Leo. > > I'd also like to ask you a question, concerning a comment from Pedro several messages back that has stayed around in my mind since then. It's not related with this specific thread title, but since it's gdbserver/win32 related, I haven't found appropriate to open a new thread just for this simple question. The issue is near the end of: http://sourceware.org/ml/gdb-patches/2007-11/msg00041.html It's about the fact of gdb win32-nat.c some time ago having the interrupt functionality similar to the one that has been recently implemented using SuspendThread (versus using DebugBreak kind of functions). Pedro commented back then that win32-nat.c did have sometime in the past a similar implementation [that must have been dropped]. Do you know/remember if it was dropped for a specific reason? My concern is about any possible drawback for this new interrupt functionality method. Regards, Leo.