[RFA] Fix a windows bug if two watchpoints are used

["Pierre Muller" <muller at ics dot u-strasbg dot fr>]

  When using watchpoints with cygwin GDB,
I sometimes ended up getting a SIGTRAP
at the exact location of the watch.

  I finally figured out a reproducible way of
generation this bug.

  If I set two hardware watchpoints on 
two integers ival1 and ival3,
 dr[0] gets set to the address of ival1
and dr[1] to the address of ival3.
If I disable the first watchpoint,
but leave the second active,
  dr[0] is changed to &ival3,
but dr[1] is not reset to zero,
as i386_low.reset_addr function is not set
in windows-nat.c.

  The strange thing now is that on the next 
hit of &ival3, dr[6] get the value 0xffff0ff3
as if both dr[0] and dr[1] have been hit...
This despite the fact that dr[7]
correctly says that only the first (i.e. dr[0]
debug register is active).
  Now comes the tricky part, why does that generate a
SIGTRAP?
  In i386_stopped_data_address,
the bits of dr_status_mirror (copied from current value of dr[6]) 
are checked from 0 to 3, but if hit is found for I,
the address value is taken from dr_mirror array.

  Thus, according to dr[6], both debug registers 0 and 1
have been hit.
  at i=0, the correct address of ival3 is found in dr_mirror[0],
but after for i=1, the value of dr_mirror[1] is used, but this
value is 0, as only one watch is active.

  I don't really understand the reason why dr[6] gets the second bit
set, and to complicate things even more, this seems to be 
processor or OS version dependent!
  The bug shows up on a Dell Latitude D620 with Intel Core Duo T7200,
but not on a HP Pavilion with  AMD Athlon ... both running Windows XP 
(Dell Prof and HP Home, which might also make a difference).

  I propose below a patch that adds
a testcase with the two watches, plus
a fix for windows-nat.c (implementing reset_addr)
plus an additional change to i386-nat.c
to further avoid similar problems on other native targets,
by resetting also the RW_LEN bits of the control register
when a given debug register is disabled.

Is this patch OK?

Pierre

ChangeLog entry:

2009-06-04  Pierre Muller  <muller@ics.u-strasbg.fr>

	* i386-nat.c (I386_DR_DISABLE): Also reset the RW_LEN part of 
	control register.
	* windows-nat.c (cygwin_reset_dr): New forward declaration.
	(windows_continue): Force debug registers overwrite if
	debug_register_changed is set.
	(windows_resume): Likewise.
	(init_windows_ops): Register cygwin_reset_dr.
	(cygwin_reset_dr): New function.

testsuite/ChangeLog entry:

2009-06-04  Pierre Muller  <muller@ics.u-strasbg.fr>
	* gdb.base/watchpoints.c: New file.
	* gdb.base/watchpoints.exp: New test.


Index: i386-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-nat.c,v
retrieving revision 1.22
diff -u -p -r1.22 i386-nat.c
--- i386-nat.c	14 May 2009 09:37:00 -0000	1.22
+++ i386-nat.c	3 Jun 2009 21:48:19 -0000
@@ -124,8 +124,9 @@ struct i386_dr_low_type i386_dr_low;
 
 /* Disable the break/watchpoint in the I'th debug register.  */
 #define I386_DR_DISABLE(i) \
