[win32] Fix watchpoint support

[win32] Fix watchpoint support

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 9954 bytes --]

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




[-- Attachment #2: fix_hwatch.diff --]
[-- Type: text/x-diff, Size: 3069 bytes --]

2007-11-21  Pedro Alves  <pedro_alves@portugalmail.pt>

	* win32-nat.c (win32_add_thread): Set Dr6 to 0xffff0ff0.
	(win32_continue): Resume threads and set the debug registers
	before calling ContinueDebugEvent.

---
 gdb/win32-nat.c |   51 +++++++++++++++++++++++++--------------------------
 1 file changed, 25 insertions(+), 26 deletions(-)

Index: src/gdb/win32-nat.c
===================================================================
--- src.orig/gdb/win32-nat.c	2007-11-20 03:19:22.000000000 +0000
+++ src/gdb/win32-nat.c	2007-11-20 23:27:58.000000000 +0000
@@ -304,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;
@@ -1132,31 +1131,32 @@ 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->suspended)
-	{
-	  if (th->suspended > 0)
-	    (void) ResumeThread (th->h);
-	  th->suspended = 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;
@@ -1242,8 +1242,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));

RE: [win32] Fix watchpoint support

[Pierre Muller]

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

Re: [win32] Fix watchpoint support

[Pedro Alves]

Pierre Muller wrote:
>   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.
> 

I frequently see intermitent problems like:

  Running ../../../gdb-server_submit/src/gdb/testsuite/gdb.base/langs.exp ...
  PASS: gdb.base/langs.exp: break on nonexistent function in langs.exp
-PASS: gdb.base/langs.exp: show language at csub in langs.exp
-PASS: gdb.base/langs.exp: backtrace in langs.exp
-PASS: gdb.base/langs.exp: up to foo in langs.exp
-PASS: gdb.base/langs.exp: show language at foo in langs.exp
-PASS: gdb.base/langs.exp: up to cppsub_ in langs.exp
-PASS: gdb.base/langs.exp: show language at cppsub_ in langs.exp
-PASS: gdb.base/langs.exp: up to fsub in langs.exp
-PASS: gdb.base/langs.exp: show language at fsub in langs.exp
-PASS: gdb.base/langs.exp: up to langs0__2do in langs.exp
-PASS: gdb.base/langs.exp: show language at langs0__2do in langs.exp
-PASS: gdb.base/langs.exp: up to main in langs.exp
-PASS: gdb.base/langs.exp: show language at main in langs.exp
-PASS: gdb.base/langs.exp: continue to exit in langs.exp
+ERROR: Couldn't send show language to GDB.
+UNRESOLVED: gdb.base/langs.exp: show language at csub in langs.exp
+ERROR: Couldn't send bt to GDB.
+UNRESOLVED: gdb.base/langs.exp: backtrace in langs.exp
+ERROR: Couldn't send up to GDB.
+UNRESOLVED: gdb.base/langs.exp: up to foo in langs.exp
+ERROR: Couldn't send show language to GDB.
+UNRESOLVED: gdb.base/langs.exp: show language at foo in langs.exp
+ERROR: Couldn't send up to GDB.
+UNRESOLVED: gdb.base/langs.exp: up to cppsub_ in langs.exp
+ERROR: Couldn't send show language to GDB.
+UNRESOLVED: gdb.base/langs.exp: show language at cppsub_ in langs.exp

Or like:

(...)
-PASS: gdb.base/return2.exp: continue to double_func
-PASS: gdb.base/return2.exp: return from double_func
-PASS: gdb.base/return2.exp: double value returned successfully
-PASS: gdb.base/return2.exp: validate result value not equal to program return value
+gdb compile failed, exit status is 1
+UNTESTED: gdb.base/return2.exp: return2.exp

The test that fails tends to change between runs.

I find myself redoing those tests that failed to be sure
it wasn't a regression.

>   Just out of curiosity, why did you choose 0xfff0ff0
> for dr[6].

The 80386 Programmer's Reference Manual says the debug handler
should move zeros to DR6 to avoid confusion, but I've read
somewhere that Windows expects it to be set to 0xffff0ff0
(undefined bits set to 1, could be 0xffff1ff0 too).  Can't
find the reference now.  Bummer.  In fact, if you search
for it on google.com/codesearch, you'll see a lot of people
choosing it too.  I don't think it really makes a
difference -- but that's no reason to keep a TODO there.  I've
tested with 0 and 0xffff0ff0, and saw no difference.

-- 
Cheers,
Pedro Alves

Re: [win32] Fix watchpoint support

[Christopher Faylor]

On Wed, Nov 21, 2007 at 12:35:21AM +0000, Pedro Alves wrote:
> 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.

Yes, that's clearly a bug.  It seems to have been introduced by the
debug register handling.

>2007-11-21  Pedro Alves  <pedro_alves@portugalmail.pt>
>
>	* win32-nat.c (win32_add_thread): Set Dr6 to 0xffff0ff0.
>	(win32_continue): Resume threads and set the debug registers
>	before calling ContinueDebugEvent.

I'm not clear on how this differs from your other patch.  It seems to do
many of the same things.  The principles are ok but it doesn't seem like
it could be applied in addition to the suspend count handling patch.

cgf

Re: [win32] Fix watchpoint support

[Pedro Alves]

Christopher Faylor wrote:

>> 2007-11-21  Pedro Alves  <pedro_alves@portugalmail.pt>
>>
>> 	* win32-nat.c (win32_add_thread): Set Dr6 to 0xffff0ff0.
>> 	(win32_continue): Resume threads and set the debug registers
>> 	before calling ContinueDebugEvent.
> 
> I'm not clear on how this differs from your other patch.  It seems to do
> many of the same things.  The principles are ok but it doesn't seem like
> it could be applied in addition to the suspend count handling patch.
> 

Oh, that's because the last patch I sent on the other thread
had this one merged in [1], because after discussing it with
Pierre, we came to the conclusion this was needed for a full
fix of the other bug.  I though merging it would be better for you,
since you'd only need to review one patch.  I was wrong then.
Sorry.  :/

[1]
http://sourceware.org/ml/gdb-patches/2007-11/msg00402.html

Cheers,
Pedro Alves