watchpoint troubles

watchpoint troubles

[Paul Koning]

I'm having all sorts of watchpoint troubles with gdb 5.3 and a remote
target.   These are things I'm running into as I'm working to improve
the implementation of watchpoints in my remote stub.

In these tests I was using "awatch", i.e., watching both loads and
stores. 

First...  I had defined HAVE_NONSTEPPABLE_WATCHPOINT to force gdb to
step over the instruction where the watchpoint occurs.  That seems to
work, but it has a bizarre side effect:

Once the watchpoint is defined, every entry to gdb (whether from that
watchpoint or from single stepping) prints out a mention of that
watchpoint.  I tracked it down to bpstat_stop_status, which looks up
the watch address.  If that comes back from the target code as zero,
it continues the breakpoint scan loop ("continue" around line 2637 in
breakpoint.c).  Since the default settings for a bpstat entry is stop
and print, two evil things happen: (a) the watchpoint info is printed
(minor), (b) the single-stepping stops immediately, i.e., an "n"
command is turned into a "stepi" command! (major).

I figured I could fix this by setting stop and print to zero before
that continue, but if I do that then gdb doesn't stop at all anymore
as soon as I continue from a watchpoint.

So, just to try something, I removed the definition of
HAVE_NONSTEPPABLE_WATCHPOINT, because it looked like part of the
problem is that the "stopped due to a watchpoint" flag is no longer
set after the single-step after the watchpoint hits.

That exposed a different bug, which is very clear in the remote
protocol debug printout.

Without that flag, the first thing that gdb does after the watchpoint
entry is to read the address being watched.  Needless to say, that
causes a watchpoint recursion within the target stub.  In the case
where HAVE_NONSTEPPABLE_WATCHPOINT is defined, things work because the
watchpoint is removed before the memory read request is made.

Since gdb normally removes and reinserts watch/break points on every
entry, I figured it's gdb's job to do things in the right order.  Bad
assumption?  I can certainly hack up the stub to remove the
watchpoints before acting on any memory access requests from gdb, but
is that kind of hackery supposed to be done?

Thanks,
	paul

Re: watchpoint troubles

[Mark Salter]

>>>>> Paul Koning writes:

> I'm having all sorts of watchpoint troubles with gdb 5.3 and a remote
> target.   These are things I'm running into as I'm working to improve
> the implementation of watchpoints in my remote stub.

...

> Without that flag, the first thing that gdb does after the watchpoint
> entry is to read the address being watched.  Needless to say, that
> causes a watchpoint recursion within the target stub.  In the case
> where HAVE_NONSTEPPABLE_WATCHPOINT is defined, things work because the
> watchpoint is removed before the memory read request is made.

> Since gdb normally removes and reinserts watch/break points on every
> entry, I figured it's gdb's job to do things in the right order.  Bad
> assumption?  I can certainly hack up the stub to remove the
> watchpoints before acting on any memory access requests from gdb, but
> is that kind of hackery supposed to be done?

This has come up before:

  http://sources.redhat.com/ml/gdb-patches/2001-03/msg00506.html

I think the answer is that the stub should disable watchpoints
anytime the target stops and reenable them when stepping or
continuing.

--Mark

Re: watchpoint troubles

[Paul Koning]

>>>>> "Mark" == Mark Salter <msalter@redhat.com> writes:

>>>>> Paul Koning writes:
 >> I'm having all sorts of watchpoint troubles with gdb 5.3 and a
 >> remote target.  These are things I'm running into as I'm working
 >> to improve the implementation of watchpoints in my remote stub.

 Mark> ...

 >> Without that flag, the first thing that gdb does after the
 >> watchpoint entry is to read the address being watched.  Needless
 >> to say, that causes a watchpoint recursion within the target stub.
 >> In the case where HAVE_NONSTEPPABLE_WATCHPOINT is defined, things
 >> work because the watchpoint is removed before the memory read
 >> request is made.

 >> Since gdb normally removes and reinserts watch/break points on
 >> every entry, I figured it's gdb's job to do things in the right
 >> order.  Bad assumption?  I can certainly hack up the stub to
 >> remove the watchpoints before acting on any memory access requests
 >> from gdb, but is that kind of hackery supposed to be done?

 Mark> This has come up before:

 Mark> http://sources.redhat.com/ml/gdb-patches/2001-03/msg00506.html

 Mark> I think the answer is that the stub should disable watchpoints
 Mark> anytime the target stops and reenable them when stepping or
 Mark> continuing.

Thanks.  

I just read that thread and the final message seems to say "gdb can be
fixed to remove the watchpoints before doing the memory read in the
case where HAVE_NONSTEPPABLE_WATCHPOINT is undefined.  Yes, I thought
so.

I may do that just to try it.  As I mentioned, what I *really* want is
for the case where HAVE_NONSTEPPABLE_WATCHPOINT *is* defined to work
correctly.  Right now it doesn't.

    paul

