This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Fri, 02 Oct 2009 02:06:31 +0200, Joel Brobecker wrote:
> If I understand correctly the problem, the Bn bits in DR6 should be
> cleared after we have received a debug exception where they triggered;
> Otherwise, these bits remain set, and we think we keep hitting this
> watchpoint.  Correct?

Generally correct, more is in the update_watchpoint() comment and it is also
more an issue for the patch 2/4.


> Of course, there is no hook for resetting these bits "before returning
> to the interrupted task", so what you are doing instead is that you
> clear this flag when the watchpoint gets inserted.  But does this work
> in breakpoint always-inserted on mode?

Sent the playground patch + new comment for update_watchpoint() below.


> > @@ -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));
> 
> I don't understand why you clear the bit when removing the watchpoint,
> however. Can you explain?

This is a more general question about sanity checking the DR registers in
general.  The registers apparently can be also changed by the inferior itself.
There are two extreme behavior possibilities for GDB:

sanity-checking one:
GDB should sanity check and make a warning() any state changes by the inferior
even for bits it does not need.  (Such as a trigger bit for disabled
DR-watchpoint, DR-watchpoint enabled/disabled not by a request from GDB etc.)

quiet one:
GDB should try to quietly deal with any changes by the inferior making minimal
changes itself, just fixing up what is required.

The latter one is mostly the current state of the patch except for this part
you have pointed you.  I think if GDB did set some DR bit it should also reset
it back to not to leave needless clutter in the DR registers after for example
detaching from the inferior.


> > +  volatile int dummy;
> > +
> > +  dummy = watchee;
> > +  dummy = 1;
> > +  dummy = 2;	/* break-at-exit */
> 
> Is that to prevent watchee from being optimized away,

"volatile" is there for this purpose (although it has no effect on -O0).


> A comment explaining the purpose of this code would be nice (you know me
> :-).
+
> > +# Before the inferior gets started we would get:
> > +# 	Target does not support this type of hardware watchpoint.
> > +gdb_test "delete 1"
> 
> I don't quite understand this. After thinking about it, maybe it's just
> because you want to try a read watchpoint instead of the read/write that
> we have setup earlier? Oh, perhaps you are meaning to say that you cannot
> do the "rwatch" at the same time the other watchpoint was set, because
> it would trigger an error while starting the inferior?  Why would that
> be, btw?

I could start explaining it but I found more simple to fix the testcase. :-)


Thanks,
Jan


gdb/
2009-10-03  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.
	* breakpoint.c (update_watchpoint): Comment resetting watchpoints.

gdb/testsuite/
2009-10-03  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/watchpoint-hw.c: New variable dummy, assign dummies to it.
	* gdb.base/watchpoint-hw.exp: Test rwatch does not repeat.  Replace
	`start' by ${test}.exp.  Replace gdb_compile by prepare_for_testing.
	Remove variable `binfile'.  Use clean_restart and runto_main for the
	new testcase part.


--- 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 MASK 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/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -903,7 +903,32 @@ fetch_watchpoint_value (struct expression *exp, struct value **valp,
    - Update the list of values that must be watched in B->loc.
 
    If the watchpoint disposition is disp_del_at_next_stop, then do nothing.
-   If this is local watchpoint that is out of scope, delete it.  */
+   If this is local watchpoint that is out of scope, delete it.
+
+   Even with `set breakpoint always-inserted on' the watchpoints are
+   removed + inserted on each stop here.  Normal breakpoinst (bp_breakpoint;
+   not processed here) must never be removed to not to be missed in non-stop
+   mode by running threads.  Hardware watchpoints (is_hardware_watchpoina;
+   processed here) are specific for each LWP (as they are stored to tasks's
+   hardware debug registers) and therefore such LWP must be stopped first to be
+   able to modify its hardware watchpoints.
+
+   Hardware watchpoints (bp_read_watchpoint and bp_access_watchpoint; false hit
+   by bp_hardware_watchpoint is not user-visible - its hit is suppressed if the
+   memory content has not changed) must be reset exactly once after being
+   presented to the user, neither sooner nor later.
+   
+   The reset point must comply to these rules:
+   * target_stopped_by_watchpoint and target_stopped_data_address are called
+     many times in GDB during one stop.
+   * Multiple hardware watchpoints can be hit on the first stop while GDB
+     presents only one reason on each stop event to the user.  The other hits
+     will get present to the user without resuming inferior (and thus removing
+     the hardware watchpoint) as the pending hits use LWP_INFO->STATUS to
+     contain the saved SIGTRAP signal.  [linux]
+   Therefore the hardware watchpoint hit can get safely reset on the watchpoint
+   removal from inferior.  */
+
 static void
 update_watchpoint (struct breakpoint *b, int reparse)
 {
--- 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 MASK 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)
 {
@@ -837,6 +867,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,15 @@ int watchee;
 int
 main (void)
 {
+  volatile int dummy;
+
+  /* Stub lines are present as no breakpoints/watchpoint gets hit if current PC
+     already stays on the line PC while entering "step"/"continue".  */
+
+  dummy = 0;	/* Stub to catch WATCHEE access after runto_main.  */
+  dummy = watchee;
+  dummy = 1;	/* Stub to catch break-at-exit after WATCHEE has been hit.  */
+  dummy = 2;	/* break-at-exit */
+
   return 0;
 }
--- a/gdb/testsuite/gdb.base/watchpoint-hw.exp
+++ b/gdb/testsuite/gdb.base/watchpoint-hw.exp
@@ -21,19 +21,12 @@ if {(![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"]
     return
 }
 
-set testfile watchpoint-hw
-set srcfile ${testfile}.c
-set binfile ${objdir}/${subdir}/${testfile}
-if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
-    untested "Couldn't compile test program"
+set test watchpoint-hw
+set srcfile ${test}.c
+if { [prepare_for_testing ${test}.exp ${test} ${srcfile}] } {
     return -1
 }
 
-gdb_exit
-gdb_start
-gdb_reinitialize_dir $srcdir/$subdir
-gdb_load ${binfile}
-
 # Create the watchpoint before the inferior gets started.  Now the native CPU
 # target is still not active and its `to_can_use_hw_breakpoint' is not
 # installed, therefore only a software watchpoint gets created.
@@ -43,10 +36,28 @@ gdb_test "watch watchee" "atchpoint 1: watchee"
 # `runto_main' or `runto main' would delete the watchpoint created above.
 
 if { [gdb_start_cmd] < 0 } {
-    untested start
+    untested ${test}.exp
     return -1
 }
 gdb_test "" "main .* at .*" "start"
 
 # Check it is really a `hw'-watchpoint.
 gdb_test "info watchpoints" "1 *hw watchpoint .* watchee"
+
+
+clean_restart $test
+
+if ![runto_main] {
+    return -1
+}
+
+gdb_test "rwatch watchee"
+
+gdb_breakpoint [gdb_get_line_number "break-at-exit"]
+
+gdb_test "continue" "Continuing.\r\nHardware read watchpoint \[0-9\]+: 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"


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]