[patch 1/4] Fix hw watchpoints: [x86*] repeated rwatch output

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

Hi,

Intel Developer's Manual Volume 3B: System Programming Guide:
	"debug handlers should clear the [DR6] register before returning to
	the interrupted task"

This currently does not happen for x86* targets on the watchpoint trigger so:
* "watch" (read/write): the referenced value is read on each SIGTRAP to
  silently find out it did not change.
* "rwatch" (read access): The hit is falsely duplicated on each SIGTRAP
  unrelated of its reason.

This problem does not affect all the targets - ppc gets the trigger reported
through `struct siginfo' from linux_nat_get_siginfo which needs no reset.

On x86* there could also be some warnings/errors if the DR_STATUS bit triggers
with no reason but the bits are no silently cleared.


Thanks,
Jan


gdb/
2009-08-17  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Fix repeated rwatch output.
	* amd64-linux-nat.c (amd64_linux_dr_set, amd64_linux_dr_set_control)
	(amd64_linux_dr_set_addr, amd64_linux_dr_reset_addr)
	(amd64_linux_dr_get_status): New comments.
	(amd64_linux_dr_unset_status): New function.
	(_initialize_amd64_linux_nat): Install it.
	* i386-linux-nat.c (i386_linux_dr_get, i386_linux_dr_set)
	(i386_linux_dr_set_control, i386_linux_dr_set_addr)
	(i386_linux_dr_reset_addr, i386_linux_dr_get_status): New comments.
	(i386_linux_dr_unset_status): New function.
	(_initialize_i386_linux_nat): Install it.
	* i386-nat.c (I386_DR_WATCH_MASK): New macro.
	(I386_DR_WATCH_HIT): Use I386_DR_WATCH_MASK.
	(i386_insert_aligned_watchpoint, i386_remove_aligned_watchpoint): Call
	i386_dr_low.unset_status.
	* i386-nat.h (struct i386_dr_low_type): Extend comments for
	set_control, set_addr, reset_addr and get_status.  New unset_status.

gdb/testsuite/
2009-08-17  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/watchpoint-hw.exp: Test rwatch does not repear.
	* gdb.base/watchpoint-hw.c: New variable dummy, assign dummies to it.

--- a/gdb/amd64-linux-nat.c
+++ b/gdb/amd64-linux-nat.c
@@ -270,6 +270,8 @@ amd64_linux_dr_get (ptid_t ptid, int regnum)
   return value;
 }
 
+/* Set debug register REGNUM to VALUE in only the one LWP of PTID.  */
+
 static void
 amd64_linux_dr_set (ptid_t ptid, int regnum, unsigned long value)
 {
@@ -286,6 +288,8 @@ amd64_linux_dr_set (ptid_t ptid, int regnum, unsigned long value)
     perror_with_name (_("Couldn't write debug register"));
 }
 
+/* Set DR_CONTROL to ADDR in all LWPs of LWP_LIST.  */
+
 static void
 amd64_linux_dr_set_control (unsigned long control)
 {
@@ -297,6 +301,8 @@ amd64_linux_dr_set_control (unsigned long control)
     amd64_linux_dr_set (ptid, DR_CONTROL, control);
 }
 
+/* Set address REGNUM (zero based) to ADDR in all LWPs of LWP_LIST.  */
+
 static void
 amd64_linux_dr_set_addr (int regnum, CORE_ADDR addr)
 {
@@ -310,18 +316,41 @@ amd64_linux_dr_set_addr (int regnum, CORE_ADDR addr)
     amd64_linux_dr_set (ptid, DR_FIRSTADDR + regnum, addr);
 }
 
+/* Set address REGNUM (zero based) to zero in all LWPs of LWP_LIST.  */
+
 static void
 amd64_linux_dr_reset_addr (int regnum)
 {
   amd64_linux_dr_set_addr (regnum, 0);
 }
 
+/* Get DR_STATUS from only the one LWP of INFERIOR_PTID.  */
+
 static unsigned long
 amd64_linux_dr_get_status (void)
 {
   return amd64_linux_dr_get (inferior_ptid, DR_STATUS);
 }
 
+/* Unset VALUE bits in DR_STATUS in all LWPs of LWP_LIST.  */
+
+static void
+amd64_linux_dr_unset_status (unsigned long mask)
+{
+  struct lwp_info *lp;
+  ptid_t ptid;
+
+  ALL_LWPS (lp, ptid)
+    {
+      unsigned long value;
+      
+      value = amd64_linux_dr_get (ptid, DR_STATUS);
+      value &= ~mask;
+      amd64_linux_dr_set (ptid, DR_STATUS, value);
+    }
+}
+
+
 static void
 amd64_linux_new_thread (ptid_t ptid)
 {
@@ -672,6 +701,7 @@ _initialize_amd64_linux_nat (void)
   i386_dr_low.set_addr = amd64_linux_dr_set_addr;
   i386_dr_low.reset_addr = amd64_linux_dr_reset_addr;
   i386_dr_low.get_status = amd64_linux_dr_get_status;
+  i386_dr_low.unset_status = amd64_linux_dr_unset_status;
   i386_set_debug_register_length (8);
 
   /* Override the GNU/Linux inferior startup hook.  */
--- a/gdb/i386-linux-nat.c
+++ b/gdb/i386-linux-nat.c
@@ -586,6 +586,8 @@ i386_linux_store_inferior_registers (struct target_ops *ops,
 
 static unsigned long i386_linux_dr[DR_CONTROL + 1];
 
+/* Get debug register REGNUM value from only the one LWP of PTID.  */
+
 static unsigned long
 i386_linux_dr_get (ptid_t ptid, int regnum)
 {
@@ -614,6 +616,8 @@ i386_linux_dr_get (ptid_t ptid, int regnum)
   return value;
 }
 
+/* Set debug register REGNUM to VALUE in only the one LWP of PTID.  */
+
 static void
 i386_linux_dr_set (ptid_t ptid, int regnum, unsigned long value)
 {
@@ -630,6 +634,8 @@ i386_linux_dr_set (ptid_t ptid, int regnum, unsigned long value)
     perror_with_name (_("Couldn't write debug register"));
 }
 
+/* Set DR_CONTROL to ADDR in all LWPs of LWP_LIST.  */
+
 static void
 i386_linux_dr_set_control (unsigned long control)
 {
@@ -641,6 +647,8 @@ i386_linux_dr_set_control (unsigned long control)
     i386_linux_dr_set (ptid, DR_CONTROL, control);
 }
 
+/* Set address REGNUM (zero based) to ADDR in all LWPs of LWP_LIST.  */
+
 static void
 i386_linux_dr_set_addr (int regnum, CORE_ADDR addr)
 {
@@ -654,18 +662,40 @@ i386_linux_dr_set_addr (int regnum, CORE_ADDR addr)
     i386_linux_dr_set (ptid, DR_FIRSTADDR + regnum, addr);
 }
 
+/* Set address REGNUM (zero based) to zero in all LWPs of LWP_LIST.  */
+
 static void
 i386_linux_dr_reset_addr (int regnum)
 {
   i386_linux_dr_set_addr (regnum, 0);
 }
 
+/* Get DR_STATUS from only the one LWP of INFERIOR_PTID.  */
+
 static unsigned long
 i386_linux_dr_get_status (void)
 {
   return i386_linux_dr_get (inferior_ptid, DR_STATUS);
 }
 
+/* Unset VALUE bits in DR_STATUS in all LWPs of LWP_LIST.  */
+
+static void
+i386_linux_dr_unset_status (unsigned long mask)
+{
+  struct lwp_info *lp;
+  ptid_t ptid;
+
+  ALL_LWPS (lp, ptid)
+    {
+      unsigned long value;
+      
+      value = i386_linux_dr_get (ptid, DR_STATUS);
+      value &= ~mask;
+      i386_linux_dr_set (ptid, DR_STATUS, value);
+    }
+}
+
 static void
 i386_linux_new_thread (ptid_t ptid)
 {
@@ -832,6 +862,7 @@ _initialize_i386_linux_nat (void)
   i386_dr_low.set_addr = i386_linux_dr_set_addr;
   i386_dr_low.reset_addr = i386_linux_dr_reset_addr;
   i386_dr_low.get_status = i386_linux_dr_get_status;
+  i386_dr_low.unset_status = i386_linux_dr_unset_status;
   i386_set_debug_register_length (4);
 
   /* Override the default ptrace resume method.  */
--- a/gdb/i386-nat.c
+++ b/gdb/i386-nat.c
@@ -137,8 +137,11 @@ struct i386_dr_low_type i386_dr_low;
 #define I386_DR_GET_RW_LEN(i) \
   ((dr_control_mirror >> (DR_CONTROL_SHIFT + DR_CONTROL_SIZE * (i))) & 0x0f)
 
+/* Mask that this I'th watchpoint has triggered.  */
+#define I386_DR_WATCH_MASK(i)	(1 << (i))
+
 /* Did the watchpoint whose address is in the I'th register break?  */
-#define I386_DR_WATCH_HIT(i)	(dr_status_mirror & (1 << (i)))
+#define I386_DR_WATCH_HIT(i)	(dr_status_mirror & I386_DR_WATCH_MASK (i))
 
 /* A macro to loop over all debug registers.  */
 #define ALL_DEBUG_REGISTERS(i)	for (i = 0; i < DR_NADDR; i++)
@@ -358,6 +361,10 @@ i386_insert_aligned_watchpoint (CORE_ADDR addr, unsigned len_rw_bits)
   i386_dr_low.set_addr (i, addr);
   i386_dr_low.set_control (dr_control_mirror);
 
+  /* Only a sanity check for leftover bits (set possibly only by inferior).  */
+  if (i386_dr_low.unset_status)
+    i386_dr_low.unset_status (I386_DR_WATCH_MASK (i));
+
   return 0;
 }
 
@@ -387,6 +394,11 @@ i386_remove_aligned_watchpoint (CORE_ADDR addr, unsigned len_rw_bits)
 	      i386_dr_low.set_control (dr_control_mirror);
 	      if (i386_dr_low.reset_addr)
 		i386_dr_low.reset_addr (i);
+
+	      /* Status must be already queried for each LWP.  Otherwise it will
+	         be lost in all-stop mode + breakpoint always-inserted off.  */
+	      if (i386_dr_low.unset_status)
+		i386_dr_low.unset_status (I386_DR_WATCH_MASK (i));
 	    }
 	  retval = 0;
 	}
--- a/gdb/i386-nat.h
+++ b/gdb/i386-nat.h
@@ -49,16 +49,19 @@ extern void i386_use_watchpoints (struct target_ops *);
    functions are:
 
       set_control              -- set the debug control (DR7)
-				  register to a given value
+				  register to a given value for all LWPs
 
       set_addr                 -- put an address into one debug
-				  register
+				  register for all LWPs
 
       reset_addr               -- reset the address stored in
-				  one debug register
+				  one debug register for all LWPs
 
       get_status               -- return the value of the debug
-				  status (DR6) register.
+				  status (DR6) register for current LWP
+
+      unset_status             -- unset the specified bits of the debug
+				  status (DR6) register for all LWPs
 
    Additionally, the native file should set the debug_register_length
    field to 4 or 8 depending on the number of bytes used for
@@ -70,6 +73,7 @@ struct i386_dr_low_type
     void (*set_addr) (int, CORE_ADDR);
     void (*reset_addr) (int);
     unsigned long (*get_status) (void);
+    void (*unset_status) (unsigned long);
     int debug_register_length;
   };
 
--- a/gdb/testsuite/gdb.base/watchpoint-hw.c
+++ b/gdb/testsuite/gdb.base/watchpoint-hw.c
@@ -20,5 +20,11 @@ int watchee;
 int
 main (void)
 {
+  volatile int dummy;
+
+  dummy = watchee;
+  dummy = 1;
+  dummy = 2;	/* break-at-exit */
+
   return 0;
 }
--- a/gdb/testsuite/gdb.base/watchpoint-hw.exp
+++ b/gdb/testsuite/gdb.base/watchpoint-hw.exp
@@ -50,3 +50,17 @@ gdb_test "" "main .* at .*" "start"
 
 # Check it is really a `hw'-watchpoint.
 gdb_test "info watchpoints" "1 *hw watchpoint .* watchee"
+
+# Before the inferior gets started we would get:
+# 	Target does not support this type of hardware watchpoint.
+gdb_test "delete 1"
+gdb_test "rwatch watchee"
+
+gdb_breakpoint [gdb_get_line_number "break-at-exit"]
+
+gdb_test "continue" "Continuing.\r\nHardware read watchpoint 3: watchee\r\n\r\nValue = 0\r\n.*"
+
+# Here should be no repeated notification of the read watchpoint.
+gdb_test "continue" \
+	 "Continuing\\.\[ \r\n\]+Breakpoint \[0-9\]+, .*break-at-exit.*" \
+	 "continue to break-at-exit"