Re: watchpoint troubles

[Paul Koning]

>>>>> "Paul" == Paul Koning <pkoning@equallogic.com> writes:

>>>>> "Mark" == Mark Salter <msalter@redhat.com> writes:
>>>>> Paul Koning writes:
 >>> I'm having all sorts of watchpoint troubles with gdb 5.3 and a
 >>> remote target.  These are things I'm running into as I'm working
 >>> to improve the implementation of watchpoints in my remote stub.

 Mark> ...

 >>> Without that flag, the first thing that gdb does after the
 >>> watchpoint entry is to read the address being watched.  Needless
 >>> to say, that causes a watchpoint recursion within the target
 >>> stub.  In the case where HAVE_NONSTEPPABLE_WATCHPOINT is defined,
 >>> things work because the watchpoint is removed before the memory
 >>> read request is made.

 >>> Since gdb normally removes and reinserts watch/break points on
 >>> every entry, I figured it's gdb's job to do things in the right
 >>> order.  Bad assumption?  I can certainly hack up the stub to
 >>> remove the watchpoints before acting on any memory access
 >>> requests from gdb, but is that kind of hackery supposed to be
 >>> done?

 Mark> This has come up before:

 Mark> http://sources.redhat.com/ml/gdb-patches/2001-03/msg00506.html

 Mark> I think the answer is that the stub should disable watchpoints
 Mark> anytime the target stops and reenable them when stepping or
 Mark> continuing.

 Paul> Thanks.

 Paul> I just read that thread and the final message seems to say "gdb
 Paul> can be fixed to remove the watchpoints before doing the memory
 Paul> read in the case where HAVE_NONSTEPPABLE_WATCHPOINT is
 Paul> undefined.  Yes, I thought so.

 Paul> I may do that just to try it.  As I mentioned, what I *really*
 Paul> want is for the case where HAVE_NONSTEPPABLE_WATCHPOINT *is*
 Paul> defined to work correctly.  Right now it doesn't.

Ok, so I tried it.

The simple approach (call remove_breakpoints before the call to
bp_stop_status) doesn't work.  And that may be another reason why
things won't work even when I do have HAVE_NONSTEPPABLE_WATCHPOINT
defined.

The problem is that remove_breakpoints for some reason frees the
val_chain of the watchpoint.  So when bpstat_stop_status looks to see
whether the watched address (as reported by the target) matches what
we're watching, it says "no" because the watchpoint appears not to be
watching anything (because its val_chain is null).  So now my
watchpoint doesn't stop at all...

A similar problem applies in the HAVE_NONSTEPPABLE_WATCHPOINT case,
because that's done by calling remove_breakpoints followed by
single-stepping the target.  So quite apart from the fact that
remote.c believes that the last thing that happened was a single step
rather than a watchpoint, the val_chain is now gone so it isn't seen
in bpstat_stop_status.

   paul

Re: watchpoint troubles

[Paul Koning]

>>>>> "Paul" == Paul Koning <pkoning@equallogic.com> writes:

>>>>> "Mark" == Mark Salter <msalter@redhat.com> writes:
>>>>> Paul Koning writes:
 >>> I'm having all sorts of watchpoint troubles with gdb 5.3 and a
 >>> remote target.  These are things I'm running into as I'm working
 >>> to improve the implementation of watchpoints in my remote stub.

 Mark> ...

 >>> Without that flag, the first thing that gdb does after the
 >>> watchpoint entry is to read the address being watched.  Needless
 >>> to say, that causes a watchpoint recursion within the target
 >>> stub.  In the case where HAVE_NONSTEPPABLE_WATCHPOINT is defined,
 >>> things work because the watchpoint is removed before the memory
 >>> read request is made.

 >>> Since gdb normally removes and reinserts watch/break points on
 >>> every entry, I figured it's gdb's job to do things in the right
 >>> order.  Bad assumption?  I can certainly hack up the stub to
 >>> remove the watchpoints before acting on any memory access
 >>> requests from gdb, but is that kind of hackery supposed to be
 >>> done?

 Mark> This has come up before:

 Mark> http://sources.redhat.com/ml/gdb-patches/2001-03/msg00506.html

 Mark> I think the answer is that the stub should disable watchpoints
 Mark> anytime the target stops and reenable them when stepping or
 Mark> continuing.

 Paul> Thanks.

 Paul> I just read that thread and the final message seems to say "gdb
 Paul> can be fixed to remove the watchpoints before doing the memory
 Paul> read in the case where HAVE_NONSTEPPABLE_WATCHPOINT is
 Paul> undefined.  Yes, I thought so.

 Paul> I may do that just to try it.  As I mentioned, what I *really*
 Paul> want is for the case where HAVE_NONSTEPPABLE_WATCHPOINT *is*
 Paul> defined to work correctly.  Right now it doesn't.

