don't run watchpoint commands when the watchpoint goes out of scope

don't run watchpoint commands when the watchpoint goes out of scope

[Pedro Alves]

While diffing gdb.log's between async/sync runs, I noticed
this bug:

(gdb) PASS: gdb.base/commands.exp: end commands on watch
continue
Continuing.
Hardware watchpoint 11: local_var

Old value = 0
New value = 1
factorial (value=1) at ../../../src/gdb/testsuite/gdb.base/run.c:81
81          return (value);
$38 = 1

Watchpoint 11 deleted because the program has left the block in
which its expression is valid.
0x0000000000400556 in main (argc=2, argv=0x7fffffffd9e8, envp=0x7fffffffda00) at ../../../src/gdb/testsuite/gdb.base/run.c:57
57          printf ("%d\n", factorial (1));
No symbol "value" in current context.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
(gdb) PASS: gdb.base/commands.exp: continue with watch


That 'No symbol "value" in current context' caught my
attention.  We stopped because the watchpoint is no longer
in scope, but, where's that "value" reference coming from?

Turns out to be two problems:

 - The test sets a "print value" command in the watchpoint, and
   that error happens because we tried to execute it when the
   watchpoint went out of scope.
   It's bogus to try to run the watchpoint's commands in 
   this case.  We are running its commands because watchpoints
   that go out of scope are also placed on the bpstat chain.
   Fixed by deleting the commands from the watchpoint when
   that happens.  The current scheme it that the watchpoint itself will
   be deleted on next stop, but meanwhile, it's commands shouldn't
   run.

 - The "commands" command sets breakpoint commands using
   map_breakpoint_numbers.  map_breakpoint_numbers
   calls its callback on each breakpoint that matches,
   _and_ on each of the breakpoint's related breakpoints.
   This ends up setting the commands in the watchpoint's
   internal scope breakpoint, which is the related breakpoint
   of the watchpoint, and the breakpoint that caused the stop
   above...  Walking the related breakpoints in map_breakpoint_numbers
   is bogus.  It should be the map_breakpoint_numbers' callback
   responsibility to iterate over the related breakpoints if it
   so needs/wants.

Tested on x86_64-linux and applied.

-- 
Pedro Alves

2011-05-24  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* breakpoint.c (watchpoint_check): If the watchpoint went out of
	scope, clear its command list.
	(map_breakpoint_numbers): Don't walk the related breakpoints list
	of each breakpoint.

	gdb/testsuite/
	* gdb.base/commands.exp (watchpoint_command_test): Check that the
	watchpoint's command list didn't execute when the watchpoint went
	out of scope.

---
 gdb/breakpoint.c                    |   21 +++------------------
 gdb/testsuite/gdb.base/commands.exp |   18 +++++++++++++++---
 2 files changed, 18 insertions(+), 21 deletions(-)

Index: src/gdb/testsuite/gdb.base/commands.exp
===================================================================
--- src.orig/gdb/testsuite/gdb.base/commands.exp	2011-05-24 15:48:26.228753680 +0100
+++ src/gdb/testsuite/gdb.base/commands.exp	2011-05-24 15:48:38.238753678 +0100
@@ -294,6 +294,9 @@ proc watchpoint_command_test {} {
 	    pass "begin commands on watch"
 	}
     }
