[patch 1/7] Support a ring of related breakpoints

[patch 1/7] Support a ring of related breakpoints

[Jan Kratochvil]

Hi,

currently breakpoint->related_breakpoint is used only to connect two related
breakpoints.  During STT_GNU_IFUNC one thread can be in the resolver while
another thread also enters the resolver.  But both threads will have
a different return address where GDB wants to fetch the resolved address.
Therefore one needs to track arbitrary number of the return address
breakpoints while still catching new entries to the STT_GNU_IFUNC resolver.

There can be a possibility to create per-thread breakpoints stored for example
in infcall_control_state like step_resume_breakpoint is.  But I belive that an
STT_GNU_IFUNC resolver can call a different STT_GNU_IFUNC resolver plus this
generalization has been already written so it should not harm.

The testcase is provided as a former version of the patch had a regression.


Thanks,
Jan


gdb/
2011-03-19  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Support a ring of related breakpoints.
	* breakpoint.c (watchpoint_del_at_next_stop): New, move here code from
	other functions, add gdb_assert.
	(update_watchpoint, watchpoint_check): Add gdb_assert.  Use
	watchpoint_del_at_next_stop.
	(bpstat_check_watchpoint): Use watchpoint_del_at_next_stop.
	(bpstat_stop_status): Handle ring in related_breakpoint.
	(set_raw_breakpoint_without_location): Initialize ring in
	related_breakpoint.
	(delete_breakpoint): Handle ring in related_breakpoint, use
	watchpoint_del_at_next_stop.
	(map_breakpoint_numbers): Handle ring in related_breakpoint.

gdb/testsuite/
2011-03-19  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Support a ring of related breakpoints.
	* gdb.base/watchpoint-delete.c: New file.
	* gdb.base/watchpoint-delete.exp: New file.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1158,6 +1158,25 @@ watchpoint_in_thread_scope (struct breakpoint *b)
 	      && !is_executing (inferior_ptid)));
 }
 
+/* Set watchpoint B to disp_del_at_next_stop, even including its possible
+   associated bp_watchpoint_scope breakpoint.  */
+
+static void
+watchpoint_del_at_next_stop (struct breakpoint *b)
+{
+  gdb_assert (is_watchpoint (b));
+
+  if (b->related_breakpoint != b)
+    {
+      gdb_assert (b->related_breakpoint->type == bp_watchpoint_scope);
+      gdb_assert (b->related_breakpoint->related_breakpoint == b);
+      b->related_breakpoint->disposition = disp_del_at_next_stop;
+      b->related_breakpoint->related_breakpoint = b->related_breakpoint;
+      b->related_breakpoint = b;
+    }
+  b->disposition = disp_del_at_next_stop;
+}
+
 /* Assuming that B is a watchpoint:
    - Reparse watchpoint expression, if REPARSE is non-zero
    - Evaluate expression and store the result in B->val
@@ -1217,6 +1236,8 @@ update_watchpoint (struct breakpoint *b, int reparse)
   struct frame_id saved_frame_id;
   int frame_saved;
 
+  gdb_assert (is_watchpoint (b));
+
   /* If this is a local watchpoint, we only want to check if the
      watchpoint frame is in scope if the current thread is the thread
      that was used to create the watchpoint.  */
@@ -1452,13 +1473,7 @@ update_watchpoint (struct breakpoint *b, int reparse)
 Watchpoint %d deleted because the program has left the block\n\
 in which its expression is valid.\n"),
 		       b->number);
-      if (b->related_breakpoint)
-	{
-	  b->related_breakpoint->disposition = disp_del_at_next_stop;
-	  b->related_breakpoint->related_breakpoint = NULL;
-	  b->related_breakpoint= NULL;
-	}
-      b->disposition = disp_del_at_next_stop;
+      watchpoint_del_at_next_stop (b);
     }
 
   /* Restore the selected frame.  */
@@ -3713,6 +3728,8 @@ watchpoint_check (void *p)
   gdb_assert (bs->breakpoint_at != NULL);
   b = bs->breakpoint_at;
 
+  gdb_assert (is_watchpoint (b));
+
   /* If this is a local watchpoint, we only want to check if the
      watchpoint frame is in scope if the current thread is the thread
      that was used to create the watchpoint.  */
@@ -3822,13 +3839,7 @@ watchpoint_check (void *p)
 		   " deleted because the program has left the block in\n\
 which its expression is valid.\n");     
 
-      if (b->related_breakpoint)
-	{
-	  b->related_breakpoint->disposition = disp_del_at_next_stop;
-	  b->related_breakpoint->related_breakpoint = NULL;
-	  b->related_breakpoint = NULL;
-	}
-      b->disposition = disp_del_at_next_stop;
+      watchpoint_del_at_next_stop (b);
 
       return WP_DELETED;
     }
@@ -4033,9 +4044,7 @@ bpstat_check_watchpoint (bpstat bs)
 	    case 0:
 	      /* Error from catch_errors.  */
 	      printf_filtered (_("Watchpoint %d deleted.\n"), b->number);
-	      if (b->related_breakpoint)
-		b->related_breakpoint->disposition = disp_del_at_next_stop;
-	      b->disposition = disp_del_at_next_stop;
+	      watchpoint_del_at_next_stop (b);
 	      /* We've already printed what needs to be printed.  */
 	      bs->print_it = print_it_done;
 	      break;