Ok, so after some messing around I did get things to work.  The case I
care about is where HAVE_NONSTEPPABLE_WATCHPOINT is defined, so I
created what looks like the minimal patch for that.  See below.

For the case where HAVE_NONSTEPPABLE_WATCHPOINT is *not* defined, this
doesn't quite work because then the watchpoint is still enabled, so if
the stub doesn't protect itself then you're in trouble.  That can be
addressed (given the other changes in the patch) by also doing a
remove_breakpoints() for that case.  I didn't bother because that case
doesn't matter to me.

The patch is somewhat ugly because it ties into remote.c.  There may
be a cleaner way but I'm not sure which route it should take.

   paul

Index: breakpoint.c
===================================================================
RCS file: /home/cvs/console_gdb/gdb/breakpoint.c,v
retrieving revision 1.1.1.2
diff -u -r1.1.1.2 breakpoint.c
--- breakpoint.c	3 Oct 2002 19:49:14 -0000	1.1.1.2
+++ breakpoint.c	7 May 2003 21:39:18 -0000
@@ -704,6 +704,22 @@
 }
 \f
 
+/* Helper routine: free the value chain for a breakpoint (watchpoint) */
+static void free_valchain (struct breakpoint *b)
+{
+  struct value *v;
+  struct value *n;
+
+  /* Free the saved value chain.  We will construct a new one
+     the next time the watchpoint is inserted.  */
+  for (v = b->val_chain; v; v = n)
+    {
+      n = v->next;
+      value_free (v);
+    }
+  b->val_chain = NULL;
+}
+
 /* insert_breakpoints is used when starting or continuing the program.
    remove_breakpoints is used when the program stops.
    Both return zero if successful,
@@ -961,6 +977,8 @@
 
 	if (within_current_scope)
 	  {
+	    free_valchain (b);
+
 	    /* Evaluate the expression and cut the chain of values
 	       produced off from the value chain.
 
@@ -970,7 +988,7 @@
 	    v = evaluate_expression (b->exp);
 	    VALUE_CONTENTS(v);
 	    value_release_to_mark (mark);
-
+	    
 	    b->val_chain = v;
 	    b->inserted = 1;
 
@@ -1461,15 +1479,6 @@
       if ((is == mark_uninserted) && (b->inserted))
 	warning ("Could not remove hardware watchpoint %d.",
 		 b->number);
-
-      /* Free the saved value chain.  We will construct a new one
-         the next time the watchpoint is inserted.  */
-      for (v = b->val_chain; v; v = n)
-	{
-	  n = v->next;
-	  value_free (v);
-	}
-      b->val_chain = NULL;
     }
   else if ((b->type == bp_catch_fork ||
 	    b->type == bp_catch_vfork ||
@@ -2633,8 +2642,12 @@
 	int found = 0;
 
 	addr = target_stopped_data_address ();
-	if (addr == 0)
+	if (addr == 0) {
+	  bs->print_it = print_it_noop;
+	  bs->stop = 0;
 	  continue;
+	}
+	
 	for (v = b->val_chain; v; v = v->next)
 	  {
 	    if (VALUE_LVAL (v) == lval_memory
@@ -6679,6 +6692,8 @@
   if (bpt->inserted)
     remove_breakpoint (bpt, mark_inserted);
 
+  free_valchain (bpt);
+  
   if (breakpoint_chain == bpt)
     breakpoint_chain = bpt->next;
 
Index: infrun.c
===================================================================
RCS file: /home/cvs/console_gdb/gdb/infrun.c,v
retrieving revision 1.1.1.2
diff -u -r1.1.1.2 infrun.c
--- infrun.c	3 Oct 2002 19:49:36 -0000	1.1.1.2
+++ infrun.c	7 May 2003 21:39:18 -0000
@@ -1389,11 +1389,12 @@
    by an event from the inferior, figure out what it means and take
    appropriate action.  */
 
+int stepped_after_stopped_by_watchpoint;
+
 void
 handle_inferior_event (struct execution_control_state *ecs)
 {
   CORE_ADDR tmp;
-  int stepped_after_stopped_by_watchpoint;
   int sw_single_step_trap_p = 0;
 
   /* Cache the last pid/waitstatus. */
Index: remote.c
===================================================================
RCS file: /home/cvs/console_gdb/gdb/remote.c,v
retrieving revision 1.5
diff -u -r1.5 remote.c
--- remote.c	7 Oct 2002 20:45:22 -0000	1.5
+++ remote.c	7 May 2003 21:39:19 -0000
@@ -4889,10 +4889,13 @@
     return remote_stopped_by_watchpoint_p;
 }
 
+extern int stepped_after_stopped_by_watchpoint;
+
 CORE_ADDR
 remote_stopped_data_address (void)
 {
-  if (remote_stopped_by_watchpoint ())
+  if (remote_stopped_by_watchpoint () ||
+      stepped_after_stopped_by_watchpoint)
     return remote_watch_data_address;
   return (CORE_ADDR)0;
 }