From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30137 invoked by alias); 21 Nov 2007 15:22:21 -0000 Received: (qmail 30116 invoked by uid 22791); 21 Nov 2007 15:22:18 -0000 X-Spam-Check-By: sourceware.org Received: from ics.u-strasbg.fr (HELO ics.u-strasbg.fr) (130.79.112.250) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 21 Nov 2007 15:22:04 +0000 Received: from ICSMULLER (laocoon.u-strasbg.fr [130.79.112.72]) by ics.u-strasbg.fr (Postfix) with ESMTP id B1D0518701A; Wed, 21 Nov 2007 16:26:35 +0100 (CET) From: "Pierre Muller" To: "'Pedro Alves'" , References: <47437D49.6080600@portugalmail.pt> In-Reply-To: <47437D49.6080600@portugalmail.pt> Subject: RE: [win32] Fix watchpoint support Date: Wed, 21 Nov 2007 15:22:00 -0000 Message-ID: <001401c82c52$4e72dcb0$eb589610$@u-strasbg.fr> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit X-Mailer: Microsoft Office Outlook 12.0 Content-Language: en-us 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/msg00398.txt.bz2 Great, I tried it out and it seems indeed to work. I got +35 expected passes and -14 unexpected failures, there difference is probably due to several tests that are not performed because of previous errors within the same test. Just out of curiosity, why did you choose 0xfff0ff0 for dr[6]. I ask because I originally committed the hardware watchpoint support for cygwin, but between my first and second version, the setting of dr[6] was replaced by a FIXME, and I do not remember why I changed that part of the patch... Pierre > -----Original Message----- > From: gdb-patches-owner@sourceware.org [mailto:gdb-patches- > owner@sourceware.org] On Behalf Of Pedro Alves > Sent: Wednesday, November 21, 2007 1:35 AM > To: gdb-patches@sourceware.org > Subject: [win32] Fix watchpoint support > > Hi, > > The current watchpoint support for native debugging on win32 systems > doesn't work reliably. Quite often, we'd miss that the inferior > stopped due to a watchpoint triggering, because Windows wouldn't report > in DR6 which watchpoint triggered. Take a look at this "set debug > infrun 1; maint show-debug-regs 1" snippet [1]: > > infrun: TARGET_WAITKIND_STOPPED > infrun: stop_pc = 0x40108e > stopped_data_addr: > CONTROL (DR7): 000d0101 STATUS (DR6): 00000000 > ^^^^^^^^ > DR0: addr=0x00403010, ref.count=1 DR1: addr=0x00000000, > ref.count=0 > DR2: addr=0x00000000, ref.count=0 DR3: addr=0x00000000, > ref.count=0 > infrun: random signal 5 > ^^^^^^^^^^^^^^^^^^^^^^^ > > Program received signal SIGTRAP, Trace/breakpoint trap. > infrun: stop_stepping > remove_watchpoint (addr=403010, len=4, type=data-write): > CONTROL (DR7): 000d0100 STATUS (DR6): 00000000 > DR0: addr=0x00000000, ref.count=0 DR1: addr=0x00000000, > ref.count=0 > DR2: addr=0x00000000, ref.count=0 DR3: addr=0x00000000, > ref.count=0 main () at watch.c:11 > 11 printf ("count %d\n", count); > > 0x00403010 in that example is the address of a variable that was just > written to. > > [1] Full example here: > http://cygwin.com/ml/cygwin/2007-10/msg00057.html > > I ended up tracing the problem to win32_continue (gdb/win32-nat.c). > Currently, it looks somewhat like this: > > 1 win32_continue(TID) > 2 ContinueDebugEvent(current_TID) > 3 foreach thread in threads do > 4 if thread == TID > 5 ResumeThread(TID) > 6 SetThreadContext(TID, DEBUG_REGS) > 7 fi > 8 hcaerof > > The first problem is that we shoudn't be calling SetThreadContext after > ResumeThread (5,6) -- it should be the other way around. > > Second, when TID represents the thread of the current debug event, TID > will already be resumed after the ContinueDebugEvent call, because we > don't call SuspendThread on the current debug event thread. > This is what caused the weird watchpoint behaviour. > > I can see more than one way to fix this : > > 1 - Only call ContinueDebugEvent after resuming and setting the context > on every other thread: > > 1 win32_continue(TID) > 2 foreach thread in threads do > 3 if thread == TID > 4 ResumeThread(TID) > 5 SetThreadContext(TID, DEBUG_REGS) > 6 fi > 7 hcaerof > 8 ContinueDebugEvent(current_TID) > > 2 - Don't ResumeThread/SetThreadContext on the thread of the current > debug event, since it is already set in > win32_resume: > > 1 win32_continue(TID) > 2 foreach thread in threads do > 3 if thread == TID && thread != current_TID > 4 ResumeThread(TID) > 5 SetThreadContext(TID, DEBUG_REGS) > 6 fi > 7 hcaerof > 8 ContinueDebugEvent(current_TID) > > I tend to prefer the first, because win32_continue isn't always called > from win32_resume, and because until ContinueDebugEvent is called, all > threads are stopped. The second choice isn't as atomic. > > The attached patch implements option 1. It fixes a bunch of watchpoint > related failures on Cygwin, and has no regressions: > > PASS: gdb.base/commands.exp: add print command to watch > PASS: gdb.base/commands.exp: add continue command to watch > PASS: gdb.base/commands.exp: end commands on watch > -FAIL: gdb.base/commands.exp: continue with watch > +PASS: gdb.base/commands.exp: continue with watch > PASS: gdb.base/commands.exp: break factorial #3 > PASS: gdb.base/commands.exp: set value to 5 in > test_command_prompt_position > PASS: gdb.base/commands.exp: if test in > test_command_prompt_position > PASS: gdb.base/display.exp: display &k > PASS: gdb.base/display.exp: display/f f > PASS: gdb.base/display.exp: display/s &sum > -FAIL: gdb.base/display.exp: first disp > -FAIL: gdb.base/display.exp: second disp > +PASS: gdb.base/display.exp: first disp > +PASS: gdb.base/display.exp: second disp > PASS: gdb.base/display.exp: catch err > PASS: gdb.base/display.exp: disab disp 1 > PASS: gdb.base/display.exp: disab disp 2 > PASS: gdb.base/display.exp: re-enab of enab > PASS: gdb.base/display.exp: undisp > PASS: gdb.base/display.exp: info disp > -FAIL: gdb.base/display.exp: next hit > +PASS: gdb.base/display.exp: next hit > PASS: gdb.base/display.exp: undisp all > PASS: gdb.base/display.exp: disab 3 > PASS: gdb.base/display.exp: watch off > Running ../../../gdb- > server_submit/src/gdb/testsuite/gdb.base/recurse.exp ... > PASS: gdb.base/recurse.exp: next over b = 0 in first instance > PASS: gdb.base/recurse.exp: set first instance watchpoint > -FAIL: gdb.base/recurse.exp: continue to first instance watchpoint, > first time > +PASS: gdb.base/recurse.exp: continue to first instance watchpoint, > +first time > PASS: gdb.base/recurse.exp: continue to recurse (a = 9) > PASS: gdb.base/recurse.exp: continue to recurse (a = 8) > PASS: gdb.base/recurse.exp: continue to recurse (a = 7) > PASS: gdb.base/recurse.exp: continue to recurse (a = 5) > PASS: gdb.base/recurse.exp: next over b = 0 in second instance > PASS: gdb.base/recurse.exp: set second instance watchpoint > -FAIL: gdb.base/recurse.exp: continue to second instance watchpoint, > first time > +PASS: gdb.base/recurse.exp: continue to second instance watchpoint, > +first time > PASS: gdb.base/recurse.exp: continue to recurse (a = 4) > PASS: gdb.base/recurse.exp: continue to recurse (a = 3) > PASS: gdb.base/recurse.exp: continue to recurse (a = 2) > PASS: gdb.base/recurse.exp: continue to recurse (a = 1) > -FAIL: gdb.base/recurse.exp: continue to second instance watchpoint, > second time > +PASS: gdb.base/recurse.exp: continue to second instance watchpoint, > +second time > PASS: gdb.base/recurse.exp: second instance watchpoint deleted > when leaving scope > -FAIL: gdb.base/recurse.exp: continue to first instance watchpoint, > second time > +PASS: gdb.base/recurse.exp: continue to first instance watchpoint, > +second time > PASS: gdb.base/recurse.exp: first instance watchpoint deleted when > leaving scope > PASS: gdb.base/watchpoint.exp: break func1 > PASS: gdb.base/watchpoint.exp: set $func1_breakpoint_number = > $bpnum > PASS: gdb.base/watchpoint.exp: continue to breakpoint at func1 > -FAIL: gdb.base/watchpoint.exp: watchpoint hit, first time > -FAIL: gdb.base/watchpoint.exp: watchpoints found in > watchpoint/breakpoint table > +PASS: gdb.base/watchpoint.exp: watchpoint hit, first time > +PASS: gdb.base/watchpoint.exp: Watchpoint hit count is 1 > +PASS: gdb.base/watchpoint.exp: delete $func1_breakpoint_number > +PASS: gdb.base/watchpoint.exp: watchpoint hit, second time > +PASS: gdb.base/watchpoint.exp: Watchpoint hit count is 2 > +PASS: gdb.base/watchpoint.exp: watchpoint hit, third time > +PASS: gdb.base/watchpoint.exp: Watchpoint hit count is 3 > +PASS: gdb.base/watchpoint.exp: watchpoint hit, fourth time > +PASS: gdb.base/watchpoint.exp: Watchpoint hit count is 4 > +PASS: gdb.base/watchpoint.exp: watchpoint hit, fifth time > +PASS: gdb.base/watchpoint.exp: Watchpoint hit count is 5 > +PASS: gdb.base/watchpoint.exp: continue to marker2 > +PASS: gdb.base/watchpoint.exp: watchpoint disabled > +PASS: gdb.base/watchpoint.exp: continue until exit at continue to exit > +in > test_simple_watchpoint > +PASS: gdb.base/watchpoint.exp: watchpoints found in > +watchpoint/breakpoint table > PASS: gdb.base/watchpoint.exp: disable watchpoint in > test_disabling_watchpoints > PASS: gdb.base/watchpoint.exp: run to marker1 in > test_disabling_watchpoints > PASS: gdb.base/watchpoint.exp: watchpoint enabled > -FAIL: gdb.base/watchpoint.exp: watchpoint hit in > test_disabling_watchpoints, first time > -FAIL: gdb.base/watchpoint.exp: watchpoint hit in > test_disabling_watchpoints, second time > +PASS: gdb.base/watchpoint.exp: watchpoint hit in > +test_disabling_watchpoints, > first time > +PASS: gdb.base/watchpoint.exp: watchpoint hit in > +test_disabling_watchpoints, > second time > PASS: gdb.base/watchpoint.exp: disable watchpoint #2 in > test_disabling_watchpoints > PASS: gdb.base/watchpoint.exp: watchpoint disabled in table > PASS: gdb.base/watchpoint.exp: disabled watchpoint skipped > PASS: gdb.mi/mi-watch.exp: hw: mi runto callee4 > PASS: gdb.mi/mi-watch.exp: hw: break-watch operation > PASS: gdb.mi/mi-watch.exp: hw: list of watchpoints > -FAIL: gdb.mi/mi-watch.exp: hw: watchpoint trigger (2) > +PASS: gdb.mi/mi-watch.exp: hw: watchpoint trigger > PASS: gdb.mi/mi-watch.exp: hw: wp out of scope > PASS: gdb.mi/mi2-watch.exp: hw: break-watch operation > PASS: gdb.mi/mi2-watch.exp: hw: list of watchpoints > -FAIL: gdb.mi/mi2-watch.exp: hw: watchpoint trigger (2) > +PASS: gdb.mi/mi2-watch.exp: hw: watchpoint trigger > PASS: gdb.mi/mi2-watch.exp: hw: wp out of scope > PASS: gdb.threads/watchthreads.exp: successfully compiled posix > threads test case > PASS: gdb.threads/watchthreads.exp: watch args[0] > PASS: gdb.threads/watchthreads.exp: watch args[1] > -FAIL: gdb.threads/watchthreads.exp: threaded watch loop > -FAIL: gdb.threads/watchthreads.exp: first watchpoint on args[0] hit > -FAIL: gdb.threads/watchthreads.exp: first watchpoint on args[1] hit > -FAIL: gdb.threads/watchthreads.exp: watchpoint on args[0] hit in > thread > -FAIL: gdb.threads/watchthreads.exp: watchpoint on args[1] hit in > thread > -FAIL: gdb.threads/watchthreads.exp: combination of threaded > watchpoints = 30 > +PASS: gdb.threads/watchthreads.exp: threaded watch loop > +PASS: gdb.threads/watchthreads.exp: first watchpoint on args[0] hit > +PASS: gdb.threads/watchthreads.exp: first watchpoint on args[1] hit > +PASS: gdb.threads/watchthreads.exp: watchpoint on args[0] hit in > thread > +PASS: gdb.threads/watchthreads.exp: watchpoint on args[1] hit in > thread > +PASS: gdb.threads/watchthreads.exp: combination of threaded > watchpoints > += 30 > > OK ? > > -- > Pedro Alves > >