RE: [win32] Fix watchpoint support

["Pierre Muller" <muller@ics.u-strasbg.fr>]

  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
> 
>