-  dr_control_mirror &= ~(3 << (DR_ENABLE_SIZE * (i)))
-
+  dr_control_mirror &= ~(3 << (DR_ENABLE_SIZE * (i)) | \
+		         0xf << (DR_CONTROL_SHIFT+DR_CONTROL_SIZE*(i)))
+ 
 /* Set in DR7 the RW and LEN fields for the I'th debug register.  */
 #define I386_DR_SET_RW_LEN(i,rwlen) \
   do { \
Index: windows-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/windows-nat.c,v
retrieving revision 1.192
diff -u -p -r1.192 windows-nat.c
--- windows-nat.c	24 May 2009 12:27:35 -0000	1.192
+++ windows-nat.c	3 Jun 2009 21:48:19 -0000
@@ -142,6 +142,7 @@ static int windows_thread_alive (struct 
 static void windows_kill_inferior (struct target_ops *);
 
 static void cygwin_set_dr (int i, CORE_ADDR addr);
+static void cygwin_reset_dr (int i);
 static void cygwin_set_dr7 (unsigned long val);
 static unsigned long cygwin_get_dr6 (void);
 
@@ -1133,7 +1134,7 @@ windows_continue (DWORD continue_status,
 
   for (th = &thread_head; (th = th->next) != NULL;)
     if ((id == -1 || id == (int) th->id)
-	&& th->suspended)
+	&& (th->suspended || debug_registers_changed))
       {
 	if (debug_registers_changed)
 	  {
@@ -1252,7 +1253,7 @@ windows_resume (struct target_ops *ops,
 	  th->context.EFlags |= FLAG_TRACE_BIT;
 	}
 
-      if (th->context.ContextFlags)
+      if (th->context.ContextFlags || debug_registers_changed)
 	{
 	  if (debug_registers_changed)
 	    {
@@ -1262,6 +1263,7 @@ windows_resume (struct target_ops *ops,
 	      th->context.Dr3 = dr[3];
 	      th->context.Dr6 = DR6_CLEAR_VALUE;
 	      th->context.Dr7 = dr[7];
+	      th->context.ContextFlags |= CONTEXT_DEBUG_REGISTERS;
 	    }
 	  CHECK (SetThreadContext (th->h, &th->context));
 	  th->context.ContextFlags = 0;
@@ -2176,7 +2178,7 @@ init_windows_ops (void)
 
   i386_dr_low.set_control = cygwin_set_dr7;
   i386_dr_low.set_addr = cygwin_set_dr;
-  i386_dr_low.reset_addr = NULL;
+  i386_dr_low.reset_addr = cygwin_reset_dr;
   i386_dr_low.get_status = cygwin_get_dr6;
 
   /* i386_dr_low.debug_register_length field is set by
@@ -2295,6 +2297,13 @@ cygwin_set_dr (int i, CORE_ADDR addr)
   debug_registers_used = 1;
 }
 
+/* Reset dr[i], to stay in sync with i386-nat dr_mirror.  */
+static void
+cygwin_reset_dr (int i)
+{
+  cygwin_set_dr (i, 0);
+}
+
 /* Pass the value VAL to the inferior in the DR7 debug control
    register.  Here we just store the address in D_REGS, the watchpoint
    will be actually set up in windows_wait.  */
Index: testsuite/gdb.base/watchpoints.c
===================================================================
RCS file: testsuite/gdb.base/watchpoints.c
diff -N testsuite/gdb.base/watchpoints.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.base/watchpoints.c	3 Jun 2009 21:48:19 -0000
@@ -0,0 +1,33 @@
+#include <stdio.h>
+#include <unistd.h>
+/*
+ * This source is mainly to test
+ * what happens when a watchpoint is removed
+ * what another watchpoint, inserted later is
+ * left active.
+ */
+
+int count = -1;
+int ival1 = -1;
+int ival2 = -1;
+int ival3 = -1;
+int ival4 = -1;
+
+int 
+main ()
+{
+#ifdef usestubs
+  set_debug_traps();
+  breakpoint();
+#endif
+
+  for (count = 0; count < 4; count++) {
+    ival1 = count; ival2 = count;
+    ival3 = count; ival4 = count;
+  }
+
+  ival1 = count; ival2 = count;  /* Outside loop */
+  ival3 = count; ival4 = count;
+
+  return 0;
+}
Index: testsuite/gdb.base/watchpoints.exp
===================================================================
RCS file: testsuite/gdb.base/watchpoints.exp
diff -N testsuite/gdb.base/watchpoints.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.base/watchpoints.exp	3 Jun 2009 21:48:19 -0000
@@ -0,0 +1,101 @@
+# Copyright 2009
+# Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file was written by Pierre Muller. (muller@ics.u-strasbg.fr)
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+set prms_id 0
+set bug_id 0
+
+set testfile "watchpoints"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+set wp_set 1
+
+if [get_compiler_info ${binfile}] {
+    return -1
+}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable
{debug}] != "" } {
+     untested watchpoint.exp
+     return -1
+}
+
+# Start with a fresh gdb.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load $binfile
+
+    runto_main
+    gdb_test "watch ival1" "" ""
+    gdb_test "watch ival3" "" ""
+
+    set timeout 600
+
+    gdb_test "cont" "Continuing.*\[Ww\]atchpoint.*ival1.*Old value =
-1.*New value = 0.*ival1 = count; ival2 = count;.*" "watchpoint hit, first
time"
+
+    gdb_test "cont" "Continuing.*\[Ww\]atchpoint.*ival3.*Old value =
-1.*New value = 0.*ival3 = count; ival4 = count;.*" "watchpoint hit, first
time"
+
+    # Check that the ival3 hit count is reported correctly
+    gdb_test "info break" ".*watchpoint\[ \t\]+keep\[ \t\]+y\[
\t\]+ival3\r\n\[ \t]+breakpoint already hit 1 time.*" "Watchpoint hit count
is 1"
+
+    # Continue until the next change for ival1, from 0 to 1.
+    gdb_test "cont" "Continuing.*\[Ww\]atchpoint.*ival1.*Old value = 0.*New
value = 1.*ival1 = count; ival2 = count;.*" "watchpoint ival1 hit, second
time"
+
+    # Check that the hit count for ival1 is reported correctly
+    gdb_test "info break" ".*watchpoint\[ \t\]+keep\[ \t\]+y\[
\t\]+ival1\r\n\[ \t]+breakpoint already hit 2 times.*" "Watchpoint ival1 hit
count is 2"
+
+    # Continue until the next change for ival3, from 0 to 1.
+    gdb_test "cont" "Continuing.*\[Ww\]atchpoint.*ival3.*Old value = 0.*New
value = 1.*ival3 = count; ival4 = count;.*" "watchpoint hit, second time"
+
+    # Check that the hit count is reported correctly
+    gdb_test "info break" ".*watchpoint\[ \t\]+keep\[ \t\]+y\[
\t\]+ival3\r\n\[ \t]+breakpoint already hit 2 times.*" "Watchpoint hit count
is 2"
+
+    # Continue until the next change, from 1 to 2.
+    gdb_test "cont" "Continuing.*\[Ww\]atchpoint.*ival1.*Old value = 1.*New
value = 2.*ival1 = count; ival2 = count;.*" "watchpoint ival1 hit, third
time"
+
+    # Check that the hit count is reported correctly
+    gdb_test "info break" ".*watchpoint\[ \t\]+keep\[ \t\]+y\[
\t\]+ival1\r\n\[ \t]+breakpoint already hit 3 times.*" "Watchpoint ival1 hit
count is 3"
+    # Disable ival1 watchpoint
+    gdb_test "disable 2" "" ""
+ 
+    # Continue until the next change, from 1 to 2.
+    gdb_test "cont" "Continuing.*\[Ww\]atchpoint.*ival3.*Old value = 1.*New
value = 2.*ival3 = count; ival4 = count;.*" "watchpoint hit, third time"
+
+    # Check that the hit count is reported correctly
+    gdb_test "info break" ".*watchpoint\[ \t\]+keep\[ \t\]+y\[
\t\]+ival3\r\n\[ \t]+breakpoint already hit 3 times.*" "Watchpoint hit count
is 3"
+    
+    # Continue until the next change, from 2 to 3.
+    gdb_test "cont" "Continuing.*\[Ww\]atchpoint.*ival3.*Old value = 2.*New
value = 3.*ival3 = count; ival4 = count;.*" "watchpoint hit, fourth time"
+
+    # Check that the hit count is reported correctly
+    gdb_test "info break" ".*watchpoint\[ \t\]+keep\[ \t\]+y\[
\t\]+ival3\r\n\[ \t]+breakpoint already hit 4 times.*" "Watchpoint hit count
is 4"
+
+    # Continue until the next change, from 3 to 4.
+    # Note that this one is outside the loop.
+
+    gdb_test "cont" "Continuing.*\[Ww\]atchpoint.*ival3.*Old value = 3.*New
value = 4.*ival3 = count; ival4 = count;.*" "watchpoint hit, fifth time"
+
+    # Check that the hit count is reported correctly
+    gdb_test "info break" ".*watchpoint\[ \t\]+keep\[ \t\]+y\[
\t\]+ival3\r\n\[ \t]+breakpoint already hit 5 times.*" "Watchpoint hit count
is 5"
+
+