proposed PATCH: make watchpoints work correctly

[Paul Koning <pkoning at equallogic dot com>]

The following patch fixes a couple of problems.  

If HAVE_NONSTEPPABLE_WATCHPOINT is defined, watchpoints would not stop
for several reasons.  First of all, remote.c would return 0 rather
than the watch address after the single step past the instruction
where the watch trap occurred.  (Changes to infrun.c, remote.c)
Second, even after that is fixed, bpstat_stop_status wouldn't match
the watchpoint because removing the watchpoint (to single step)
deleted the valchain which is used to do the matching (Change to
breakpoint.c) 

If a watchpoint is defined, but the program stops for some other
reason -- either a breakpoint, or a break instruction hardcoded in the
target code -- bpstat_stop_status would encounter the watchpoint in
its scan for possible reasons.  It would take no action on it but
leave its "stop" and "print" bits set so you would see the stop
reported as if it were a watchpoint hit.  Also, a "next" or "step"
command would act as "stepi", i.e., stop after every instruction.
(Changes to breakpoint.c).

By the way, I first tried clearing "stop" in the status entry for a
watchpoint if the watchpoint wasn't the stop reason.  That doesn't
work -- if you do that, then the program doesn't stop if a watchpoint
is active but the stop reason is something else.

I have copyright assignment papers on file with FSF for gcc and
binutils -- does that cover this or should I add paperwork for gdb
specifically?   Also, I have no write access to anything.  Lastly, I'm
not sure how many things have to be tested for a patch to be
considered acceptable.  

     paul

diff -u gdb-5.3/gdb/breakpoint.c gdb-patches/gdb/breakpoint.c
--- gdb-5.3/gdb/breakpoint.c	Fri Aug 30 03:14:19 2002
+++ gdb-patches/gdb/breakpoint.c	Wed May 28 11:29:08 2003
@@ -704,6 +704,22 @@
 }
 
 
+/* 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.
 
@@ -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 ||
@@ -2503,10 +2512,16 @@
 	|| b->enable_state == bp_call_disabled)
       continue;
 
-    if (b->type != bp_watchpoint
-	&& b->type != bp_hardware_watchpoint
-	&& b->type != bp_read_watchpoint
-	&& b->type != bp_access_watchpoint
+    /* Watchpoints are treated as non-existent if the reason we stopped
+       wasn't a watchpoint (we didn't stop on some data address).  
+       Otherwise gdb won't stop on a break instruction in the code
+       (not from a breakpoint) when a watchpoint has been defined.
+    */
+    if (!((b->type == bp_watchpoint ||
+           b->type == bp_hardware_watchpoint ||
+           b->type == bp_read_watchpoint ||
+           b->type == bp_access_watchpoint)
+          && target_stopped_data_address () != 0)
 	&& b->type != bp_hardware_breakpoint
 	&& b->type != bp_catch_fork
 	&& b->type != bp_catch_vfork
@@ -6679,6 +6694,8 @@
   if (bpt->inserted)
     remove_breakpoint (bpt, mark_inserted);
 
+  free_valchain (bpt);
+  
   if (breakpoint_chain == bpt)
     breakpoint_chain = bpt->next;
 
diff -u gdb-5.3/gdb/infrun.c gdb-patches/gdb/infrun.c
--- gdb-5.3/gdb/infrun.c	Wed Sep 18 18:44:33 2002
+++ gdb-patches/gdb/infrun.c	Wed May 28 11:29:02 2003
@@ -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. */
diff -u gdb-5.3/gdb/remote.c gdb-patches/gdb/remote.c
--- gdb-5.3/gdb/remote.c	Sun Aug 18 19:17:57 2002
+++ gdb-patches/gdb/remote.c	Wed May 28 11:30:45 2003
@@ -4869,10 +4869,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;
 }