@@ -4246,7 +4255,7 @@ bpstat_stop_status (struct address_space *aspace,
 	     watchpoint as triggered so that we will handle the
 	     out-of-scope event.  We'll get to the watchpoint next
 	     iteration.  */
-	  if (b->type == bp_watchpoint_scope)
+	  if (b->type == bp_watchpoint_scope && b->related_breakpoint != b)
 	    b->related_breakpoint->watchpoint_triggered = watch_triggered_yes;
 	}
     }
@@ -5697,6 +5706,7 @@ set_raw_breakpoint_without_location (struct gdbarch *gdbarch,
   b->ops = NULL;
   b->condition_not_parsed = 0;
   b->py_bp_object = NULL;
+  b->related_breakpoint = b;
 
   /* Add this breakpoint to the end of the chain so that a list of
      breakpoints will come out in order of increasing numbers.  */
@@ -10058,12 +10068,20 @@ delete_breakpoint (struct breakpoint *bpt)
 
   /* At least avoid this stale reference until the reference counting
      of breakpoints gets resolved.  */
-  if (bpt->related_breakpoint != NULL)
+  if (bpt->related_breakpoint != bpt)
     {
-      gdb_assert (bpt->related_breakpoint->related_breakpoint == bpt);
-      bpt->related_breakpoint->disposition = disp_del_at_next_stop;
-      bpt->related_breakpoint->related_breakpoint = NULL;
-      bpt->related_breakpoint = NULL;
+      struct breakpoint *related;
+
+      if (bpt->type == bp_watchpoint_scope)
+	watchpoint_del_at_next_stop (bpt->related_breakpoint);
+      else if (bpt->related_breakpoint->type == bp_watchpoint_scope)
+	watchpoint_del_at_next_stop (bpt);
+
+      /* Unlink bpt from the bpt->related_breakpoint ring.  */
+      for (related = bpt; related->related_breakpoint != bpt;
+	   related = related->related_breakpoint);
+      related->related_breakpoint = bpt->related_breakpoint;
+      bpt->related_breakpoint = bpt;
     }
 
   observer_notify_breakpoint_deleted (bpt->number);
@@ -10844,11 +10862,25 @@ map_breakpoint_numbers (char *args, void (*function) (struct breakpoint *,
 	  ALL_BREAKPOINTS_SAFE (b, tmp)
 	    if (b->number == num)
 	      {
-		struct breakpoint *related_breakpoint = b->related_breakpoint;
+		struct breakpoint *related_breakpoint;
+
 		match = 1;
-		function (b, data);
-		if (related_breakpoint)
-		  function (related_breakpoint, data);
+		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);
 		break;
 	      }
 	  if (match == 0)
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-delete.c
@@ -0,0 +1,33 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010, 2011 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+void
+func (void)
+{
+  volatile int x = 0;
+
+  x++;	/* break-here */
+  x++;
+}
+
+int
+main (void)
+{
+  func ();
+
+  return 0;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-delete.exp
@@ -0,0 +1,38 @@
+# Copyright 2010, 2011 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+set testfile "watchpoint-delete"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}] } {
+    untested ${testfile}.exp
+    return -1
+}
+
+# It is more compatible this way.
+gdb_test_no_output "set can-use-hw-watchpoints 0"
+
+if ![runto_main] {
+    return -1
+}
+
+# Ensure there is a parent frame to create related bp_watchpoint_scope.
+gdb_breakpoint [gdb_get_line_number "break-here"]
+gdb_continue_to_breakpoint "break-here" ".* break-here .*"
+
+gdb_test "watch x" {Watchpoint [0-9]+: x}
+
+gdb_test_no_output {delete $bpnum}

Re: [patch 1/7] Support a ring of related breakpoints

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> currently breakpoint->related_breakpoint is used only to connect
Jan> two related breakpoints.  During STT_GNU_IFUNC one thread can be in
Jan> the resolver while another thread also enters the resolver.  But
Jan> both threads will have a different return address where GDB wants
Jan> to fetch the resolved address.  Therefore one needs to track
Jan> arbitrary number of the return address breakpoints while still
Jan> catching new entries to the STT_GNU_IFUNC resolver.

I read this and it looks fine to me.

At first I didn't see how a ring was actually created, but the only set
of this field is in watch_command_1, and it links the two together:

      b->related_breakpoint = scope_breakpoint;
      scope_breakpoint->related_breakpoint = b;

Tom

Re: [patch 1/7] Support a ring of related breakpoints

[Jan Kratochvil]

On Mon, 21 Mar 2011 21:06:25 +0100, Tom Tromey wrote:
> At first I didn't see how a ring was actually created, but the only set
> of this field is in watch_command_1, and it links the two together:
> 
>       b->related_breakpoint = scope_breakpoint;
>       scope_breakpoint->related_breakpoint = b;

The real multi-breakpoint ring is then created in [patch 6/7]
elf_gnu_ifunc_resolver_stop.


Thanks,
Jan

[commit] Re: [patch 1/7] Support a ring of related breakpoints

[Jan Kratochvil]

On Mon, 21 Mar 2011 21:06:25 +0100, Tom Tromey wrote:
> >>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:
> 
> Jan> currently breakpoint->related_breakpoint is used only to connect
> Jan> two related breakpoints.  During STT_GNU_IFUNC one thread can be in
> Jan> the resolver while another thread also enters the resolver.  But
> Jan> both threads will have a different return address where GDB wants
> Jan> to fetch the resolved address.  Therefore one needs to track
> Jan> arbitrary number of the return address breakpoints while still
> Jan> catching new entries to the STT_GNU_IFUNC resolver.
> 
> I read this and it looks fine to me.

Checked in:
	http://sourceware.org/ml/gdb-cvs/2011-03/msg00309.html


Thanks,
Jan