+    # See the 'No symbol "value...' fail below.  This command will
+    # fail if it's executed in the wrong frame.  If adjusting the
+    # test, make sure this property holds.
     gdb_test_multiple "print value" "add print command to watch" {
 	-re ">$" {
 	    pass "add print command to watch"
@@ -308,9 +311,18 @@ proc watchpoint_command_test {} {
 	"" \
 	"end commands on watch"
 
-    gdb_test "continue" \
-	"Continuing.*\[Ww\]atchpoint $wp_id deleted because the program has left the block in.*which its expression is valid.*run.c:(57|82).*" \
-	"continue with watch"
+    set test "continue with watch"
+    gdb_test_multiple "continue" "$test" {
+	-re "No symbol \"value\" in current context.\r\n$gdb_prompt $" {
+	    # Happens if GDB actually runs the watchpoints commands,
+	    # even though the watchpoint was deleted for not being in
+	    # scope.
+	    fail $test
+	}
+ 	-re "Continuing.*\[Ww\]atchpoint $wp_id deleted because the program has left the block in.*which its expression is valid.*run.c:(57|82).*$gdb_prompt $" {
+	    pass $test
+	}
+   }
 }
 
 proc test_command_prompt_position {} {
Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2011-05-24 15:48:24.638753681 +0100
+++ src/gdb/breakpoint.c	2011-05-24 15:48:38.238753678 +0100
@@ -3880,6 +3880,8 @@ watchpoint_check (void *p)
 		   " deleted because the program has left the block in\n\
 which its expression is valid.\n");     
 
+      /* Make sure the watchpoint's commands aren't executed.  */
+      decref_counted_command_line (&b->commands);
       watchpoint_del_at_next_stop (b);
 
       return WP_DELETED;
@@ -11599,25 +11601,8 @@ map_breakpoint_numbers (char *args, void
 	  ALL_BREAKPOINTS_SAFE (b, tmp)
 	    if (b->number == num)
 	      {
-		struct breakpoint *related_breakpoint;
-
 		match = 1;
-		related_breakpoint = b;
-		do
-		  {
-		    struct breakpoint *next_related_b;
-
-		    /* FUNCTION can be also delete_breakpoint.  */
-		    next_related_b = related_breakpoint->related_breakpoint;
-		    function (related_breakpoint, data);
-
-		    /* For delete_breakpoint of the last entry of the ring we
-		       were traversing we would never get back to B.  */
-		    if (next_related_b == related_breakpoint)
-		      break;
-		    related_breakpoint = next_related_b;
-		  }
-		while (related_breakpoint != b);
+		function (b, data);
 		break;
 	      }
 	  if (match == 0)

Re: don't run watchpoint commands when the watchpoint goes out of scope

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

Pedro>    Walking the related breakpoints in map_breakpoint_numbers
Pedro>    is bogus.  It should be the map_breakpoint_numbers' callback
Pedro>    responsibility to iterate over the related breakpoints if it
Pedro>    so needs/wants.

I totally agree; but I wonder what relies on this.
Annotation indicates that this code has been there forever.

Tom

Re: don't run watchpoint commands when the watchpoint goes out of scope

[Pedro Alves]

On Tuesday 24 May 2011 19:56:22, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> 
> Pedro>    Walking the related breakpoints in map_breakpoint_numbers
> Pedro>    is bogus.  It should be the map_breakpoint_numbers' callback
> Pedro>    responsibility to iterate over the related breakpoints if it
> Pedro>    so needs/wants.
> 
> I totally agree; but I wonder what relies on this.
> Annotation indicates that this code has been there forever.

Hmm, I thought I had made sure nothing relies on this, but
I may have misread the code.  Sorry about that.

On "delete", we'd previously delete watchpoint's "related" breakpoints,
but now we're leaving in place with disp == del on next stop (done
directly from within delete_breakpoint.  That shouldn't hurt, other than
causing a stop on the scope breakpoint once for nothing.  On
"enable"/"disable", it shouldn't make a difference for watchpoint
"related" breakpoints either, since we never disable those
breakpoints.

The new ifunc "related" breakpoints is the large piece that I apparently
missed (most of it is outside breakpoint.c).  It looks like that it may
happen that we may end up with "related" breakpoints pending off a
bp_gnu_ifunc_resolver breakpoint, and we should delete them all
with "delete".  Thing is, "delete FOO" used to delete all the
related breakpoints (because it goes through map_breakpoint_numbers),
while "delete" does not, because that just calls delete_breakpoint
directly on each user breakpoint, and delete_breakpoint doesn't go
through ifunc related breakpoints deleting them.  Hmm.  Either
make delete_breakpoint itself always handle related breakpoints, or make
the "delete" command (and others) walk related breakpoints in the
paths that don't use map_breakpoint_numbers?  This looks like will
need a bit more fixing than I'd like to propose myself to at the
moment (my fix-bugs-as-I-trip-on-them stack is already nested
too deep).  Better keep the old bugs than add new different
bugs, so this patch restores the old behavior of iterating
over the "related" breakpoints, except in the case of the
"commands" command.  I'll apply it tomorrow, barring comments.

Tested on x86_64-linux, no regressions.

Thanks,
-- 
Pedro Alves

2011-05-25  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* breakpoint.c (iterate_related_breakpoints): New.
	(do_map_delete_breakpoint): New.
	(delete_command): Pass do_map_delete_breakpoint to
	map_breakpoint_numbers.
	(do_disable_breakpoint): New.
	(do_map_disable_breakpoint): Iterate over the breakpoint's related
	breakpoints.
	(do_enable_breakpoint): Rename to ...
	(enable_breakpoint_disp): ... this.
	(enable_breakpoint): Adjust.
	(do_enable_breakpoint): New.
	(enable_once_breakpoint): Delete.
	(do_map_enable_breakpoint): New.
	(do_map_enable_once_breakpoint): New.
	(enable_once_command, enable_delete_command)
	(delete_trace_command): Iterate over the breakpoint's related
	breakpoints.

---
 gdb/breakpoint.c |   99 ++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 84 insertions(+), 15 deletions(-)

Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2011-05-25 18:17:31.000000000 +0100
+++ src/gdb/breakpoint.c	2011-05-25 20:56:06.637364386 +0100
@@ -176,7 +176,7 @@ static void hbreak_command (char *, int)
 
 static void thbreak_command (char *, int);
 
-static void do_enable_breakpoint (struct breakpoint *, enum bpdisp);
+static void enable_breakpoint_disp (struct breakpoint *, enum bpdisp);
 
 static void stop_command (char *arg, int from_tty);
 
@@ -10812,8 +10812,42 @@ make_cleanup_delete_breakpoint (struct b
   return make_cleanup (do_delete_breakpoint_cleanup, b);
 }
 
-/* A callback for map_breakpoint_numbers that calls
-   delete_breakpoint.  */
+/* Iterator function to call a user-provided callback function once
+   for each of B and its related breakpoints.  */
+
+static void
+iterate_over_related_breakpoints (struct breakpoint *b,
+				  void (*function) (struct breakpoint *,
+						    void *),
+				  void *data)
+{
+  struct breakpoint *related;
+
+  related = b;
+  do
+    {
+      struct breakpoint *next;
+
+      /* FUNCTION may delete RELATED.  */
+      next = related->related_breakpoint;
+
+      if (next == related)
+	{
+	  /* RELATED is the last ring entry.  */
+	  function (related, data);
+
+	  /* FUNCTION may have deleted it, so we'd never reach back to
+	     B.  There's nothing left to do anyway, so just break
+	     out.  */
+	  break;
+	}
+      else
+	function (related, data);
+
+      related = next;
+    }
+  while (related != b);
+}
 
 static void
 do_delete_breakpoint (struct breakpoint *b, void *ignore)
@@ -10821,6 +10855,15 @@ do_delete_breakpoint (struct breakpoint
   delete_breakpoint (b);
 }
 
+/* A callback for map_breakpoint_numbers that calls
+   delete_breakpoint.  */
+
+static void
+do_map_delete_breakpoint (struct breakpoint *b, void *ignore)
+{
+  iterate_over_related_breakpoints (b, do_delete_breakpoint, NULL);
+}
+
 void
 delete_command (char *arg, int from_tty)
 {
@@ -10852,7 +10895,7 @@ delete_command (char *arg, int from_tty)
 	}
     }
   else
-    map_breakpoint_numbers (arg, do_delete_breakpoint, NULL);
+    map_breakpoint_numbers (arg, do_map_delete_breakpoint, NULL);
 }
 
 static int
@@ -11672,13 +11715,21 @@ disable_breakpoint (struct breakpoint *b
   observer_notify_breakpoint_modified (bpt);
 }
 
+/* A callback for iterate_over_related_breakpoints.  */
+
+static void
+do_disable_breakpoint (struct breakpoint *b, void *ignore)
+{
+  disable_breakpoint (b);
+}
+
 /* A callback for map_breakpoint_numbers that calls
    disable_breakpoint.  */
 
 static void
 do_map_disable_breakpoint (struct breakpoint *b, void *ignore)
 {
-  disable_breakpoint (b);
+  iterate_over_related_breakpoints (b, do_disable_breakpoint, NULL);
 }
 
 static void
@@ -11710,7 +11761,7 @@ disable_command (char *args, int from_tt
 }
 
 static void
-do_enable_breakpoint (struct breakpoint *bpt, enum bpdisp disposition)
+enable_breakpoint_disp (struct breakpoint *bpt, enum bpdisp disposition)
 {
   int target_resources_ok;
 
@@ -11771,7 +11822,13 @@ do_enable_breakpoint (struct breakpoint
 void
 enable_breakpoint (struct breakpoint *bpt)
 {
-  do_enable_breakpoint (bpt, bpt->disposition);
+  enable_breakpoint_disp (bpt, bpt->disposition);
+}
+
+static void
+do_enable_breakpoint (struct breakpoint *bpt, void *arg)
+{
+  enable_breakpoint (bpt);
 }
 
 /* A callback for map_breakpoint_numbers that calls
@@ -11780,7 +11837,7 @@ enable_breakpoint (struct breakpoint *bp
 static void
 do_map_enable_breakpoint (struct breakpoint *b, void *ignore)
 {
-  enable_breakpoint (b);
+  iterate_over_related_breakpoints (b, do_enable_breakpoint, NULL);
 }
 
 /* The enable command enables the specified breakpoints (or all defined
@@ -11816,27 +11873,39 @@ enable_command (char *args, int from_tty
 }
 
 static void
-enable_once_breakpoint (struct breakpoint *bpt, void *ignore)
+do_enable_breakpoint_disp (struct breakpoint *bpt, void *arg)
+{
+  enum bpdisp disp = *(enum bpdisp *) arg;
+
+  enable_breakpoint_disp (bpt, disp);
+}
+
+static void
+do_map_enable_once_breakpoint (struct breakpoint *bpt, void *ignore)
 {
-  do_enable_breakpoint (bpt, disp_disable);
+  enum bpdisp disp = disp_disable;
+
+  iterate_over_related_breakpoints (bpt, do_enable_breakpoint_disp, &disp);
 }
 
 static void
 enable_once_command (char *args, int from_tty)
 {
-  map_breakpoint_numbers (args, enable_once_breakpoint, NULL);
+  map_breakpoint_numbers (args, do_map_enable_once_breakpoint, NULL);
 }
 
 static void
-enable_delete_breakpoint (struct breakpoint *bpt, void *ignore)
+do_map_enable_delete_breakpoint (struct breakpoint *bpt, void *ignore)
 {
-  do_enable_breakpoint (bpt, disp_del);
+  enum bpdisp disp = disp_del;
+
+  iterate_over_related_breakpoints (bpt, do_enable_breakpoint_disp, &disp);
 }
 
 static void
 enable_delete_command (char *args, int from_tty)
 {
-  map_breakpoint_numbers (args, enable_delete_breakpoint, NULL);
+  map_breakpoint_numbers (args, do_map_enable_delete_breakpoint, NULL);
 }
 \f
 static void
@@ -12362,7 +12431,7 @@ delete_trace_command (char *arg, int fro
 	}
     }
   else
-    map_breakpoint_numbers (arg, do_delete_breakpoint, NULL);
+    map_breakpoint_numbers (arg, do_map_delete_breakpoint, NULL);
 }
 
 /* Helper function for trace_pass_command.  */

Re: don't run watchpoint commands when the watchpoint goes out of scope

[Pedro Alves]

On Wednesday 25 May 2011 21:52:00, Pedro Alves wrote:
> (...) this patch restores the old behavior of iterating
> over the "related" breakpoints, except in the case of the
> "commands" command.  I'll apply it tomorrow, barring comments.
> 
> Tested on x86_64-linux, no regressions.

Applied now.  Thanks.

-- 
Pedro Alves