public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [patch 1/2] Convert hardware watchpoints to use breakpoint_ops
@ 2010-08-17 19:41 Thiago Jung Bauermann
  2010-10-07 14:47 ` Thiago Jung Bauermann
  2010-10-16 17:43 ` Pedro Alves
  0 siblings, 2 replies; 24+ messages in thread
From: Thiago Jung Bauermann @ 2010-08-17 19:41 UTC (permalink / raw)
  To: gdb-patches ml

Hi all,

So I'm continuing my saga of submitting the patches implementing
advanced hardware debug features of the PowerPC BookE processors...

This one is in preparation for the patch for masked hardware watchpoints
and ranged hardware watchpoints. The next e-mail will explain better
what they are.

The point of this patch is that previous incarnations of the masked and
ranged hardware watchpoint implementation added many special cases in
breakpoint.c and by using the breakpoint_ops infrastructure most of them
can be eliminated. This follows a suggestion Joel made once when
reviewing the patch on ranged and masked breakpoints (which I'll submit
again later):

> In terms of implementation, I'm almost thinking that this type of
> breakpoint should be implemented using the breakpoint_ops structure.
> It would cause a bit of code redundancy, but your patch is typically
> doing:
> 
>   if (is_range_breakpoint (...))
>     {
>       do something
>     }
>   else
>     {
>       do the regular thing;
>     }
> 
> I'll let you explore this and let me know what you think. I'm 50/50
> on this suggestion, as it has pros and cons.  The advantage is that
> you no longer have to check for the type of hw_breakpoint that you
> have.
> I can immediately assume that this is the type that you have.  The
> downside is a bit of code duplication in how we display the
> information.
> We might be able to work around this by factorizing the code out of
> print_it_typical, print_one_breakpoint_location, etc...

This patch actually fixes some inconsistencies in the catchpoints
support: target.c sets the default implementation of
to_remove_{fork,vfork,exec}_catchpoint to be tcomplain, which throws an
exception, but remove_breakpoint_1 is not prepared to deal with it. The
only target which implements catchpoints is linux-nat.c and it doesn't
actually implement the to_remove_* methods, so in theory an exception
would be thrown when GDB tried to remove the catchpoints. The only
reason it works is that inf-child.c implements the remove methods to
return 0 indicating that the target doesn't support the feature and
linux-nat.c inherits those...

Also, remove_breakpoint_1 expects breakpoint_ops.remove to return 0 on
success and 1 on failure, but as I mentioned before inf-child.c uses a
different convention. This patch makes breakpoint_ops.insert and
breakpoint_ops.remove use the same convention used by
target_insert_watchpoint and target_remove_watchpoint, and they don't
throw exceptions anymore.

I could have implement the print_* methods for watchpoints which would
IMHO make watchpoint handling more object-oriented (= organized? :-) ),
but it would actually make the code slightly bigger (one switch
statement to distinguish between bp_hardware_watchpoint,
bp_read_watchpoint and bp_access_watchpoint per method) so I left this
out. I can implement them if you think it's a good idea.

Tested on ppc-linux, ppc64-linux and i686-linux with no regressions. Ok
to commit?
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


2010-08-17  Thiago Jung Bauermann  <bauerman@br.ibm.com>

	Convert hardware watchpoints to use breakpoint_ops.

gdb/
	* breakpoint.c (insert_catchpoint): Delete function.
	(update_watchpoint): Save and restore OPS to/from OLD_OPS when
	changing the type of the watchpoint.
	(insert_bp_location): Call the watchpoint or catchpoint's
	breakpoint_ops.insert method.
	(remove_breakpoint_1): Call the watchpoint or catchpoint's
	breakpoint_ops.remove method.
	(set_raw_breakpoint_without_location): Initialize the OLD_OPS field.
	(insert_watchpoint, remove_watchpoint): New functions.
	(watchpoint_breakpoint_ops): New structure.
	(watch_command_1): Initialize the OPS field.
	* breakpoint.h (breakpoint_ops) <insert>: Return int instead of void.
	Accept pointer to struct bp_location instead of pointer to
	struct breakpoint.  Adapt all implementations.
	(breakpoint_ops) <remove>: Accept pointer to struct bp_location instead
	of pointer to struct breakpoint.  Adapt all implementations.
	(breakpoint) <old_ops>: New field.
	* inf-child.c (inf_child_insert_fork_catchpoint)
	(inf_child_remove_fork_catchpoint, inf_child_insert_vfork_catchpoint)
	(inf_child_remove_vfork_catchpoint, inf_child_insert_exec_catchpoint)
	(inf_child_remove_exec_catchpoint, inf_child_set_syscall_catchpoint):
	Delete functions.
	(inf_child_target): Remove initialization of to_insert_fork_catchpoint,
	to_remove_fork_catchpoint, to_insert_vfork_catchpoint,
	to_remove_vfork_catchpoint, to_insert_exec_catchpoint,
	to_remove_exec_catchpoint and to_set_syscall_catchpoint.
	* target.c (update_current_target): Change default implementation of
	to_insert_fork_catchpoint, to_remove_fork_catchpoint,
	to_insert_vfork_catchpoint, to_remove_vfork_catchpoint,
	to_insert_exec_catchpoint, to_remove_exec_catchpoint and
	to_set_syscall_catchpoint to return_one.
	(debug_to_insert_fork_catchpoint, debug_to_insert_vfork_catchpoint)
	(debug_to_insert_exec_catchpoint): Report return value.
	* target.h (to_insert_fork_catchpoint, to_insert_vfork_catchpoint)
	(to_insert_fork_catchpoint): Change declaration to return int instead
	of void.

gdb/testsuite/
	* gdb.base/foll-exec.exp: Adapt to new error string when the catchpoint
	type is not supported.
	* gdb.base/foll-fork.exp: Likewise.
	* gdb.base/foll-vfork.exp: Likewise.

Index: gdb.git/gdb/breakpoint.c
===================================================================
--- gdb.git.orig/gdb/breakpoint.c	2010-08-16 20:17:16.000000000 -0300
+++ gdb.git/gdb/breakpoint.c	2010-08-16 20:17:23.000000000 -0300
@@ -1194,18 +1194,6 @@ breakpoint_restore_shadows (gdb_byte *bu
 }
 \f
 
-/* A wrapper function for inserting catchpoints.  */
-static void
-insert_catchpoint (struct ui_out *uo, void *args)
-{
-  struct breakpoint *b = (struct breakpoint *) args;
-
-  gdb_assert (b->type == bp_catchpoint);
-  gdb_assert (b->ops != NULL && b->ops->insert != NULL);
-
-  b->ops->insert (b);
-}
-
 /* Return true if BPT is of any hardware watchpoint kind.  */
 
 static int
@@ -1420,15 +1408,37 @@ update_watchpoint (struct breakpoint *b,
 	    mem_cnt = can_use_hardware_watchpoint (val_chain);
 
 	    if (!mem_cnt)
-	      b->type = bp_watchpoint;
+	      {
+		b->type = bp_watchpoint;
+
+		/* When downgrading to a software watchpoint, we have to
+		   remember the value of the OPS field so that we can restore
+		   it when upgrading to a hardware watchpoint again.  */
+		if (b->ops)
+		  {
+		    b->old_ops = b->ops;
+		    b->ops = NULL;
+		  }
+	      }
 	    else
 	      {
 		int target_resources_ok = target_can_use_hardware_watchpoint
 		  (bp_hardware_watchpoint, i + mem_cnt, other_type_used);
 		if (target_resources_ok <= 0)
-		  b->type = bp_watchpoint;
+		  {
+		    b->type = bp_watchpoint;
+		    if (b->ops)
+		      {
+			b->old_ops = b->ops;
+			b->ops = NULL;
+		      }
+		  }
 		else
-		  b->type = bp_hardware_watchpoint;
+		  {
+		    b->type = bp_hardware_watchpoint;
+		    if (b->old_ops)
+		      b->ops = b->old_ops;
+		  }
 	      }
 	  }
 
@@ -1742,10 +1752,7 @@ Note: automatically using hardware break
 	      watchpoints.  It's not clear that it's necessary... */
 	   && bpt->owner->disposition != disp_del_at_next_stop)
     {
-      val = target_insert_watchpoint (bpt->address,
-				      bpt->length,
-				      bpt->watchpoint_type,
-				      bpt->owner->cond_exp);
+      val = bpt->owner->ops->insert (bpt);
 
       /* If trying to set a read-watchpoint, and it turns out it's not
 	 supported, try emulating one with an access watchpoint.  */
@@ -1771,12 +1778,12 @@ Note: automatically using hardware break
 
 	  if (val == 1)
 	    {
-	      val = target_insert_watchpoint (bpt->address,
-					      bpt->length,
-					      hw_access,
-					      bpt->owner->cond_exp);
-	      if (val == 0)
-		bpt->watchpoint_type = hw_access;
+	      bpt->watchpoint_type = hw_access;
+	      val = bpt->owner->ops->insert (bpt);
+
+	      if (val)
+		/* Back to the original value.  */
+		bpt->watchpoint_type = hw_read;
 	    }
 	}
 
@@ -1785,14 +1792,22 @@ Note: automatically using hardware break
 
   else if (bpt->owner->type == bp_catchpoint)
     {
-      struct gdb_exception e = catch_exception (uiout, insert_catchpoint,
-						bpt->owner, RETURN_MASK_ERROR);
-      exception_fprintf (gdb_stderr, e, "warning: inserting catchpoint %d: ",
-			 bpt->owner->number);
-      if (e.reason < 0)
-	bpt->owner->enable_state = bp_disabled;
-      else
-	bpt->inserted = 1;
+      gdb_assert (bpt->owner->ops != NULL && bpt->owner->ops->insert != NULL);
+
+      val = bpt->owner->ops->insert (bpt);
+      if (val)
+	{
+	  bpt->owner->enable_state = bp_disabled;
+
+	  if (val == 1)
+	    warning (_("\
+Inserting catchpoint %d: Your system does not support this type of catchpoint."),
+		     bpt->owner->number);
+	  else
+	    warning (_("Error inserting catchpoint %d."), bpt->owner->number);
+	}
+
+      bpt->inserted = (val == 0);
 
       /* We've already printed an error message if there was a problem
 	 inserting this catchpoint, and we've disabled the catchpoint,
@@ -2443,8 +2458,7 @@ remove_breakpoint_1 (struct bp_location 
   else if (b->loc_type == bp_loc_hardware_watchpoint)
     {
       b->inserted = (is == mark_inserted);
-      val = target_remove_watchpoint (b->address, b->length,
-				      b->watchpoint_type, b->owner->cond_exp);
+      b->owner->ops->remove (b);
 
       /* Failure to remove any of the hardware watchpoints comes here.  */
       if ((is == mark_uninserted) && (b->inserted))
@@ -2457,9 +2471,10 @@ remove_breakpoint_1 (struct bp_location 
     {
       gdb_assert (b->owner->ops != NULL && b->owner->ops->remove != NULL);
 
-      val = b->owner->ops->remove (b->owner);
+      val = b->owner->ops->remove (b);
       if (val)
 	return val;
+
       b->inserted = (is == mark_inserted);
     }
 
@@ -5426,6 +5441,7 @@ set_raw_breakpoint_without_location (str
   b->exec_pathname = NULL;
   b->syscalls_to_be_caught = NULL;
   b->ops = NULL;
+  b->old_ops = NULL;
   b->condition_not_parsed = 0;
 
   /* Add this breakpoint to the end of the chain
@@ -5809,16 +5825,16 @@ disable_breakpoints_in_unloaded_shlib (s
 
 /* Implement the "insert" breakpoint_ops method for fork catchpoints.  */
 
-static void
-insert_catch_fork (struct breakpoint *b)
+static int
+insert_catch_fork (struct bp_location *b)
 {
-  target_insert_fork_catchpoint (PIDGET (inferior_ptid));
+  return target_insert_fork_catchpoint (PIDGET (inferior_ptid));
 }
 
 /* Implement the "remove" breakpoint_ops method for fork catchpoints.  */
 
 static int
-remove_catch_fork (struct breakpoint *b)
+remove_catch_fork (struct bp_location *b)
 {
   return target_remove_fork_catchpoint (PIDGET (inferior_ptid));
 }
@@ -5901,16 +5917,16 @@ static struct breakpoint_ops catch_fork_
 
 /* Implement the "insert" breakpoint_ops method for vfork catchpoints.  */
 
-static void
-insert_catch_vfork (struct breakpoint *b)
+static int
+insert_catch_vfork (struct bp_location *b)
 {
-  target_insert_vfork_catchpoint (PIDGET (inferior_ptid));
+  return target_insert_vfork_catchpoint (PIDGET (inferior_ptid));
 }
 
 /* Implement the "remove" breakpoint_ops method for vfork catchpoints.  */
 
 static int
-remove_catch_vfork (struct breakpoint *b)
+remove_catch_vfork (struct bp_location *b)
 {
   return target_remove_vfork_catchpoint (PIDGET (inferior_ptid));
 }
@@ -5993,20 +6009,20 @@ static struct breakpoint_ops catch_vfork
 /* Implement the "insert" breakpoint_ops method for syscall
    catchpoints.  */
 
-static void
-insert_catch_syscall (struct breakpoint *b)
+static int
+insert_catch_syscall (struct bp_location *b)
 {
   struct inferior *inf = current_inferior ();
 
   ++inf->total_syscalls_count;
-  if (!b->syscalls_to_be_caught)
+  if (!b->owner->syscalls_to_be_caught)
     ++inf->any_syscall_count;
   else
     {
       int i, iter;
 
       for (i = 0;
-           VEC_iterate (int, b->syscalls_to_be_caught, i, iter);
+           VEC_iterate (int, b->owner->syscalls_to_be_caught, i, iter);
            i++)
 	{
           int elem;
@@ -6027,30 +6043,30 @@ insert_catch_syscall (struct breakpoint 
 	}
     }
 
-  target_set_syscall_catchpoint (PIDGET (inferior_ptid),
-				 inf->total_syscalls_count != 0,
-				 inf->any_syscall_count,
-				 VEC_length (int, inf->syscalls_counts),
-				 VEC_address (int, inf->syscalls_counts));
+  return target_set_syscall_catchpoint (PIDGET (inferior_ptid),
+					inf->total_syscalls_count != 0,
+					inf->any_syscall_count,
+					VEC_length (int, inf->syscalls_counts),
+					VEC_address (int, inf->syscalls_counts));
 }
 
 /* Implement the "remove" breakpoint_ops method for syscall
    catchpoints.  */
 
 static int
-remove_catch_syscall (struct breakpoint *b)
+remove_catch_syscall (struct bp_location *b)
 {
   struct inferior *inf = current_inferior ();
 
   --inf->total_syscalls_count;
-  if (!b->syscalls_to_be_caught)
+  if (!b->owner->syscalls_to_be_caught)
     --inf->any_syscall_count;
   else
     {
       int i, iter;
 
       for (i = 0;
-           VEC_iterate (int, b->syscalls_to_be_caught, i, iter);
+           VEC_iterate (int, b->owner->syscalls_to_be_caught, i, iter);
            i++)
 	{
           int elem;
@@ -6349,14 +6365,14 @@ create_fork_vfork_event_catchpoint (stru
 
 /* Exec catchpoints.  */
 
-static void
-insert_catch_exec (struct breakpoint *b)
+static int
+insert_catch_exec (struct bp_location *b)
 {
-  target_insert_exec_catchpoint (PIDGET (inferior_ptid));
+  return target_insert_exec_catchpoint (PIDGET (inferior_ptid));
 }
 
 static int
-remove_catch_exec (struct breakpoint *b)
+remove_catch_exec (struct bp_location *b)
 {
   return target_remove_exec_catchpoint (PIDGET (inferior_ptid));
 }
@@ -7984,6 +8000,37 @@ watchpoint_exp_is_const (const struct ex
   return 1;
 }
 
+/* Implement the "insert" breakpoint_ops method for hardware watchpoints.  */
+
+static int
+insert_watchpoint (struct bp_location *bpt)
+{
+  return target_insert_watchpoint (bpt->address, bpt->length,
+				   bpt->watchpoint_type, bpt->owner->cond_exp);
+}
+
+/* Implement the "remove" breakpoint_ops method for hardware watchpoints.  */
+
+static int
+remove_watchpoint (struct bp_location *bpt)
+{
+  return target_remove_watchpoint (bpt->address, bpt->length,
+				   bpt->watchpoint_type, bpt->owner->cond_exp);
+}
+
+/* The breakpoint_ops structure to be used in hardware watchpoints.  */
+
+static struct breakpoint_ops watchpoint_breakpoint_ops =
+{
+  insert_watchpoint,
+  remove_watchpoint,
+  NULL, /* breakpoint_hit */
+  NULL, /* print_it */
+  NULL, /* print_one */
+  NULL, /* print_mention */
+  NULL  /* print_recreate */
+};
+
 /* accessflag:  hw_write:  watch write, 
                 hw_read:   watch read, 
 		hw_access: watch access (read or write) */
@@ -8226,6 +8273,8 @@ watch_command_1 (char *arg, int accessfl
     b->exp_string = savestring (exp_start, exp_end - exp_start);
   b->val = val;
   b->val_valid = 1;
+  b->ops = &watchpoint_breakpoint_ops;
+
   if (cond_start)
     b->cond_string = savestring (cond_start, cond_end - cond_start);
   else
Index: gdb.git/gdb/breakpoint.h
===================================================================
--- gdb.git.orig/gdb/breakpoint.h	2010-08-16 20:17:16.000000000 -0300
+++ gdb.git/gdb/breakpoint.h	2010-08-16 20:17:23.000000000 -0300
@@ -339,16 +339,18 @@ struct bp_location
    will be called instead of the performing the default action for this
    bptype.  */
 
-struct breakpoint_ops 
+struct breakpoint_ops
 {
-  /* Insert the breakpoint or activate the catchpoint.  Should raise
-     an exception if the operation failed.  */
-  void (*insert) (struct breakpoint *);
+  /* Insert the breakpoint or watchpoint or activate the catchpoint.
+     Return 0 for success, 1 if the breakpoint, watchpoint or catchpoint
+     type is not supported, -1 for failure.  */
+  int (*insert) (struct bp_location *);
 
   /* Remove the breakpoint/catchpoint that was previously inserted
-     with the "insert" method above.  Return non-zero if the operation
-     succeeded.  */
-  int (*remove) (struct breakpoint *);
+     with the "insert" method above.  Return 0 for success, 1 if the
+     breakpoint, watchpoint or catchpoint type is not supported,
+     -1 for failure.  */
+  int (*remove) (struct bp_location *);
 
   /* Return non-zero if the debugger should tell the user that this
      breakpoint was hit.  */
@@ -525,6 +527,11 @@ struct breakpoint
     /* Methods associated with this breakpoint.  */
     struct breakpoint_ops *ops;
 
+    /* When a watchpoint changes from hardware to software watchpoint, its
+       OPS field will be set to NULL.  We need to remember its original value
+       so that we can restore it if the watchpoint is upgraded again later.  */
+    struct breakpoint_ops *old_ops;
+
     /* Is breakpoint's condition not yet parsed because we found
        no location initially so had no context to parse
        the condition in.  */
Index: gdb.git/gdb/inf-child.c
===================================================================
--- gdb.git.orig/gdb/inf-child.c	2010-08-16 20:17:16.000000000 -0300
+++ gdb.git/gdb/inf-child.c	2010-08-16 20:17:23.000000000 -0300
@@ -94,36 +94,6 @@ inf_child_acknowledge_created_inferior (
      created inferior" operation by a debugger.  */
 }
 
-static void
-inf_child_insert_fork_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of fork
-     events.  */
-}
-
-static int
-inf_child_remove_fork_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of fork
-     events.  */
-  return 0;
-}
-
-static void
-inf_child_insert_vfork_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of vfork
-     events.  */
-}
-
-static int
-inf_child_remove_vfork_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of vfork
-     events.  */
-  return 0;
-}
-
 static int
 inf_child_follow_fork (struct target_ops *ops, int follow_child)
 {
@@ -132,30 +102,6 @@ inf_child_follow_fork (struct target_ops
   return 0;
 }
 
-static void
-inf_child_insert_exec_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of exec
-     events.  */
-}
-
-static int
-inf_child_remove_exec_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of exec
-     events.  */
-  return 0;
-}
-
-static int
-inf_child_set_syscall_catchpoint (int pid, int needed, int any_count,
-				  int table_size, int *table)
-{
-  /* This version of Unix doesn't support notification of syscall
-     events.  */
-  return 0;
-}
-
 static int
 inf_child_can_run (void)
 {
@@ -193,14 +139,7 @@ inf_child_target (void)
   t->to_terminal_info = child_terminal_info;
   t->to_post_startup_inferior = inf_child_post_startup_inferior;
   t->to_acknowledge_created_inferior = inf_child_acknowledge_created_inferior;
-  t->to_insert_fork_catchpoint = inf_child_insert_fork_catchpoint;
-  t->to_remove_fork_catchpoint = inf_child_remove_fork_catchpoint;
-  t->to_insert_vfork_catchpoint = inf_child_insert_vfork_catchpoint;
-  t->to_remove_vfork_catchpoint = inf_child_remove_vfork_catchpoint;
   t->to_follow_fork = inf_child_follow_fork;
-  t->to_insert_exec_catchpoint = inf_child_insert_exec_catchpoint;
-  t->to_remove_exec_catchpoint = inf_child_remove_exec_catchpoint;
-  t->to_set_syscall_catchpoint = inf_child_set_syscall_catchpoint;
   t->to_can_run = inf_child_can_run;
   t->to_pid_to_exec_file = inf_child_pid_to_exec_file;
   t->to_stratum = process_stratum;
Index: gdb.git/gdb/linux-nat.c
===================================================================
--- gdb.git.orig/gdb/linux-nat.c	2010-08-16 20:17:16.000000000 -0300
+++ gdb.git/gdb/linux-nat.c	2010-08-16 20:17:23.000000000 -0300
@@ -961,36 +961,34 @@ Attaching after process %d fork to child
 }
 
 \f
-static void
+static int
 linux_child_insert_fork_catchpoint (int pid)
 {
-  if (! linux_supports_tracefork (pid))
-    error (_("Your system does not support fork catchpoints."));
+  return !linux_supports_tracefork (pid);
 }
 
-static void
+static int
 linux_child_insert_vfork_catchpoint (int pid)
 {
-  if (!linux_supports_tracefork (pid))
-    error (_("Your system does not support vfork catchpoints."));
+  return !linux_supports_tracefork (pid);
 }
 
-static void
+static int
 linux_child_insert_exec_catchpoint (int pid)
 {
-  if (!linux_supports_tracefork (pid))
-    error (_("Your system does not support exec catchpoints."));
+  return !linux_supports_tracefork (pid);
 }
 
 static int
 linux_child_set_syscall_catchpoint (int pid, int needed, int any_count,
 				    int table_size, int *table)
 {
-  if (! linux_supports_tracesysgood (pid))
-    error (_("Your system does not support syscall catchpoints."));
+  if (!linux_supports_tracesysgood (pid))
+    return 1;
+
   /* On GNU/Linux, we ignore the arguments.  It means that we only
      enable the syscall catchpoints, but do not disable them.
-     
+
      Also, we do not use the `table' information because we do not
      filter system calls here.  We let GDB do the logic for us.  */
   return 0;
Index: gdb.git/gdb/target.c
===================================================================
--- gdb.git.orig/gdb/target.c	2010-08-16 20:17:16.000000000 -0300
+++ gdb.git/gdb/target.c	2010-08-16 20:17:23.000000000 -0300
@@ -782,26 +782,26 @@ update_current_target (void)
 	    (void (*) (int))
 	    target_ignore);
   de_fault (to_insert_fork_catchpoint,
-	    (void (*) (int))
-	    tcomplain);
+	    (int (*) (int))
+	    return_one);
   de_fault (to_remove_fork_catchpoint,
 	    (int (*) (int))
-	    tcomplain);
+	    return_one);
   de_fault (to_insert_vfork_catchpoint,
-	    (void (*) (int))
-	    tcomplain);
+	    (int (*) (int))
+	    return_one);
   de_fault (to_remove_vfork_catchpoint,
 	    (int (*) (int))
-	    tcomplain);
+	    return_one);
   de_fault (to_insert_exec_catchpoint,
-	    (void (*) (int))
-	    tcomplain);
+	    (int (*) (int))
+	    return_one);
   de_fault (to_remove_exec_catchpoint,
 	    (int (*) (int))
-	    tcomplain);
+	    return_one);
   de_fault (to_set_syscall_catchpoint,
 	    (int (*) (int, int, int, int, int *))
-	    tcomplain);
+	    return_one);
   de_fault (to_has_exited,
 	    (int (*) (int, int, int *))
 	    return_zero);
@@ -3657,13 +3657,17 @@ debug_to_acknowledge_created_inferior (i
 		      pid);
 }
 
-static void
+static int
 debug_to_insert_fork_catchpoint (int pid)
 {
-  debug_target.to_insert_fork_catchpoint (pid);
+  int retval;
 
-  fprintf_unfiltered (gdb_stdlog, "target_insert_fork_catchpoint (%d)\n",
-		      pid);
+  retval = debug_target.to_insert_fork_catchpoint (pid);
+
+  fprintf_unfiltered (gdb_stdlog, "target_insert_fork_catchpoint (%d) = %d\n",
+		      pid, retval);
+
+  return retval;
 }
 
 static int
@@ -3679,13 +3683,17 @@ debug_to_remove_fork_catchpoint (int pid
   return retval;
 }
 
-static void
+static int
 debug_to_insert_vfork_catchpoint (int pid)
 {
-  debug_target.to_insert_vfork_catchpoint (pid);
+  int retval;
 
-  fprintf_unfiltered (gdb_stdlog, "target_insert_vfork_catchpoint (%d)\n",
-		      pid);
+  retval = debug_target.to_insert_vfork_catchpoint (pid);
+
+  fprintf_unfiltered (gdb_stdlog, "target_insert_vfork_catchpoint (%d) = %d\n",
+		      pid, retval);
+
+  return retval;
 }
 
 static int
@@ -3701,13 +3709,17 @@ debug_to_remove_vfork_catchpoint (int pi
   return retval;
 }
 
-static void
+static int
 debug_to_insert_exec_catchpoint (int pid)
 {
-  debug_target.to_insert_exec_catchpoint (pid);
+  int retval;
 
-  fprintf_unfiltered (gdb_stdlog, "target_insert_exec_catchpoint (%d)\n",
-		      pid);
+  retval = debug_target.to_insert_exec_catchpoint (pid);
+
+  fprintf_unfiltered (gdb_stdlog, "target_insert_exec_catchpoint (%d) = %d\n",
+		      pid, retval);
+
+  return retval;
 }
 
 static int
Index: gdb.git/gdb/target.h
===================================================================
--- gdb.git.orig/gdb/target.h	2010-08-16 20:17:16.000000000 -0300
+++ gdb.git/gdb/target.h	2010-08-16 20:17:23.000000000 -0300
@@ -468,12 +468,12 @@ struct target_ops
 				char *, char *, char **, int);
     void (*to_post_startup_inferior) (ptid_t);
     void (*to_acknowledge_created_inferior) (int);
-    void (*to_insert_fork_catchpoint) (int);
+    int (*to_insert_fork_catchpoint) (int);
     int (*to_remove_fork_catchpoint) (int);
-    void (*to_insert_vfork_catchpoint) (int);
+    int (*to_insert_vfork_catchpoint) (int);
     int (*to_remove_vfork_catchpoint) (int);
     int (*to_follow_fork) (struct target_ops *, int);
-    void (*to_insert_exec_catchpoint) (int);
+    int (*to_insert_exec_catchpoint) (int);
     int (*to_remove_exec_catchpoint) (int);
     int (*to_set_syscall_catchpoint) (int, int, int, int, int *);
     int (*to_has_exited) (int, int, int *);
@@ -1087,7 +1087,10 @@ int target_follow_fork (int follow_child
 
    TABLE is an array of ints, indexed by syscall number.  An element in
    this array is nonzero if that syscall should be caught.  This argument
-   only matters if ANY_COUNT is zero.  */
+   only matters if ANY_COUNT is zero.
+
+   Return 0 for success, 1 if syscall catchpoints are not supported or -1
+   for failure.  */
 
 #define target_set_syscall_catchpoint(pid, needed, any_count, table_size, table) \
      (*current_target.to_set_syscall_catchpoint) (pid, needed, any_count, \
Index: gdb.git/gdb/testsuite/gdb.base/foll-exec.exp
===================================================================
--- gdb.git.orig/gdb/testsuite/gdb.base/foll-exec.exp	2010-08-16 20:17:16.000000000 -0300
+++ gdb.git/gdb/testsuite/gdb.base/foll-exec.exp	2010-08-16 20:17:23.000000000 -0300
@@ -89,7 +89,7 @@ proc do_exec_tests {} {
    gdb_test "catch exec" "Catchpoint \[0-9\]* \\(exec\\)" "insert first exec catchpoint"
    set has_exec_catchpoints 0
    gdb_test_multiple "continue" "continue to first exec catchpoint" {
-     -re ".*Your system does not support exec catchpoints.*$gdb_prompt $" {
+     -re ".*Your system does not support this type of catchpoint.*$gdb_prompt $" {
        unsupported "continue to first exec catchpoint"
      }
      -re ".*Catchpoint.*$gdb_prompt $" {
Index: gdb.git/gdb/testsuite/gdb.base/foll-fork.exp
===================================================================
--- gdb.git.orig/gdb/testsuite/gdb.base/foll-fork.exp	2010-08-16 20:17:16.000000000 -0300
+++ gdb.git/gdb/testsuite/gdb.base/foll-fork.exp	2010-08-16 20:17:23.000000000 -0300
@@ -45,7 +45,7 @@ proc check_fork_catchpoints {} {
   gdb_test "catch fork" "Catchpoint \[0-9\]* \\(fork\\)" "insert first fork catchpoint"
   set has_fork_catchpoints 0
   gdb_test_multiple "continue" "continue to first fork catchpoint" {
-    -re ".*Your system does not support fork catchpoints.*$gdb_prompt $" {
+    -re ".*Your system does not support this type of catchpoint.*$gdb_prompt $" {
       unsupported "continue to first fork catchpoint"
     }
     -re ".*Catchpoint.*$gdb_prompt $" {
Index: gdb.git/gdb/testsuite/gdb.base/foll-vfork.exp
===================================================================
--- gdb.git.orig/gdb/testsuite/gdb.base/foll-vfork.exp	2010-08-16 20:17:16.000000000 -0300
+++ gdb.git/gdb/testsuite/gdb.base/foll-vfork.exp	2010-08-16 20:17:23.000000000 -0300
@@ -74,7 +74,7 @@ proc check_vfork_catchpoints {} {
   gdb_test "catch vfork" "Catchpoint \[0-9\]* \\(vfork\\)" "insert first vfork catchpoint"
   set has_vfork_catchpoints 0
   gdb_test_multiple "continue" "continue to first vfork catchpoint" {
-    -re ".*Your system does not support vfork catchpoints.*$gdb_prompt $" {
+    -re ".*Your system does not support this type of catchpoint.*$gdb_prompt $" {
       unsupported "continue to first vfork catchpoint"
     }
     -re ".*Catchpoint.*$gdb_prompt $" {


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops
  2010-08-17 19:41 [patch 1/2] Convert hardware watchpoints to use breakpoint_ops Thiago Jung Bauermann
@ 2010-10-07 14:47 ` Thiago Jung Bauermann
  2010-10-16 17:43 ` Pedro Alves
  1 sibling, 0 replies; 24+ messages in thread
From: Thiago Jung Bauermann @ 2010-10-07 14:47 UTC (permalink / raw)
  To: gdb-patches ml

Hi everybody,

On Tue, 2010-08-17 at 16:41 -0300, Thiago Jung Bauermann wrote:
> Tested on ppc-linux, ppc64-linux and i686-linux with no regressions. Ok
> to commit?

Ping.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops
  2010-08-17 19:41 [patch 1/2] Convert hardware watchpoints to use breakpoint_ops Thiago Jung Bauermann
  2010-10-07 14:47 ` Thiago Jung Bauermann
@ 2010-10-16 17:43 ` Pedro Alves
  2010-10-20  0:31   ` Thiago Jung Bauermann
  1 sibling, 1 reply; 24+ messages in thread
From: Pedro Alves @ 2010-10-16 17:43 UTC (permalink / raw)
  To: gdb-patches; +Cc: Thiago Jung Bauermann

On Tuesday 17 August 2010 20:41:11, Thiago Jung Bauermann wrote:
>             if (!mem_cnt)
> -             b->type = bp_watchpoint;
> +             {
> +               b->type = bp_watchpoint;
> +
> +               /* When downgrading to a software watchpoint, we have to
> +                  remember the value of the OPS field so that we can restore
> +                  it when upgrading to a hardware watchpoint again.  */
> +               if (b->ops)
> +                 {
> +                   b->old_ops = b->ops;
> +                   b->ops = NULL;
> +                 }

IMO, if you needed this juggling, something is wrong with the design.  :-/

-- 
Pedro Alves

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops
  2010-10-16 17:43 ` Pedro Alves
@ 2010-10-20  0:31   ` Thiago Jung Bauermann
  2010-11-04 21:17     ` Thiago Jung Bauermann
                       ` (3 more replies)
  0 siblings, 4 replies; 24+ messages in thread
From: Thiago Jung Bauermann @ 2010-10-20  0:31 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

Hi Pedro,

On Sat, 2010-10-16 at 18:43 +0100, Pedro Alves wrote:
> On Tuesday 17 August 2010 20:41:11, Thiago Jung Bauermann wrote:
> >             if (!mem_cnt)
> > -             b->type = bp_watchpoint;
> > +             {
> > +               b->type = bp_watchpoint;
> > +
> > +               /* When downgrading to a software watchpoint, we have to
> > +                  remember the value of the OPS field so that we can restore
> > +                  it when upgrading to a hardware watchpoint again.  */
> > +               if (b->ops)
> > +                 {
> > +                   b->old_ops = b->ops;
> > +                   b->ops = NULL;
> > +                 }
> 
> IMO, if you needed this juggling, something is wrong with the design.  :-/

Thank you very much for your review!

That code is there because some methods in
ranged_watchpoints_breakpoint_ops and masked_watchpoints_breakpoint_ops
couldn't cope with b->type == bp_watchpoint.

Thinking more about it, ranged watchpoints work fine in software
watchpoint mode, and those methods can be easily adapted. As for masked
watchpoints, I don't think they can be implemented in software and I'll
have to add some logic to disable them if there are no watchpoint
registers available (probably a new "works_in_software_mode" field in
breakpoint_ops to be called in update_watchpoint).

Here's a new version of this patch without the old_ops juggling. I'll
send the updated version of the 2nd patch in the series in a couple of
days.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


2010-10-19  Thiago Jung Bauermann  <bauerman@br.ibm.com>

	Convert hardware watchpoints to use breakpoint_ops.

gdb/
	* breakpoint.h (breakpoint_ops) <insert>: Return int instead of void.
	Accept pointer to struct bp_location instead of pointer to
	struct breakpoint.  Adapt all implementations.
	(breakpoint_ops) <remove>: Accept pointer to struct bp_location instead
	of pointer to struct breakpoint.  Adapt all implementations.
	* breakpoint.c (insert_catchpoint): Delete function.
	(insert_bp_location): Call the watchpoint or catchpoint's
	breakpoint_ops.insert method.
	(remove_breakpoint_1): Call the watchpoint or catchpoint's
	breakpoint_ops.remove method.
	(insert_watchpoint, remove_watchpoint): New functions.
	(watchpoint_breakpoint_ops): New structure.
	(watch_command_1): Initialize the OPS field.
	* inf-child.c (inf_child_insert_fork_catchpoint)
	(inf_child_remove_fork_catchpoint, inf_child_insert_vfork_catchpoint)
	(inf_child_remove_vfork_catchpoint, inf_child_insert_exec_catchpoint)
	(inf_child_remove_exec_catchpoint, inf_child_set_syscall_catchpoint):
	Delete functions.
	(inf_child_target): Remove initialization of to_insert_fork_catchpoint,
	to_remove_fork_catchpoint, to_insert_vfork_catchpoint,
	to_remove_vfork_catchpoint, to_insert_exec_catchpoint,
	to_remove_exec_catchpoint and to_set_syscall_catchpoint.
	* target.c (update_current_target): Change default implementation of
	to_insert_fork_catchpoint, to_remove_fork_catchpoint,
	to_insert_vfork_catchpoint, to_remove_vfork_catchpoint,
	to_insert_exec_catchpoint, to_remove_exec_catchpoint and
	to_set_syscall_catchpoint to return_one.
	(debug_to_insert_fork_catchpoint, debug_to_insert_vfork_catchpoint)
	(debug_to_insert_exec_catchpoint): Report return value.
	* target.h (to_insert_fork_catchpoint, to_insert_vfork_catchpoint)
	(to_insert_fork_catchpoint): Change declaration to return int instead
	of void.

gdb/testsuite/
	* gdb.base/foll-exec.exp: Adapt to new error string when the catchpoint
	type is not supported.
	* gdb.base/foll-fork.exp: Likewise.
	* gdb.base/foll-vfork.exp: Likewise.


Index: gdb.git/gdb/breakpoint.c
===================================================================
--- gdb.git.orig/gdb/breakpoint.c	2010-10-13 15:59:12.000000000 -0300
+++ gdb.git/gdb/breakpoint.c	2010-10-19 21:08:20.000000000 -0200
@@ -1191,18 +1191,6 @@ breakpoint_restore_shadows (gdb_byte *bu
 }
 \f
 
-/* A wrapper function for inserting catchpoints.  */
-static void
-insert_catchpoint (struct ui_out *uo, void *args)
-{
-  struct breakpoint *b = (struct breakpoint *) args;
-
-  gdb_assert (b->type == bp_catchpoint);
-  gdb_assert (b->ops != NULL && b->ops->insert != NULL);
-
-  b->ops->insert (b);
-}
-
 /* Return true if BPT is of any hardware watchpoint kind.  */
 
 static int
@@ -1739,10 +1727,7 @@ Note: automatically using hardware break
 	      watchpoints.  It's not clear that it's necessary... */
 	   && bpt->owner->disposition != disp_del_at_next_stop)
     {
-      val = target_insert_watchpoint (bpt->address,
-				      bpt->length,
-				      bpt->watchpoint_type,
-				      bpt->owner->cond_exp);
+      val = bpt->owner->ops->insert (bpt);
 
       /* If trying to set a read-watchpoint, and it turns out it's not
 	 supported, try emulating one with an access watchpoint.  */
@@ -1768,12 +1753,12 @@ Note: automatically using hardware break
 
 	  if (val == 1)
 	    {
-	      val = target_insert_watchpoint (bpt->address,
-					      bpt->length,
-					      hw_access,
-					      bpt->owner->cond_exp);
-	      if (val == 0)
-		bpt->watchpoint_type = hw_access;
+	      bpt->watchpoint_type = hw_access;
+	      val = bpt->owner->ops->insert (bpt);
+
+	      if (val)
+		/* Back to the original value.  */
+		bpt->watchpoint_type = hw_read;
 	    }
 	}
 
@@ -1782,14 +1767,22 @@ Note: automatically using hardware break
 
   else if (bpt->owner->type == bp_catchpoint)
     {
-      struct gdb_exception e = catch_exception (uiout, insert_catchpoint,
-						bpt->owner, RETURN_MASK_ERROR);
-      exception_fprintf (gdb_stderr, e, "warning: inserting catchpoint %d: ",
-			 bpt->owner->number);
-      if (e.reason < 0)
-	bpt->owner->enable_state = bp_disabled;
-      else
-	bpt->inserted = 1;
+      gdb_assert (bpt->owner->ops != NULL && bpt->owner->ops->insert != NULL);
+
+      val = bpt->owner->ops->insert (bpt);
+      if (val)
+	{
+	  bpt->owner->enable_state = bp_disabled;
+
+	  if (val == 1)
+	    warning (_("\
+Inserting catchpoint %d: Your system does not support this type of catchpoint."),
+		     bpt->owner->number);
+	  else
+	    warning (_("Error inserting catchpoint %d."), bpt->owner->number);
+	}
+
+      bpt->inserted = (val == 0);
 
       /* We've already printed an error message if there was a problem
 	 inserting this catchpoint, and we've disabled the catchpoint,
@@ -2440,8 +2433,7 @@ remove_breakpoint_1 (struct bp_location 
   else if (b->loc_type == bp_loc_hardware_watchpoint)
     {
       b->inserted = (is == mark_inserted);
-      val = target_remove_watchpoint (b->address, b->length,
-				      b->watchpoint_type, b->owner->cond_exp);
+      b->owner->ops->remove (b);
 
       /* Failure to remove any of the hardware watchpoints comes here.  */
       if ((is == mark_uninserted) && (b->inserted))
@@ -2454,9 +2446,10 @@ remove_breakpoint_1 (struct bp_location 
     {
       gdb_assert (b->owner->ops != NULL && b->owner->ops->remove != NULL);
 
-      val = b->owner->ops->remove (b->owner);
+      val = b->owner->ops->remove (b);
       if (val)
 	return val;
+
       b->inserted = (is == mark_inserted);
     }
 
@@ -5838,16 +5831,16 @@ disable_breakpoints_in_unloaded_shlib (s
 
 /* Implement the "insert" breakpoint_ops method for fork catchpoints.  */
 
-static void
-insert_catch_fork (struct breakpoint *b)
+static int
+insert_catch_fork (struct bp_location *b)
 {
-  target_insert_fork_catchpoint (PIDGET (inferior_ptid));
+  return target_insert_fork_catchpoint (PIDGET (inferior_ptid));
 }
 
 /* Implement the "remove" breakpoint_ops method for fork catchpoints.  */
 
 static int
-remove_catch_fork (struct breakpoint *b)
+remove_catch_fork (struct bp_location *b)
 {
   return target_remove_fork_catchpoint (PIDGET (inferior_ptid));
 }
@@ -5930,16 +5923,16 @@ static struct breakpoint_ops catch_fork_
 
 /* Implement the "insert" breakpoint_ops method for vfork catchpoints.  */
 
-static void
-insert_catch_vfork (struct breakpoint *b)
+static int
+insert_catch_vfork (struct bp_location *b)
 {
-  target_insert_vfork_catchpoint (PIDGET (inferior_ptid));
+  return target_insert_vfork_catchpoint (PIDGET (inferior_ptid));
 }
 
 /* Implement the "remove" breakpoint_ops method for vfork catchpoints.  */
 
 static int
-remove_catch_vfork (struct breakpoint *b)
+remove_catch_vfork (struct bp_location *b)
 {
   return target_remove_vfork_catchpoint (PIDGET (inferior_ptid));
 }
@@ -6022,20 +6015,20 @@ static struct breakpoint_ops catch_vfork
 /* Implement the "insert" breakpoint_ops method for syscall
    catchpoints.  */
 
-static void
-insert_catch_syscall (struct breakpoint *b)
+static int
+insert_catch_syscall (struct bp_location *b)
 {
   struct inferior *inf = current_inferior ();
 
   ++inf->total_syscalls_count;
-  if (!b->syscalls_to_be_caught)
+  if (!b->owner->syscalls_to_be_caught)
     ++inf->any_syscall_count;
   else
     {
       int i, iter;
 
       for (i = 0;
-           VEC_iterate (int, b->syscalls_to_be_caught, i, iter);
+           VEC_iterate (int, b->owner->syscalls_to_be_caught, i, iter);
            i++)
 	{
           int elem;
@@ -6056,30 +6049,30 @@ insert_catch_syscall (struct breakpoint 
 	}
     }
 
-  target_set_syscall_catchpoint (PIDGET (inferior_ptid),
-				 inf->total_syscalls_count != 0,
-				 inf->any_syscall_count,
-				 VEC_length (int, inf->syscalls_counts),
-				 VEC_address (int, inf->syscalls_counts));
+  return target_set_syscall_catchpoint (PIDGET (inferior_ptid),
+					inf->total_syscalls_count != 0,
+					inf->any_syscall_count,
+					VEC_length (int, inf->syscalls_counts),
+					VEC_address (int, inf->syscalls_counts));
 }
 
 /* Implement the "remove" breakpoint_ops method for syscall
    catchpoints.  */
 
 static int
-remove_catch_syscall (struct breakpoint *b)
+remove_catch_syscall (struct bp_location *b)
 {
   struct inferior *inf = current_inferior ();
 
   --inf->total_syscalls_count;
-  if (!b->syscalls_to_be_caught)
+  if (!b->owner->syscalls_to_be_caught)
     --inf->any_syscall_count;
   else
     {
       int i, iter;
 
       for (i = 0;
-           VEC_iterate (int, b->syscalls_to_be_caught, i, iter);
+           VEC_iterate (int, b->owner->syscalls_to_be_caught, i, iter);
            i++)
 	{
           int elem;
@@ -6378,14 +6371,14 @@ create_fork_vfork_event_catchpoint (stru
 
 /* Exec catchpoints.  */
 
-static void
-insert_catch_exec (struct breakpoint *b)
+static int
+insert_catch_exec (struct bp_location *b)
 {
-  target_insert_exec_catchpoint (PIDGET (inferior_ptid));
+  return target_insert_exec_catchpoint (PIDGET (inferior_ptid));
 }
 
 static int
-remove_catch_exec (struct breakpoint *b)
+remove_catch_exec (struct bp_location *b)
 {
   return target_remove_exec_catchpoint (PIDGET (inferior_ptid));
 }
@@ -8013,6 +8006,37 @@ watchpoint_exp_is_const (const struct ex
   return 1;
 }
 
+/* Implement the "insert" breakpoint_ops method for hardware watchpoints.  */
+
+static int
+insert_watchpoint (struct bp_location *bpt)
+{
+  return target_insert_watchpoint (bpt->address, bpt->length,
+				   bpt->watchpoint_type, bpt->owner->cond_exp);
+}
+
+/* Implement the "remove" breakpoint_ops method for hardware watchpoints.  */
+
+static int
+remove_watchpoint (struct bp_location *bpt)
+{
+  return target_remove_watchpoint (bpt->address, bpt->length,
+				   bpt->watchpoint_type, bpt->owner->cond_exp);
+}
+
+/* The breakpoint_ops structure to be used in hardware watchpoints.  */
+
+static struct breakpoint_ops watchpoint_breakpoint_ops =
+{
+  insert_watchpoint,
+  remove_watchpoint,
+  NULL, /* breakpoint_hit */
+  NULL, /* print_it */
+  NULL, /* print_one */
+  NULL, /* print_mention */
+  NULL  /* print_recreate */
+};
+
 /* accessflag:  hw_write:  watch write, 
                 hw_read:   watch read, 
 		hw_access: watch access (read or write) */
@@ -8255,6 +8279,8 @@ watch_command_1 (char *arg, int accessfl
     b->exp_string = savestring (exp_start, exp_end - exp_start);
   b->val = val;
   b->val_valid = 1;
+  b->ops = &watchpoint_breakpoint_ops;
+
   if (cond_start)
     b->cond_string = savestring (cond_start, cond_end - cond_start);
   else
Index: gdb.git/gdb/breakpoint.h
===================================================================
--- gdb.git.orig/gdb/breakpoint.h	2010-08-25 14:10:16.000000000 -0300
+++ gdb.git/gdb/breakpoint.h	2010-10-19 21:08:20.000000000 -0200
@@ -344,16 +344,18 @@ struct bp_location
    will be called instead of the performing the default action for this
    bptype.  */
 
-struct breakpoint_ops 
+struct breakpoint_ops
 {
-  /* Insert the breakpoint or activate the catchpoint.  Should raise
-     an exception if the operation failed.  */
-  void (*insert) (struct breakpoint *);
+  /* Insert the breakpoint or watchpoint or activate the catchpoint.
+     Return 0 for success, 1 if the breakpoint, watchpoint or catchpoint
+     type is not supported, -1 for failure.  */
+  int (*insert) (struct bp_location *);
 
   /* Remove the breakpoint/catchpoint that was previously inserted
-     with the "insert" method above.  Return non-zero if the operation
-     succeeded.  */
-  int (*remove) (struct breakpoint *);
+     with the "insert" method above.  Return 0 for success, 1 if the
+     breakpoint, watchpoint or catchpoint type is not supported,
+     -1 for failure.  */
+  int (*remove) (struct bp_location *);
 
   /* Return non-zero if the debugger should tell the user that this
      breakpoint was hit.  */
Index: gdb.git/gdb/inf-child.c
===================================================================
--- gdb.git.orig/gdb/inf-child.c	2010-08-20 16:00:16.000000000 -0300
+++ gdb.git/gdb/inf-child.c	2010-10-19 21:08:21.000000000 -0200
@@ -94,36 +94,6 @@ inf_child_acknowledge_created_inferior (
      created inferior" operation by a debugger.  */
 }
 
-static void
-inf_child_insert_fork_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of fork
-     events.  */
-}
-
-static int
-inf_child_remove_fork_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of fork
-     events.  */
-  return 0;
-}
-
-static void
-inf_child_insert_vfork_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of vfork
-     events.  */
-}
-
-static int
-inf_child_remove_vfork_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of vfork
-     events.  */
-  return 0;
-}
-
 static int
 inf_child_follow_fork (struct target_ops *ops, int follow_child)
 {
@@ -132,30 +102,6 @@ inf_child_follow_fork (struct target_ops
   return 0;
 }
 
-static void
-inf_child_insert_exec_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of exec
-     events.  */
-}
-
-static int
-inf_child_remove_exec_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of exec
-     events.  */
-  return 0;
-}
-
-static int
-inf_child_set_syscall_catchpoint (int pid, int needed, int any_count,
-				  int table_size, int *table)
-{
-  /* This version of Unix doesn't support notification of syscall
-     events.  */
-  return 0;
-}
-
 static int
 inf_child_can_run (void)
 {
@@ -193,14 +139,7 @@ inf_child_target (void)
   t->to_terminal_info = child_terminal_info;
   t->to_post_startup_inferior = inf_child_post_startup_inferior;
   t->to_acknowledge_created_inferior = inf_child_acknowledge_created_inferior;
-  t->to_insert_fork_catchpoint = inf_child_insert_fork_catchpoint;
-  t->to_remove_fork_catchpoint = inf_child_remove_fork_catchpoint;
-  t->to_insert_vfork_catchpoint = inf_child_insert_vfork_catchpoint;
-  t->to_remove_vfork_catchpoint = inf_child_remove_vfork_catchpoint;
   t->to_follow_fork = inf_child_follow_fork;
-  t->to_insert_exec_catchpoint = inf_child_insert_exec_catchpoint;
-  t->to_remove_exec_catchpoint = inf_child_remove_exec_catchpoint;
-  t->to_set_syscall_catchpoint = inf_child_set_syscall_catchpoint;
   t->to_can_run = inf_child_can_run;
   t->to_pid_to_exec_file = inf_child_pid_to_exec_file;
   t->to_stratum = process_stratum;
Index: gdb.git/gdb/linux-nat.c
===================================================================
--- gdb.git.orig/gdb/linux-nat.c	2010-10-19 21:04:59.000000000 -0200
+++ gdb.git/gdb/linux-nat.c	2010-10-19 21:08:21.000000000 -0200
@@ -961,36 +961,34 @@ Attaching after process %d fork to child
 }
 
 \f
-static void
+static int
 linux_child_insert_fork_catchpoint (int pid)
 {
-  if (! linux_supports_tracefork (pid))
-    error (_("Your system does not support fork catchpoints."));
+  return !linux_supports_tracefork (pid);
 }
 
-static void
+static int
 linux_child_insert_vfork_catchpoint (int pid)
 {
-  if (!linux_supports_tracefork (pid))
-    error (_("Your system does not support vfork catchpoints."));
+  return !linux_supports_tracefork (pid);
 }
 
-static void
+static int
 linux_child_insert_exec_catchpoint (int pid)
 {
-  if (!linux_supports_tracefork (pid))
-    error (_("Your system does not support exec catchpoints."));
+  return !linux_supports_tracefork (pid);
 }
 
 static int
 linux_child_set_syscall_catchpoint (int pid, int needed, int any_count,
 				    int table_size, int *table)
 {
-  if (! linux_supports_tracesysgood (pid))
-    error (_("Your system does not support syscall catchpoints."));
+  if (!linux_supports_tracesysgood (pid))
+    return 1;
+
   /* On GNU/Linux, we ignore the arguments.  It means that we only
      enable the syscall catchpoints, but do not disable them.
-     
+
      Also, we do not use the `table' information because we do not
      filter system calls here.  We let GDB do the logic for us.  */
   return 0;
Index: gdb.git/gdb/target.c
===================================================================
--- gdb.git.orig/gdb/target.c	2010-10-13 15:59:13.000000000 -0300
+++ gdb.git/gdb/target.c	2010-10-19 21:08:21.000000000 -0200
@@ -782,26 +782,26 @@ update_current_target (void)
 	    (void (*) (int))
 	    target_ignore);
   de_fault (to_insert_fork_catchpoint,
-	    (void (*) (int))
-	    tcomplain);
+	    (int (*) (int))
+	    return_one);
   de_fault (to_remove_fork_catchpoint,
 	    (int (*) (int))
-	    tcomplain);
+	    return_one);
   de_fault (to_insert_vfork_catchpoint,
-	    (void (*) (int))
-	    tcomplain);
+	    (int (*) (int))
+	    return_one);
   de_fault (to_remove_vfork_catchpoint,
 	    (int (*) (int))
-	    tcomplain);
+	    return_one);
   de_fault (to_insert_exec_catchpoint,
-	    (void (*) (int))
-	    tcomplain);
+	    (int (*) (int))
+	    return_one);
   de_fault (to_remove_exec_catchpoint,
 	    (int (*) (int))
-	    tcomplain);
+	    return_one);
   de_fault (to_set_syscall_catchpoint,
 	    (int (*) (int, int, int, int, int *))
-	    tcomplain);
+	    return_one);
   de_fault (to_has_exited,
 	    (int (*) (int, int, int *))
 	    return_zero);
@@ -3657,13 +3657,17 @@ debug_to_acknowledge_created_inferior (i
 		      pid);
 }
 
-static void
+static int
 debug_to_insert_fork_catchpoint (int pid)
 {
-  debug_target.to_insert_fork_catchpoint (pid);
+  int retval;
 
-  fprintf_unfiltered (gdb_stdlog, "target_insert_fork_catchpoint (%d)\n",
-		      pid);
+  retval = debug_target.to_insert_fork_catchpoint (pid);
+
+  fprintf_unfiltered (gdb_stdlog, "target_insert_fork_catchpoint (%d) = %d\n",
+		      pid, retval);
+
+  return retval;
 }
 
 static int
@@ -3679,13 +3683,17 @@ debug_to_remove_fork_catchpoint (int pid
   return retval;
 }
 
-static void
+static int
 debug_to_insert_vfork_catchpoint (int pid)
 {
-  debug_target.to_insert_vfork_catchpoint (pid);
+  int retval;
 
-  fprintf_unfiltered (gdb_stdlog, "target_insert_vfork_catchpoint (%d)\n",
-		      pid);
+  retval = debug_target.to_insert_vfork_catchpoint (pid);
+
+  fprintf_unfiltered (gdb_stdlog, "target_insert_vfork_catchpoint (%d) = %d\n",
+		      pid, retval);
+
+  return retval;
 }
 
 static int
@@ -3701,13 +3709,17 @@ debug_to_remove_vfork_catchpoint (int pi
   return retval;
 }
 
-static void
+static int
 debug_to_insert_exec_catchpoint (int pid)
 {
-  debug_target.to_insert_exec_catchpoint (pid);
+  int retval;
 
-  fprintf_unfiltered (gdb_stdlog, "target_insert_exec_catchpoint (%d)\n",
-		      pid);
+  retval = debug_target.to_insert_exec_catchpoint (pid);
+
+  fprintf_unfiltered (gdb_stdlog, "target_insert_exec_catchpoint (%d) = %d\n",
+		      pid, retval);
+
+  return retval;
 }
 
 static int
Index: gdb.git/gdb/target.h
===================================================================
--- gdb.git.orig/gdb/target.h	2010-10-13 15:59:13.000000000 -0300
+++ gdb.git/gdb/target.h	2010-10-19 21:08:21.000000000 -0200
@@ -468,12 +468,12 @@ struct target_ops
 				char *, char *, char **, int);
     void (*to_post_startup_inferior) (ptid_t);
     void (*to_acknowledge_created_inferior) (int);
-    void (*to_insert_fork_catchpoint) (int);
+    int (*to_insert_fork_catchpoint) (int);
     int (*to_remove_fork_catchpoint) (int);
-    void (*to_insert_vfork_catchpoint) (int);
+    int (*to_insert_vfork_catchpoint) (int);
     int (*to_remove_vfork_catchpoint) (int);
     int (*to_follow_fork) (struct target_ops *, int);
-    void (*to_insert_exec_catchpoint) (int);
+    int (*to_insert_exec_catchpoint) (int);
     int (*to_remove_exec_catchpoint) (int);
     int (*to_set_syscall_catchpoint) (int, int, int, int, int *);
     int (*to_has_exited) (int, int, int *);
@@ -1083,7 +1083,10 @@ int target_follow_fork (int follow_child
 
    TABLE is an array of ints, indexed by syscall number.  An element in
    this array is nonzero if that syscall should be caught.  This argument
-   only matters if ANY_COUNT is zero.  */
+   only matters if ANY_COUNT is zero.
+
+   Return 0 for success, 1 if syscall catchpoints are not supported or -1
+   for failure.  */
 
 #define target_set_syscall_catchpoint(pid, needed, any_count, table_size, table) \
      (*current_target.to_set_syscall_catchpoint) (pid, needed, any_count, \
Index: gdb.git/gdb/testsuite/gdb.base/foll-exec.exp
===================================================================
--- gdb.git.orig/gdb/testsuite/gdb.base/foll-exec.exp	2010-08-20 16:00:16.000000000 -0300
+++ gdb.git/gdb/testsuite/gdb.base/foll-exec.exp	2010-10-19 21:08:21.000000000 -0200
@@ -89,7 +89,7 @@ proc do_exec_tests {} {
    gdb_test "catch exec" "Catchpoint \[0-9\]* \\(exec\\)" "insert first exec catchpoint"
    set has_exec_catchpoints 0
    gdb_test_multiple "continue" "continue to first exec catchpoint" {
-     -re ".*Your system does not support exec catchpoints.*$gdb_prompt $" {
+     -re ".*Your system does not support this type of catchpoint.*$gdb_prompt $" {
        unsupported "continue to first exec catchpoint"
      }
      -re ".*Catchpoint.*$gdb_prompt $" {
Index: gdb.git/gdb/testsuite/gdb.base/foll-fork.exp
===================================================================
--- gdb.git.orig/gdb/testsuite/gdb.base/foll-fork.exp	2010-10-19 21:05:00.000000000 -0200
+++ gdb.git/gdb/testsuite/gdb.base/foll-fork.exp	2010-10-19 21:08:21.000000000 -0200
@@ -45,7 +45,7 @@ proc check_fork_catchpoints {} {
   gdb_test "catch fork" "Catchpoint \[0-9\]* \\(fork\\)" "insert first fork catchpoint"
   set has_fork_catchpoints 0
   gdb_test_multiple "continue" "continue to first fork catchpoint" {
-    -re ".*Your system does not support fork catchpoints.*$gdb_prompt $" {
+    -re ".*Your system does not support this type of catchpoint.*$gdb_prompt $" {
       unsupported "continue to first fork catchpoint"
     }
     -re ".*Catchpoint.*$gdb_prompt $" {
Index: gdb.git/gdb/testsuite/gdb.base/foll-vfork.exp
===================================================================
--- gdb.git.orig/gdb/testsuite/gdb.base/foll-vfork.exp	2010-08-20 16:00:16.000000000 -0300
+++ gdb.git/gdb/testsuite/gdb.base/foll-vfork.exp	2010-10-19 21:08:21.000000000 -0200
@@ -74,7 +74,7 @@ proc check_vfork_catchpoints {} {
   gdb_test "catch vfork" "Catchpoint \[0-9\]* \\(vfork\\)" "insert first vfork catchpoint"
   set has_vfork_catchpoints 0
   gdb_test_multiple "continue" "continue to first vfork catchpoint" {
-    -re ".*Your system does not support vfork catchpoints.*$gdb_prompt $" {
+    -re ".*Your system does not support this type of catchpoint.*$gdb_prompt $" {
       unsupported "continue to first vfork catchpoint"
     }
     -re ".*Catchpoint.*$gdb_prompt $" {


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops
  2010-10-20  0:31   ` Thiago Jung Bauermann
@ 2010-11-04 21:17     ` Thiago Jung Bauermann
  2010-11-08 18:43     ` Joel Brobecker
                       ` (2 subsequent siblings)
  3 siblings, 0 replies; 24+ messages in thread
From: Thiago Jung Bauermann @ 2010-11-04 21:17 UTC (permalink / raw)
  To: gdb-patches

On Tue, 2010-10-19 at 22:31 -0200, Thiago Jung Bauermann wrote:
> Here's a new version of this patch without the old_ops juggling. I'll
> send the updated version of the 2nd patch in the series in a couple of
> days.

Ping.

The updated version of the 2nd patch in the series is out already, and
has the documentation bits approved.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops
  2010-10-20  0:31   ` Thiago Jung Bauermann
  2010-11-04 21:17     ` Thiago Jung Bauermann
@ 2010-11-08 18:43     ` Joel Brobecker
  2010-11-08 21:39       ` Thiago Jung Bauermann
  2010-11-15 22:23     ` Joel Brobecker
  2010-11-16  4:06     ` [patch 1/2] Convert hardware watchpoints to use breakpoint_ops Jan Kratochvil
  3 siblings, 1 reply; 24+ messages in thread
From: Joel Brobecker @ 2010-11-08 18:43 UTC (permalink / raw)
  To: Thiago Jung Bauermann; +Cc: gdb-patches

Just for the record, I've started looking at both patches, and already
spent a good hour on them. But I think that I should give myself some
time to to let every piece sink in, before I make an official review.

I urge another one of us to look at this change, because it's not
completely obvious. In particular, I think it was my suggestion
that prompted the move to using breakpoint_ops for watchpoints.
But it's not obvious that there is a gain.  I think the gain shows up
in the second patch, where we avoid having:
  
  1. To define new kinds for range and mask watchpoints

  2. To program dispatching manually, Eg:

     if (bp->print_one)
       bp->print_one (...)
     else if (bp->type == hw_watchpoint)
       print_one_normal_watchpoint (...)
     else if (bp->type == range_watchpoint)
       print_one_range_watchpoint (...)
     else if (bp->type == mask_watchpoint)
       [...]


-- 
Joel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops
  2010-11-08 18:43     ` Joel Brobecker
@ 2010-11-08 21:39       ` Thiago Jung Bauermann
  0 siblings, 0 replies; 24+ messages in thread
From: Thiago Jung Bauermann @ 2010-11-08 21:39 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: gdb-patches

On Mon, 2010-11-08 at 10:43 -0800, Joel Brobecker wrote:
> Just for the record, I've started looking at both patches, and already
> spent a good hour on them. But I think that I should give myself some
> time to to let every piece sink in, before I make an official review.

Thank you very much! I'll be glad to clarify any question that may come
up during your review of the patch. No need to think too hard about what
the patch does in some obscure part, I'll be glad to explain. :-)

> I urge another one of us to look at this change, because it's not
> completely obvious. In particular, I think it was my suggestion
> that prompted the move to using breakpoint_ops for watchpoints.
> But it's not obvious that there is a gain.  I think the gain shows up
> in the second patch, where we avoid having:

Yes, I think the 2nd patch became cleaner when I modified it to use
breakpoint_ops. And yes, it was your idea. :-)

IMHO the code would be even cleaner if I/we ported the existing
breakpoints and watchpoints code completely to breakpoint_ops,
eliminating functions like print_it_typical. It would make this part of
GDB more object-oriented. Though it would perhaps increase the total
line count instead of decreasing it, so it's not a clear win.
 
>   1. To define new kinds for range and mask watchpoints
> 
>   2. To program dispatching manually, Eg:
> 
>      if (bp->print_one)
>        bp->print_one (...)
>      else if (bp->type == hw_watchpoint)
>        print_one_normal_watchpoint (...)
>      else if (bp->type == range_watchpoint)
>        print_one_range_watchpoint (...)
>      else if (bp->type == mask_watchpoint)
>        [...]

It also avoids adding special cases just for masked and ranged
watchpoints. Instead, I added a method to breakpoints_op and if it's
defined, I just call it. For instance, in update_watchpoint:

+               if (b->ops && b->ops->extra_resources_needed)
+                 mem_cnt += b->ops->extra_resources_needed (b);


-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops
  2010-10-20  0:31   ` Thiago Jung Bauermann
  2010-11-04 21:17     ` Thiago Jung Bauermann
  2010-11-08 18:43     ` Joel Brobecker
@ 2010-11-15 22:23     ` Joel Brobecker
  2010-11-16 19:03       ` Thiago Jung Bauermann
  2010-11-16  4:06     ` [patch 1/2] Convert hardware watchpoints to use breakpoint_ops Jan Kratochvil
  3 siblings, 1 reply; 24+ messages in thread
From: Joel Brobecker @ 2010-11-15 22:23 UTC (permalink / raw)
  To: Thiago Jung Bauermann; +Cc: gdb-patches

> 2010-10-19  Thiago Jung Bauermann  <bauerman@br.ibm.com>
> 
> 	Convert hardware watchpoints to use breakpoint_ops.
> 
> gdb/
> 	* breakpoint.h (breakpoint_ops) <insert>: Return int instead of void.
> 	Accept pointer to struct bp_location instead of pointer to
> 	struct breakpoint.  Adapt all implementations.
> 	(breakpoint_ops) <remove>: Accept pointer to struct bp_location instead
> 	of pointer to struct breakpoint.  Adapt all implementations.
> 	* breakpoint.c (insert_catchpoint): Delete function.
> 	(insert_bp_location): Call the watchpoint or catchpoint's
> 	breakpoint_ops.insert method.
> 	(remove_breakpoint_1): Call the watchpoint or catchpoint's
> 	breakpoint_ops.remove method.
> 	(insert_watchpoint, remove_watchpoint): New functions.
> 	(watchpoint_breakpoint_ops): New structure.
> 	(watch_command_1): Initialize the OPS field.
> 	* inf-child.c (inf_child_insert_fork_catchpoint)
> 	(inf_child_remove_fork_catchpoint, inf_child_insert_vfork_catchpoint)
> 	(inf_child_remove_vfork_catchpoint, inf_child_insert_exec_catchpoint)
> 	(inf_child_remove_exec_catchpoint, inf_child_set_syscall_catchpoint):
> 	Delete functions.
> 	(inf_child_target): Remove initialization of to_insert_fork_catchpoint,
> 	to_remove_fork_catchpoint, to_insert_vfork_catchpoint,
> 	to_remove_vfork_catchpoint, to_insert_exec_catchpoint,
> 	to_remove_exec_catchpoint and to_set_syscall_catchpoint.
> 	* target.c (update_current_target): Change default implementation of
> 	to_insert_fork_catchpoint, to_remove_fork_catchpoint,
> 	to_insert_vfork_catchpoint, to_remove_vfork_catchpoint,
> 	to_insert_exec_catchpoint, to_remove_exec_catchpoint and
> 	to_set_syscall_catchpoint to return_one.
> 	(debug_to_insert_fork_catchpoint, debug_to_insert_vfork_catchpoint)
> 	(debug_to_insert_exec_catchpoint): Report return value.
> 	* target.h (to_insert_fork_catchpoint, to_insert_vfork_catchpoint)
> 	(to_insert_fork_catchpoint): Change declaration to return int instead
> 	of void.
> 
> gdb/testsuite/
> 	* gdb.base/foll-exec.exp: Adapt to new error string when the catchpoint
> 	type is not supported.
> 	* gdb.base/foll-fork.exp: Likewise.
> 	* gdb.base/foll-vfork.exp: Likewise.

I'm OK with this patch.  Just a possible suggestion below...
Can you wait a couple more days to give anyone one last chance
at making comments on this patch? After that, please go ahead
and commit.

> +	  if (val == 1)
> +	    warning (_("\
> +Inserting catchpoint %d: Your system does not support this type of catchpoint."),
> +		     bpt->owner->number);
> +	  else
> +	    warning (_("Error inserting catchpoint %d."), bpt->owner->number);

Just curious: Why not say "Error inserting catchpoint %d" in both cases
(we would still keep the ": <not supported>" part in the first case)?

> -    void (*to_insert_fork_catchpoint) (int);
> +    int (*to_insert_fork_catchpoint) (int);
>      int (*to_remove_fork_catchpoint) (int);
> -    void (*to_insert_vfork_catchpoint) (int);
> +    int (*to_insert_vfork_catchpoint) (int);
>      int (*to_remove_vfork_catchpoint) (int);
>      int (*to_follow_fork) (struct target_ops *, int);
> -    void (*to_insert_exec_catchpoint) (int);
> +    int (*to_insert_exec_catchpoint) (int);
>      int (*to_remove_exec_catchpoint) (int);
>      int (*to_set_syscall_catchpoint) (int, int, int, int, int *);

I think we really should be documenting at least the return value.
Apparently, at least some of these "method" are documented though
their associated "target_<...>" macro/function.  I'd rather see
that documentation next to the method rather than the macro, but
that's for another discussion.


-- 
Joel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops
  2010-10-20  0:31   ` Thiago Jung Bauermann
                       ` (2 preceding siblings ...)
  2010-11-15 22:23     ` Joel Brobecker
@ 2010-11-16  4:06     ` Jan Kratochvil
  2010-11-16  8:07       ` Joel Brobecker
  3 siblings, 1 reply; 24+ messages in thread
From: Jan Kratochvil @ 2010-11-16  4:06 UTC (permalink / raw)
  To: Thiago Jung Bauermann; +Cc: Pedro Alves, gdb-patches

On Wed, 20 Oct 2010 02:31:31 +0200, Thiago Jung Bauermann wrote:
> -static void
> -insert_catch_fork (struct breakpoint *b)
> +static int
> +insert_catch_fork (struct bp_location *b)

Such variables (across the whole patch) should be really renamed when changing
its type.

I understand there are already cases of such incorrect naming in GDB but we
should not make it worse.

Also were these functions intended per-breakpoint or per-bp_location?
It looks to me currently they are used only for single-location breakpoint so
no one knows.  (I guess they were meant for breakpoint.)


> -struct breakpoint_ops 
> +struct breakpoint_ops
>  {
> -  /* Insert the breakpoint or activate the catchpoint.  Should raise
> -     an exception if the operation failed.  */
> -  void (*insert) (struct breakpoint *);
> +  /* Insert the breakpoint or watchpoint or activate the catchpoint.
> +     Return 0 for success, 1 if the breakpoint, watchpoint or catchpoint
> +     type is not supported, -1 for failure.  */
> +  int (*insert) (struct bp_location *);
>  
>    /* Remove the breakpoint/catchpoint that was previously inserted
> -     with the "insert" method above.  Return non-zero if the operation
> -     succeeded.  */
> -  int (*remove) (struct breakpoint *);
> +     with the "insert" method above.  Return 0 for success, 1 if the
> +     breakpoint, watchpoint or catchpoint type is not supported,
> +     -1 for failure.  */
> +  int (*remove) (struct bp_location *);

At least rename it to insert_bploc (or insert_location etc.).  This will need
to be cleaned up with the regular breakpoints/watchpoints conversion to
breakpoint_ops.



Thanks,
Jan

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops
  2010-11-16  4:06     ` [patch 1/2] Convert hardware watchpoints to use breakpoint_ops Jan Kratochvil
@ 2010-11-16  8:07       ` Joel Brobecker
  2010-11-16 18:51         ` Jan Kratochvil
  0 siblings, 1 reply; 24+ messages in thread
From: Joel Brobecker @ 2010-11-16  8:07 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: Thiago Jung Bauermann, Pedro Alves, gdb-patches

> > -static void
> > -insert_catch_fork (struct breakpoint *b)
> > +static int
> > +insert_catch_fork (struct bp_location *b)
> 
> Such variables (across the whole patch) should be really renamed when
> changing its type.

How about doing such a rename as a patch on its own?  I've spent
a fair amount of time thinking about this patch, and I'd rather not
have to review it again if the changes are only going to be minor
and not affect behavior.

I don't have any issue with blocking checkin of this patch until patches
implementing your suggestions are approved, if that helps.  I just
would rather avoid having to re-review the same patch again. Knowing
how git works, this shouldn't be very hard to do.

(notice: IIRC, when I first looked at patch #2, one of my reactions
is that I wanted to see the patch split-up in several smaller pieces;
I will explain that when I get to that patch)

> Also were these functions intended per-breakpoint or per-bp_location?
> It looks to me currently they are used only for single-location
> breakpoint so no one knows.  (I guess they were meant for breakpoint.)

I think that eventually, we want them to be per bp-location.  It does
not matter right now, as you say, since they are only used for single-
location breakpoints.

That's a good point, though. Perhaps a documentation update can help
make that clearer. OK with a separate patch?

> > -  void (*insert) (struct breakpoint *);
> > +  /* Insert the breakpoint or watchpoint or activate the catchpoint.
> > +     Return 0 for success, 1 if the breakpoint, watchpoint or catchpoint
> > +     type is not supported, -1 for failure.  */
> > +  int (*insert) (struct bp_location *);
[...]
> At least rename it to insert_bploc (or insert_location etc.).  This
> will need to be cleaned up with the regular breakpoints/watchpoints
> conversion to breakpoint_ops.

I don't feel that strongly about it. I don't feel that renaming
"insert" that takes a bp_loc into "insert_bploc" is going to help
much. But I'm OK.  I am suggesting that we push that as a followup
patch, if that's OK with you, just to ease review of that change
alone.

-- 
Joel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops
  2010-11-16  8:07       ` Joel Brobecker
@ 2010-11-16 18:51         ` Jan Kratochvil
  2010-11-17  3:47           ` [patch] Renaming: {insert,remove} += _location [Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops] Jan Kratochvil
  2010-11-17  3:47           ` [patch] renaming: bp_location: b->bl &co. " Jan Kratochvil
  0 siblings, 2 replies; 24+ messages in thread
From: Jan Kratochvil @ 2010-11-16 18:51 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: Thiago Jung Bauermann, Pedro Alves, gdb-patches

On Tue, 16 Nov 2010 09:07:21 +0100, Joel Brobecker wrote:
> How about doing such a rename as a patch on its own?

OK, going to send one today.


> I don't have any issue with blocking checkin of this patch until patches
> implementing your suggestions are approved, if that helps.

If one uses some IDE there is probably not a problem with the variable->type
association so I do not request a patch block.


> That's a good point, though. Perhaps a documentation update can help
> make that clearer. OK with a separate patch?

It should be made somewhere clear when it is bpt and when it is bp-loc.


Thanks,
Jan

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops
  2010-11-15 22:23     ` Joel Brobecker
@ 2010-11-16 19:03       ` Thiago Jung Bauermann
  2010-11-18 17:18         ` Joel Brobecker
  0 siblings, 1 reply; 24+ messages in thread
From: Thiago Jung Bauermann @ 2010-11-16 19:03 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: gdb-patches

Hi Joel,

Thanks for the review!

On Mon, 2010-11-15 at 14:23 -0800, Joel Brobecker wrote:
> I'm OK with this patch.  Just a possible suggestion below...
> Can you wait a couple more days to give anyone one last chance
> at making comments on this patch? After that, please go ahead
> and commit.

Ok. This patch doesn't do much on its own, so I'll commit it just when
the masked and ranged watchpoints patch is approved.

> > +	  if (val == 1)
> > +	    warning (_("\
> > +Inserting catchpoint %d: Your system does not support this type of catchpoint."),
> > +		     bpt->owner->number);
> > +	  else
> > +	    warning (_("Error inserting catchpoint %d."), bpt->owner->number);
> 
> Just curious: Why not say "Error inserting catchpoint %d" in both cases
> (we would still keep the ": <not supported>" part in the first case)?

I kept the wording as similar as possible to the original. Your
suggestion is then to have the code below instead?

+	  if (val == 1)
+	    warning (_("\
+Error inserting catchpoint %d: Your system does not support this type of catchpoint."),
+		     bpt->owner->number);
+	  else
+	    warning (_("Error inserting catchpoint %d."), bpt->owner->number);

I think it's good.

> > -    void (*to_insert_fork_catchpoint) (int);
> > +    int (*to_insert_fork_catchpoint) (int);
> >      int (*to_remove_fork_catchpoint) (int);
> > -    void (*to_insert_vfork_catchpoint) (int);
> > +    int (*to_insert_vfork_catchpoint) (int);
> >      int (*to_remove_vfork_catchpoint) (int);
> >      int (*to_follow_fork) (struct target_ops *, int);
> > -    void (*to_insert_exec_catchpoint) (int);
> > +    int (*to_insert_exec_catchpoint) (int);
> >      int (*to_remove_exec_catchpoint) (int);
> >      int (*to_set_syscall_catchpoint) (int, int, int, int, int *);
> 
> I think we really should be documenting at least the return value.
> Apparently, at least some of these "method" are documented though
> their associated "target_<...>" macro/function.  I'd rather see
> that documentation next to the method rather than the macro, but
> that's for another discussion.

What about these additional comments (I'll send the updated patch after
understanding your suggestion above)?

diff --git a/gdb/target.h b/gdb/target.h
index 54a6747..f4395c0 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -476,6 +476,11 @@ struct target_ops
 				char *, char *, char **, int);
     void (*to_post_startup_inferior) (ptid_t);
     void (*to_acknowledge_created_inferior) (int);
+
+
+    /* The insert and remove catchpoint functions return 0 for success,
+       1 if the watchpoint type is not supported and -1 for failure.  */
+
     int (*to_insert_fork_catchpoint) (int);
     int (*to_remove_fork_catchpoint) (int);
     int (*to_insert_vfork_catchpoint) (int);
@@ -484,6 +489,7 @@ struct target_ops
     int (*to_insert_exec_catchpoint) (int);
     int (*to_remove_exec_catchpoint) (int);
     int (*to_set_syscall_catchpoint) (int, int, int, int, int *);
+
     int (*to_has_exited) (int, int, int *);
     void (*to_mourn_inferior) (struct target_ops *);
     int (*to_can_run) (void);
@@ -1042,7 +1048,8 @@ void target_create_inferior (char *exec_file, char *args,
 
 /* On some targets, we can catch an inferior fork or vfork event when
    it occurs.  These functions insert/remove an already-created
-   catchpoint for such events.  */
+   catchpoint for such events.  They return  0 for success, 1 if the
+   catchpoint type is not supported and -1 for failure.  */
 
 #define target_insert_fork_catchpoint(pid) \
      (*current_target.to_insert_fork_catchpoint) (pid)
@@ -1068,7 +1075,8 @@ int target_follow_fork (int follow_child);
 
 /* On some targets, we can catch an inferior exec event when it
    occurs.  These functions insert/remove an already-created
-   catchpoint for such events.  */
+   catchpoint for such events.  They return  0 for success, 1 if the
+   catchpoint type is not supported and -1 for failure.  */
 
 #define target_insert_exec_catchpoint(pid) \
      (*current_target.to_insert_exec_catchpoint) (pid)


-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [patch] renaming: bp_location: b->bl &co.  [Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops]
  2010-11-16 18:51         ` Jan Kratochvil
  2010-11-17  3:47           ` [patch] Renaming: {insert,remove} += _location [Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops] Jan Kratochvil
@ 2010-11-17  3:47           ` Jan Kratochvil
  2010-11-18 17:15             ` Joel Brobecker
  2010-12-23 18:50             ` Thiago Jung Bauermann
  1 sibling, 2 replies; 24+ messages in thread
From: Jan Kratochvil @ 2010-11-17  3:47 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: Thiago Jung Bauermann, Pedro Alves, gdb-patches

On Tue, 16 Nov 2010 19:51:01 +0100, Jan Kratochvil wrote:
> On Tue, 16 Nov 2010 09:07:21 +0100, Joel Brobecker wrote:
> > How about doing such a rename as a patch on its own?
> 
> OK, going to send one today.

I haven't written a ChangeLog entry but the change safety has been verified by
stripped code differences.  Still GDB compiles out line numbers and assert
expressions so some of the differences were roughly checked in objdump -d they
are safe.

I will check it in after the Thiago's check-in.


Regards,
Jan


gdb/
2010-11-17  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Code cleanup - renaming.
	* breakpoint.c: Use bl for `*bp_location' variables, blp_tmp for
	`**bp_location' helper variables, b_tmp for `*breakpoint' helper
	variables.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1087,14 +1087,15 @@ bp_location_has_shadow (struct bp_location *bl)
    by replacing any memory breakpoints with their shadowed contents.
 
    The range of shadowed area by each bp_location is:
-     b->address - bp_location_placed_address_before_address_max
-     up to b->address + bp_location_shadow_len_after_address_max
+     bl->address - bp_location_placed_address_before_address_max
+     up to bl->address + bp_location_shadow_len_after_address_max
    The range we were requested to resolve shadows for is:
      memaddr ... memaddr + len
    Thus the safe cutoff boundaries for performance optimization are
-     memaddr + len <= b->address - bp_location_placed_address_before_address_max
+     memaddr + len <= (bl->address
+		       - bp_location_placed_address_before_address_max)
    and:
-     b->address + bp_location_shadow_len_after_address_max <= memaddr  */
+     bl->address + bp_location_shadow_len_after_address_max <= memaddr  */
 
 void
 breakpoint_restore_shadows (gdb_byte *buf, ULONGEST memaddr, LONGEST len)
@@ -1109,12 +1110,12 @@ breakpoint_restore_shadows (gdb_byte *buf, ULONGEST memaddr, LONGEST len)
   bc_r = bp_location_count;
   while (bc_l + 1 < bc_r)
     {
-      struct bp_location *b;
+      struct bp_location *bl;
 
       bc = (bc_l + bc_r) / 2;
-      b = bp_location[bc];
+      bl = bp_location[bc];
 
-      /* Check first B->ADDRESS will not overflow due to the added constant.
+      /* Check first BL->ADDRESS will not overflow due to the added constant.
 	 Then advance the left boundary only if we are sure the BC element can
 	 in no way affect the BUF content (MEMADDR to MEMADDR + LEN range).
 
@@ -1122,8 +1123,10 @@ breakpoint_restore_shadows (gdb_byte *buf, ULONGEST memaddr, LONGEST len)
 	 we cannot miss a breakpoint with its shadow range tail still reaching
 	 MEMADDR.  */
 
-      if (b->address + bp_location_shadow_len_after_address_max >= b->address
-	  && b->address + bp_location_shadow_len_after_address_max <= memaddr)
+      if ((bl->address + bp_location_shadow_len_after_address_max
+	   >= bl->address)
+	  && (bl->address + bp_location_shadow_len_after_address_max
+	      <= memaddr))
 	bc_l = bc;
       else
 	bc_r = bc;
@@ -1133,34 +1136,34 @@ breakpoint_restore_shadows (gdb_byte *buf, ULONGEST memaddr, LONGEST len)
 
   for (bc = bc_l; bc < bp_location_count; bc++)
   {
-    struct bp_location *b = bp_location[bc];
+    struct bp_location *bl = bp_location[bc];
     CORE_ADDR bp_addr = 0;
     int bp_size = 0;
     int bptoffset = 0;
 
-    /* bp_location array has B->OWNER always non-NULL.  */
-    if (b->owner->type == bp_none)
+    /* bp_location array has BL->OWNER always non-NULL.  */
+    if (bl->owner->type == bp_none)
       warning (_("reading through apparently deleted breakpoint #%d?"),
-              b->owner->number);
+	       bl->owner->number);
 
     /* Performance optimization: any futher element can no longer affect BUF
        content.  */
 
-    if (b->address >= bp_location_placed_address_before_address_max
-        && memaddr + len <= b->address
-			    - bp_location_placed_address_before_address_max)
+    if (bl->address >= bp_location_placed_address_before_address_max
+        && memaddr + len <= (bl->address
+			     - bp_location_placed_address_before_address_max))
       break;
 
-    if (!bp_location_has_shadow (b))
+    if (!bp_location_has_shadow (bl))
       continue;
-    if (!breakpoint_address_match (b->target_info.placed_address_space, 0,
+    if (!breakpoint_address_match (bl->target_info.placed_address_space, 0,
 				   current_program_space->aspace, 0))
       continue;
 
     /* Addresses and length of the part of the breakpoint that
        we need to copy.  */
-    bp_addr = b->target_info.placed_address;
-    bp_size = b->target_info.shadow_len;
+    bp_addr = bl->target_info.placed_address;
+    bp_size = bl->target_info.shadow_len;
 
     if (bp_addr + bp_size <= memaddr)
       /* The breakpoint is entirely before the chunk of memory we
@@ -1188,7 +1191,7 @@ breakpoint_restore_shadows (gdb_byte *buf, ULONGEST memaddr, LONGEST len)
       }
 
     memcpy (buf + bp_addr - memaddr,
-	    b->target_info.shadow_contents + bptoffset, bp_size);
+	    bl->target_info.shadow_contents + bptoffset, bp_size);
   }
 }
 \f
@@ -1533,15 +1536,15 @@ in which its expression is valid.\n"),
 /* Returns 1 iff breakpoint location should be
    inserted in the inferior.  */
 static int
-should_be_inserted (struct bp_location *bpt)
+should_be_inserted (struct bp_location *bl)
 {
-  if (bpt->owner == NULL || !breakpoint_enabled (bpt->owner))
+  if (bl->owner == NULL || !breakpoint_enabled (bl->owner))
     return 0;
 
-  if (bpt->owner->disposition == disp_del_at_next_stop)
+  if (bl->owner->disposition == disp_del_at_next_stop)
     return 0;
 
-  if (!bpt->enabled || bpt->shlib_disabled || bpt->duplicate)
+  if (!bl->enabled || bl->shlib_disabled || bl->duplicate)
     return 0;
 
   /* This is set for example, when we're attached to the parent of a
@@ -1552,43 +1555,43 @@ should_be_inserted (struct bp_location *bpt)
      memory region, do not insert breakpoints in the parent, otherwise
      the child could still trip on the parent's breakpoints.  Since
      the parent is blocked anyway, it won't miss any breakpoint.  */
-  if (bpt->pspace->breakpoints_not_allowed)
+  if (bl->pspace->breakpoints_not_allowed)
     return 0;
 
   /* Tracepoints are inserted by the target at a time of its choosing,
      not by us.  */
-  if (is_tracepoint (bpt->owner))
+  if (is_tracepoint (bl->owner))
     return 0;
 
   return 1;
 }
 
-/* Insert a low-level "breakpoint" of some type.  BPT is the breakpoint.
-   Any error messages are printed to TMP_ERROR_STREAM; and DISABLED_BREAKS,
-   and HW_BREAKPOINT_ERROR are used to report problems.
+/* Insert a low-level "breakpoint" of some type.  BL is the breakpoint
+   location.  Any error messages are printed to TMP_ERROR_STREAM; and
+   DISABLED_BREAKS, and HW_BREAKPOINT_ERROR are used to report problems.
 
    NOTE drow/2003-09-09: This routine could be broken down to an object-style
    method for each breakpoint or catchpoint type.  */
 static int
-insert_bp_location (struct bp_location *bpt,
+insert_bp_location (struct bp_location *bl,
 		    struct ui_file *tmp_error_stream,
 		    int *disabled_breaks,
 		    int *hw_breakpoint_error)
 {
   int val = 0;
 
-  if (!should_be_inserted (bpt) || bpt->inserted)
+  if (!should_be_inserted (bl) || bl->inserted)
     return 0;
 
   /* Initialize the target-specific information.  */
-  memset (&bpt->target_info, 0, sizeof (bpt->target_info));
-  bpt->target_info.placed_address = bpt->address;
-  bpt->target_info.placed_address_space = bpt->pspace->aspace;
+  memset (&bl->target_info, 0, sizeof (bl->target_info));
+  bl->target_info.placed_address = bl->address;
+  bl->target_info.placed_address_space = bl->pspace->aspace;
 
-  if (bpt->loc_type == bp_loc_software_breakpoint
-      || bpt->loc_type == bp_loc_hardware_breakpoint)
+  if (bl->loc_type == bp_loc_software_breakpoint
+      || bl->loc_type == bp_loc_hardware_breakpoint)
     {
-      if (bpt->owner->type != bp_hardware_breakpoint)
+      if (bl->owner->type != bp_hardware_breakpoint)
 	{
 	  /* If the explicitly specified breakpoint type
 	     is not hardware breakpoint, check the memory map to see
@@ -1606,7 +1609,7 @@ insert_bp_location (struct bp_location *bpt,
 	     problem is that memory map has changed during running program,
 	     but it's not going to work anyway with current gdb.  */
 	  struct mem_region *mr 
-	    = lookup_mem_region (bpt->target_info.placed_address);
+	    = lookup_mem_region (bl->target_info.placed_address);
 	  
 	  if (mr)
 	    {
@@ -1619,11 +1622,11 @@ insert_bp_location (struct bp_location *bpt,
 		  else 
 		    new_type = bp_loc_software_breakpoint;
 		  
-		  if (new_type != bpt->loc_type)
+		  if (new_type != bl->loc_type)
 		    {
 		      static int said = 0;
 
-		      bpt->loc_type = new_type;
+		      bl->loc_type = new_type;
 		      if (!said)
 			{
 			  fprintf_filtered (gdb_stdout, _("\
@@ -1632,26 +1635,26 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
 			}
 		    }
 		}
-	      else if (bpt->loc_type == bp_loc_software_breakpoint
+	      else if (bl->loc_type == bp_loc_software_breakpoint
 		       && mr->attrib.mode != MEM_RW)	    
 		warning (_("cannot set software breakpoint at readonly address %s"),
-			 paddress (bpt->gdbarch, bpt->address));
+			 paddress (bl->gdbarch, bl->address));
 	    }
 	}
         
       /* First check to see if we have to handle an overlay.  */
       if (overlay_debugging == ovly_off
-	  || bpt->section == NULL
-	  || !(section_is_overlay (bpt->section)))
+	  || bl->section == NULL
+	  || !(section_is_overlay (bl->section)))
 	{
 	  /* No overlay handling: just set the breakpoint.  */
 
-	  if (bpt->loc_type == bp_loc_hardware_breakpoint)
-	    val = target_insert_hw_breakpoint (bpt->gdbarch,
-					       &bpt->target_info);
+	  if (bl->loc_type == bp_loc_hardware_breakpoint)
+	    val = target_insert_hw_breakpoint (bl->gdbarch,
+					       &bl->target_info);
 	  else
-	    val = target_insert_breakpoint (bpt->gdbarch,
-					    &bpt->target_info);
+	    val = target_insert_breakpoint (bl->gdbarch,
+					    &bl->target_info);
 	}
       else
 	{
@@ -1662,34 +1665,34 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
 	      /* Yes -- overlay event support is not active, 
 		 so we must try to set a breakpoint at the LMA.
 		 This will not work for a hardware breakpoint.  */
-	      if (bpt->loc_type == bp_loc_hardware_breakpoint)
+	      if (bl->loc_type == bp_loc_hardware_breakpoint)
 		warning (_("hardware breakpoint %d not supported in overlay!"),
-			 bpt->owner->number);
+			 bl->owner->number);
 	      else
 		{
-		  CORE_ADDR addr = overlay_unmapped_address (bpt->address,
-							     bpt->section);
+		  CORE_ADDR addr = overlay_unmapped_address (bl->address,
+							     bl->section);
 		  /* Set a software (trap) breakpoint at the LMA.  */
-		  bpt->overlay_target_info = bpt->target_info;
-		  bpt->overlay_target_info.placed_address = addr;
-		  val = target_insert_breakpoint (bpt->gdbarch,
-						  &bpt->overlay_target_info);
+		  bl->overlay_target_info = bl->target_info;
+		  bl->overlay_target_info.placed_address = addr;
+		  val = target_insert_breakpoint (bl->gdbarch,
+						  &bl->overlay_target_info);
 		  if (val != 0)
 		    fprintf_unfiltered (tmp_error_stream,
 					"Overlay breakpoint %d failed: in ROM?\n",
-					bpt->owner->number);
+					bl->owner->number);
 		}
 	    }
 	  /* Shall we set a breakpoint at the VMA? */
-	  if (section_is_mapped (bpt->section))
+	  if (section_is_mapped (bl->section))
 	    {
 	      /* Yes.  This overlay section is mapped into memory.  */
-	      if (bpt->loc_type == bp_loc_hardware_breakpoint)
-		val = target_insert_hw_breakpoint (bpt->gdbarch,
-						   &bpt->target_info);
+	      if (bl->loc_type == bp_loc_hardware_breakpoint)
+		val = target_insert_hw_breakpoint (bl->gdbarch,
+						   &bl->target_info);
 	      else
-		val = target_insert_breakpoint (bpt->gdbarch,
-						&bpt->target_info);
+		val = target_insert_breakpoint (bl->gdbarch,
+						&bl->target_info);
 	    }
 	  else
 	    {
@@ -1702,40 +1705,40 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
       if (val)
 	{
 	  /* Can't set the breakpoint.  */
-	  if (solib_name_from_address (bpt->pspace, bpt->address))
+	  if (solib_name_from_address (bl->pspace, bl->address))
 	    {
 	      /* See also: disable_breakpoints_in_shlibs. */
 	      val = 0;
-	      bpt->shlib_disabled = 1;
+	      bl->shlib_disabled = 1;
 	      if (!*disabled_breaks)
 		{
 		  fprintf_unfiltered (tmp_error_stream, 
 				      "Cannot insert breakpoint %d.\n", 
-				      bpt->owner->number);
+				      bl->owner->number);
 		  fprintf_unfiltered (tmp_error_stream, 
 				      "Temporarily disabling shared library breakpoints:\n");
 		}
 	      *disabled_breaks = 1;
 	      fprintf_unfiltered (tmp_error_stream,
-				  "breakpoint #%d\n", bpt->owner->number);
+				  "breakpoint #%d\n", bl->owner->number);
 	    }
 	  else
 	    {
-	      if (bpt->loc_type == bp_loc_hardware_breakpoint)
+	      if (bl->loc_type == bp_loc_hardware_breakpoint)
 		{
 		  *hw_breakpoint_error = 1;
 		  fprintf_unfiltered (tmp_error_stream, 
 				      "Cannot insert hardware breakpoint %d.\n",
-				      bpt->owner->number);
+				      bl->owner->number);
 		}
 	      else
 		{
 		  fprintf_unfiltered (tmp_error_stream, 
 				      "Cannot insert breakpoint %d.\n", 
-				      bpt->owner->number);
+				      bl->owner->number);
 		  fprintf_filtered (tmp_error_stream, 
 				    "Error accessing memory address ");
-		  fputs_filtered (paddress (bpt->gdbarch, bpt->address),
+		  fputs_filtered (paddress (bl->gdbarch, bl->address),
 				  tmp_error_stream);
 		  fprintf_filtered (tmp_error_stream, ": %s.\n",
 				    safe_strerror (val));
@@ -1744,21 +1747,21 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
 	    }
 	}
       else
-	bpt->inserted = 1;
+	bl->inserted = 1;
 
       return val;
     }
 
-  else if (bpt->loc_type == bp_loc_hardware_watchpoint
+  else if (bl->loc_type == bp_loc_hardware_watchpoint
 	   /* NOTE drow/2003-09-08: This state only exists for removing
 	      watchpoints.  It's not clear that it's necessary... */
-	   && bpt->owner->disposition != disp_del_at_next_stop)
+	   && bl->owner->disposition != disp_del_at_next_stop)
     {
-      val = bpt->owner->ops->insert (bpt);
+      val = bl->owner->ops->insert (bl);
 
       /* If trying to set a read-watchpoint, and it turns out it's not
 	 supported, try emulating one with an access watchpoint.  */
-      if (val == 1 && bpt->watchpoint_type == hw_read)
+      if (val == 1 && bl->watchpoint_type == hw_read)
 	{
 	  struct bp_location *loc, **loc_temp;
 
@@ -1766,50 +1769,50 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
 	     hw_access location that would be considered a duplicate
 	     of this one.  */
 	  ALL_BP_LOCATIONS (loc, loc_temp)
-	    if (loc != bpt
+	    if (loc != bl
 		&& loc->watchpoint_type == hw_access
-		&& watchpoint_locations_match (bpt, loc))
+		&& watchpoint_locations_match (bl, loc))
 	      {
-		bpt->duplicate = 1;
-		bpt->inserted = 1;
-		bpt->target_info = loc->target_info;
-		bpt->watchpoint_type = hw_access;
+		bl->duplicate = 1;
+		bl->inserted = 1;
+		bl->target_info = loc->target_info;
+		bl->watchpoint_type = hw_access;
 		val = 0;
 		break;
 	      }
 
 	  if (val == 1)
 	    {
-	      bpt->watchpoint_type = hw_access;
-	      val = bpt->owner->ops->insert (bpt);
+	      bl->watchpoint_type = hw_access;
+	      val = bl->owner->ops->insert (bl);
 
 	      if (val)
 		/* Back to the original value.  */
-		bpt->watchpoint_type = hw_read;
+		bl->watchpoint_type = hw_read;
 	    }
 	}
 
-      bpt->inserted = (val == 0);
+      bl->inserted = (val == 0);
     }
 
-  else if (bpt->owner->type == bp_catchpoint)
+  else if (bl->owner->type == bp_catchpoint)
     {
-      gdb_assert (bpt->owner->ops != NULL && bpt->owner->ops->insert != NULL);
+      gdb_assert (bl->owner->ops != NULL && bl->owner->ops->insert != NULL);
 
-      val = bpt->owner->ops->insert (bpt);
+      val = bl->owner->ops->insert (bl);
       if (val)
 	{
-	  bpt->owner->enable_state = bp_disabled;
+	  bl->owner->enable_state = bp_disabled;
 
 	  if (val == 1)
 	    warning (_("\
 Inserting catchpoint %d: Your system does not support this type of catchpoint."),
-		     bpt->owner->number);
+		     bl->owner->number);
 	  else
-	    warning (_("Error inserting catchpoint %d."), bpt->owner->number);
+	    warning (_("Error inserting catchpoint %d."), bl->owner->number);
 	}
 
-      bpt->inserted = (val == 0);
+      bl->inserted = (val == 0);
 
       /* We've already printed an error message if there was a problem
 	 inserting this catchpoint, and we've disabled the catchpoint,
@@ -1894,7 +1897,7 @@ static void
 insert_breakpoint_locations (void)
 {
   struct breakpoint *bpt;
-  struct bp_location *b, **bp_tmp;
+  struct bp_location *bl, **blp_tmp;
   int error = 0;
   int val = 0;
   int disabled_breaks = 0;
@@ -1909,19 +1912,19 @@ insert_breakpoint_locations (void)
 
   save_current_space_and_thread ();
 
-  ALL_BP_LOCATIONS (b, bp_tmp)
+  ALL_BP_LOCATIONS (bl, blp_tmp)
     {
-      if (!should_be_inserted (b) || b->inserted)
+      if (!should_be_inserted (bl) || bl->inserted)
 	continue;
 
       /* There is no point inserting thread-specific breakpoints if the
-	 thread no longer exists.  ALL_BP_LOCATIONS bp_location has B->OWNER
+	 thread no longer exists.  ALL_BP_LOCATIONS bp_location has BL->OWNER
 	 always non-NULL.  */
-      if (b->owner->thread != -1
-	  && !valid_thread_id (b->owner->thread))
+      if (bl->owner->thread != -1
+	  && !valid_thread_id (bl->owner->thread))
 	continue;
 
-      switch_to_program_space_and_thread (b->pspace);
+      switch_to_program_space_and_thread (bl->pspace);
 
       /* For targets that support global breakpoints, there's no need
 	 to select an inferior to insert breakpoint to.  In fact, even
@@ -1931,8 +1934,7 @@ insert_breakpoint_locations (void)
 	  && ptid_equal (inferior_ptid, null_ptid))
 	continue;
 
-      val = insert_bp_location (b, tmp_error_stream,
-				    &disabled_breaks,
+      val = insert_bp_location (bl, tmp_error_stream, &disabled_breaks,
 				    &hw_breakpoint_error);
       if (val)
 	error = val;
@@ -1994,13 +1996,13 @@ You may have requested too many hardware breakpoints/watchpoints.\n");
 int
 remove_breakpoints (void)
 {
-  struct bp_location *b, **bp_tmp;
+  struct bp_location *bl, **blp_tmp;
   int val = 0;
 
-  ALL_BP_LOCATIONS (b, bp_tmp)
+  ALL_BP_LOCATIONS (bl, blp_tmp)
   {
-    if (b->inserted)
-      val |= remove_breakpoint (b, mark_uninserted);
+    if (bl->inserted)
+      val |= remove_breakpoint (bl, mark_uninserted);
   }
   return val;
 }
@@ -2010,18 +2012,18 @@ remove_breakpoints (void)
 int
 remove_breakpoints_pid (int pid)
 {
-  struct bp_location *b, **b_tmp;
+  struct bp_location *bl, **blp_tmp;
   int val;
   struct inferior *inf = find_inferior_pid (pid);
 
-  ALL_BP_LOCATIONS (b, b_tmp)
+  ALL_BP_LOCATIONS (bl, blp_tmp)
   {
-    if (b->pspace != inf->pspace)
+    if (bl->pspace != inf->pspace)
       continue;
 
-    if (b->inserted)
+    if (bl->inserted)
       {
-	val = remove_breakpoint (b, mark_uninserted);
+	val = remove_breakpoint (bl, mark_uninserted);
 	if (val != 0)
 	  return val;
       }
@@ -2032,13 +2034,13 @@ remove_breakpoints_pid (int pid)
 int
 remove_hw_watchpoints (void)
 {
-  struct bp_location *b, **bp_tmp;
+  struct bp_location *bl, **blp_tmp;
   int val = 0;
 
-  ALL_BP_LOCATIONS (b, bp_tmp)
+  ALL_BP_LOCATIONS (bl, blp_tmp)
   {
-    if (b->inserted && b->loc_type == bp_loc_hardware_watchpoint)
-      val |= remove_breakpoint (b, mark_uninserted);
+    if (bl->inserted && bl->loc_type == bp_loc_hardware_watchpoint)
+      val |= remove_breakpoint (bl, mark_uninserted);
   }
   return val;
 }
@@ -2047,7 +2049,7 @@ int
 reattach_breakpoints (int pid)
 {
   struct cleanup *old_chain;
-  struct bp_location *b, **bp_tmp;
+  struct bp_location *bl, **blp_tmp;
   int val;
   struct ui_file *tmp_error_stream = mem_fileopen ();
   int dummy1 = 0, dummy2 = 0;
@@ -2065,16 +2067,15 @@ reattach_breakpoints (int pid)
 
   make_cleanup_ui_file_delete (tmp_error_stream);
 
-  ALL_BP_LOCATIONS (b, bp_tmp)
+  ALL_BP_LOCATIONS (bl, blp_tmp)
   {
-    if (b->pspace != inf->pspace)
+    if (bl->pspace != inf->pspace)
       continue;
 
-    if (b->inserted)
+    if (bl->inserted)
       {
-	b->inserted = 0;
-	val = insert_bp_location (b, tmp_error_stream,
-				  &dummy1, &dummy2);
+	bl->inserted = 0;
+	val = insert_bp_location (bl, tmp_error_stream, &dummy1, &dummy2);
 	if (val != 0)
 	  {
 	    do_cleanups (old_chain);
@@ -2232,8 +2233,7 @@ create_std_terminate_master_breakpoint (const char *func_name)
 void
 update_breakpoints_after_exec (void)
 {
-  struct breakpoint *b;
-  struct breakpoint *temp;
+  struct breakpoint *b, *b_tmp;
   struct bp_location *bploc, **bplocp_tmp;
 
   /* We're about to delete breakpoints from GDB's lists.  If the
@@ -2248,7 +2248,7 @@ update_breakpoints_after_exec (void)
     if (bploc->pspace == current_program_space)
       gdb_assert (!bploc->inserted);
 
-  ALL_BREAKPOINTS_SAFE (b, temp)
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
   {
     if (b->pspace != current_program_space)
       continue;
@@ -2352,7 +2352,7 @@ update_breakpoints_after_exec (void)
 int
 detach_breakpoints (int pid)
 {
-  struct bp_location *b, **bp_tmp;
+  struct bp_location *bl, **blp_tmp;
   int val = 0;
   struct cleanup *old_chain = save_inferior_ptid ();
   struct inferior *inf = current_inferior ();
@@ -2362,13 +2362,13 @@ detach_breakpoints (int pid)
 
   /* Set inferior_ptid; remove_breakpoint_1 uses this global.  */
   inferior_ptid = pid_to_ptid (pid);
-  ALL_BP_LOCATIONS (b, bp_tmp)
+  ALL_BP_LOCATIONS (bl, blp_tmp)
   {
-    if (b->pspace != inf->pspace)
+    if (bl->pspace != inf->pspace)
       continue;
 
-    if (b->inserted)
-      val |= remove_breakpoint_1 (b, mark_inserted);
+    if (bl->inserted)
+      val |= remove_breakpoint_1 (bl, mark_inserted);
   }
 
   /* Detach single-step breakpoints as well.  */
@@ -2378,30 +2378,30 @@ detach_breakpoints (int pid)
   return val;
 }
 
-/* Remove the breakpoint location B from the current address space.
+/* Remove the breakpoint location BL from the current address space.
    Note that this is used to detach breakpoints from a child fork.
    When we get here, the child isn't in the inferior list, and neither
    do we have objects to represent its address space --- we should
-   *not* look at b->pspace->aspace here.  */
+   *not* look at bl->pspace->aspace here.  */
 
 static int
-remove_breakpoint_1 (struct bp_location *b, insertion_state_t is)
+remove_breakpoint_1 (struct bp_location *bl, insertion_state_t is)
 {
   int val;
 
-  /* B is never in moribund_locations by our callers.  */
-  gdb_assert (b->owner != NULL);
+  /* BL is never in moribund_locations by our callers.  */
+  gdb_assert (bl->owner != NULL);
 
-  if (b->owner->enable_state == bp_permanent)
+  if (bl->owner->enable_state == bp_permanent)
     /* Permanent breakpoints cannot be inserted or removed.  */
     return 0;
 
   /* The type of none suggests that owner is actually deleted.
      This should not ever happen.  */
-  gdb_assert (b->owner->type != bp_none);
+  gdb_assert (bl->owner->type != bp_none);
 
-  if (b->loc_type == bp_loc_software_breakpoint
-      || b->loc_type == bp_loc_hardware_breakpoint)
+  if (bl->loc_type == bp_loc_software_breakpoint
+      || bl->loc_type == bp_loc_hardware_breakpoint)
     {
       /* "Normal" instruction breakpoint: either the standard
 	 trap-instruction bp (bp_breakpoint), or a
@@ -2409,15 +2409,15 @@ remove_breakpoint_1 (struct bp_location *b, insertion_state_t is)
 
       /* First check to see if we have to handle an overlay.  */
       if (overlay_debugging == ovly_off
-	  || b->section == NULL
-	  || !(section_is_overlay (b->section)))
+	  || bl->section == NULL
+	  || !(section_is_overlay (bl->section)))
 	{
 	  /* No overlay handling: just remove the breakpoint.  */
 
-	  if (b->loc_type == bp_loc_hardware_breakpoint)
-	    val = target_remove_hw_breakpoint (b->gdbarch, &b->target_info);
+	  if (bl->loc_type == bp_loc_hardware_breakpoint)
+	    val = target_remove_hw_breakpoint (bl->gdbarch, &bl->target_info);
 	  else
-	    val = target_remove_breakpoint (b->gdbarch, &b->target_info);
+	    val = target_remove_breakpoint (bl->gdbarch, &bl->target_info);
 	}
       else
 	{
@@ -2430,31 +2430,31 @@ remove_breakpoint_1 (struct bp_location *b, insertion_state_t is)
 		*/
 		/* Ignore any failures: if the LMA is in ROM, we will
 		   have already warned when we failed to insert it.  */
-		if (b->loc_type == bp_loc_hardware_breakpoint)
-		  target_remove_hw_breakpoint (b->gdbarch,
-					       &b->overlay_target_info);
+		if (bl->loc_type == bp_loc_hardware_breakpoint)
+		  target_remove_hw_breakpoint (bl->gdbarch,
+					       &bl->overlay_target_info);
 		else
-		  target_remove_breakpoint (b->gdbarch,
-					    &b->overlay_target_info);
+		  target_remove_breakpoint (bl->gdbarch,
+					    &bl->overlay_target_info);
 	      }
 	  /* Did we set a breakpoint at the VMA? 
 	     If so, we will have marked the breakpoint 'inserted'.  */
-	  if (b->inserted)
+	  if (bl->inserted)
 	    {
 	      /* Yes -- remove it.  Previously we did not bother to
 		 remove the breakpoint if the section had been
 		 unmapped, but let's not rely on that being safe.  We
 		 don't know what the overlay manager might do.  */
-	      if (b->loc_type == bp_loc_hardware_breakpoint)
-		val = target_remove_hw_breakpoint (b->gdbarch,
-						   &b->target_info);
+	      if (bl->loc_type == bp_loc_hardware_breakpoint)
+		val = target_remove_hw_breakpoint (bl->gdbarch,
+						   &bl->target_info);
 
 	      /* However, we should remove *software* breakpoints only
 		 if the section is still mapped, or else we overwrite
 		 wrong code with the saved shadow contents.  */
-	      else if (section_is_mapped (b->section))
-		val = target_remove_breakpoint (b->gdbarch,
-						&b->target_info);
+	      else if (section_is_mapped (bl->section))
+		val = target_remove_breakpoint (bl->gdbarch,
+						&bl->target_info);
 	      else
 		val = 0;
 	    }
@@ -2468,61 +2468,61 @@ remove_breakpoint_1 (struct bp_location *b, insertion_state_t is)
       /* In some cases, we might not be able to remove a breakpoint
 	 in a shared library that has already been removed, but we
 	 have not yet processed the shlib unload event.  */
-      if (val && solib_name_from_address (b->pspace, b->address))
+      if (val && solib_name_from_address (bl->pspace, bl->address))
 	val = 0;
 
       if (val)
 	return val;
-      b->inserted = (is == mark_inserted);
+      bl->inserted = (is == mark_inserted);
     }
-  else if (b->loc_type == bp_loc_hardware_watchpoint)
+  else if (bl->loc_type == bp_loc_hardware_watchpoint)
     {
-      b->inserted = (is == mark_inserted);
-      b->owner->ops->remove (b);
+      bl->inserted = (is == mark_inserted);
+      bl->owner->ops->remove (bl);
 
       /* Failure to remove any of the hardware watchpoints comes here.  */
-      if ((is == mark_uninserted) && (b->inserted))
+      if ((is == mark_uninserted) && (bl->inserted))
 	warning (_("Could not remove hardware watchpoint %d."),
-		 b->owner->number);
+		 bl->owner->number);
     }
-  else if (b->owner->type == bp_catchpoint
-           && breakpoint_enabled (b->owner)
-           && !b->duplicate)
+  else if (bl->owner->type == bp_catchpoint
+           && breakpoint_enabled (bl->owner)
+           && !bl->duplicate)
     {
-      gdb_assert (b->owner->ops != NULL && b->owner->ops->remove != NULL);
+      gdb_assert (bl->owner->ops != NULL && bl->owner->ops->remove != NULL);
 
-      val = b->owner->ops->remove (b);
+      val = bl->owner->ops->remove (bl);
       if (val)
 	return val;
 
-      b->inserted = (is == mark_inserted);
+      bl->inserted = (is == mark_inserted);
     }
 
   return 0;
 }
 
 static int
-remove_breakpoint (struct bp_location *b, insertion_state_t is)
+remove_breakpoint (struct bp_location *bl, insertion_state_t is)
 {
   int ret;
   struct cleanup *old_chain;
 
-  /* B is never in moribund_locations by our callers.  */
-  gdb_assert (b->owner != NULL);
+  /* BL is never in moribund_locations by our callers.  */
+  gdb_assert (bl->owner != NULL);
 
-  if (b->owner->enable_state == bp_permanent)
+  if (bl->owner->enable_state == bp_permanent)
     /* Permanent breakpoints cannot be inserted or removed.  */
     return 0;
 
   /* The type of none suggests that owner is actually deleted.
      This should not ever happen.  */
-  gdb_assert (b->owner->type != bp_none);
+  gdb_assert (bl->owner->type != bp_none);
 
   old_chain = save_current_space_and_thread ();
 
-  switch_to_program_space_and_thread (b->pspace);
+  switch_to_program_space_and_thread (bl->pspace);
 
-  ret = remove_breakpoint_1 (b, is);
+  ret = remove_breakpoint_1 (bl, is);
 
   do_cleanups (old_chain);
   return ret;
@@ -2533,11 +2533,11 @@ remove_breakpoint (struct bp_location *b, insertion_state_t is)
 void
 mark_breakpoints_out (void)
 {
-  struct bp_location *bpt, **bptp_tmp;
+  struct bp_location *bl, **blp_tmp;
 
-  ALL_BP_LOCATIONS (bpt, bptp_tmp)
-    if (bpt->pspace == current_program_space)
-      bpt->inserted = 0;
+  ALL_BP_LOCATIONS (bl, blp_tmp)
+    if (bl->pspace == current_program_space)
+      bl->inserted = 0;
 }
 
 /* Clear the "inserted" flag in all breakpoints and delete any
@@ -2555,8 +2555,8 @@ mark_breakpoints_out (void)
 void
 breakpoint_init_inferior (enum inf_context context)
 {
-  struct breakpoint *b, *temp;
-  struct bp_location *bpt, **bptp_tmp;
+  struct breakpoint *b, *b_tmp;
+  struct bp_location *bl, **blp_tmp;
   int ix;
   struct program_space *pspace = current_program_space;
 
@@ -2565,15 +2565,15 @@ breakpoint_init_inferior (enum inf_context context)
   if (gdbarch_has_global_breakpoints (target_gdbarch))
     return;
 
-  ALL_BP_LOCATIONS (bpt, bptp_tmp)
+  ALL_BP_LOCATIONS (bl, blp_tmp)
   {
-    /* ALL_BP_LOCATIONS bp_location has BPT->OWNER always non-NULL.  */
-    if (bpt->pspace == pspace
-	&& bpt->owner->enable_state != bp_permanent)
-      bpt->inserted = 0;
+    /* ALL_BP_LOCATIONS bp_location has BL->OWNER always non-NULL.  */
+    if (bl->pspace == pspace
+	&& bl->owner->enable_state != bp_permanent)
+      bl->inserted = 0;
   }
 
-  ALL_BREAKPOINTS_SAFE (b, temp)
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
   {
     if (b->loc && b->loc->pspace != pspace)
       continue;
@@ -2633,8 +2633,8 @@ breakpoint_init_inferior (enum inf_context context)
   }
 
   /* Get rid of the moribund locations.  */
-  for (ix = 0; VEC_iterate (bp_location_p, moribund_locations, ix, bpt); ++ix)
-    decref_bp_location (&bpt);
+  for (ix = 0; VEC_iterate (bp_location_p, moribund_locations, ix, bl); ++ix)
+    decref_bp_location (&bl);
   VEC_free (bp_location_p, moribund_locations);
 }
 
@@ -2656,26 +2656,26 @@ breakpoint_init_inferior (enum inf_context context)
 enum breakpoint_here
 breakpoint_here_p (struct address_space *aspace, CORE_ADDR pc)
 {
-  struct bp_location *bpt, **bptp_tmp;
+  struct bp_location *bl, **blp_tmp;
   int any_breakpoint_here = 0;
 
-  ALL_BP_LOCATIONS (bpt, bptp_tmp)
+  ALL_BP_LOCATIONS (bl, blp_tmp)
     {
-      if (bpt->loc_type != bp_loc_software_breakpoint
-	  && bpt->loc_type != bp_loc_hardware_breakpoint)
+      if (bl->loc_type != bp_loc_software_breakpoint
+	  && bl->loc_type != bp_loc_hardware_breakpoint)
 	continue;
 
-      /* ALL_BP_LOCATIONS bp_location has BPT->OWNER always non-NULL.  */
-      if ((breakpoint_enabled (bpt->owner)
-	   || bpt->owner->enable_state == bp_permanent)
-	  && breakpoint_address_match (bpt->pspace->aspace, bpt->address,
+      /* ALL_BP_LOCATIONS bp_location has bl->OWNER always non-NULL.  */
+      if ((breakpoint_enabled (bl->owner)
+	   || bl->owner->enable_state == bp_permanent)
+	  && breakpoint_address_match (bl->pspace->aspace, bl->address,
 				       aspace, pc))
 	{
 	  if (overlay_debugging 
-	      && section_is_overlay (bpt->section) 
-	      && !section_is_mapped (bpt->section))
+	      && section_is_overlay (bl->section) 
+	      && !section_is_mapped (bl->section))
 	    continue;		/* unmapped overlay -- can't be a match */
-	  else if (bpt->owner->enable_state == bp_permanent)
+	  else if (bl->owner->enable_state == bp_permanent)
 	    return permanent_breakpoint_here;
 	  else
 	    any_breakpoint_here = 1;
@@ -2709,21 +2709,21 @@ moribund_breakpoint_here_p (struct address_space *aspace, CORE_ADDR pc)
 int
 regular_breakpoint_inserted_here_p (struct address_space *aspace, CORE_ADDR pc)
 {
-  struct bp_location *bpt, **bptp_tmp;
+  struct bp_location *bl, **blp_tmp;
 
-  ALL_BP_LOCATIONS (bpt, bptp_tmp)
+  ALL_BP_LOCATIONS (bl, blp_tmp)
     {
-      if (bpt->loc_type != bp_loc_software_breakpoint
-	  && bpt->loc_type != bp_loc_hardware_breakpoint)
+      if (bl->loc_type != bp_loc_software_breakpoint
+	  && bl->loc_type != bp_loc_hardware_breakpoint)
 	continue;
 
-      if (bpt->inserted
-	  && breakpoint_address_match (bpt->pspace->aspace, bpt->address,
+      if (bl->inserted
+	  && breakpoint_address_match (bl->pspace->aspace, bl->address,
 				       aspace, pc))
 	{
 	  if (overlay_debugging 
-	      && section_is_overlay (bpt->section) 
-	      && !section_is_mapped (bpt->section))
+	      && section_is_overlay (bl->section) 
+	      && !section_is_mapped (bl->section))
 	    continue;		/* unmapped overlay -- can't be a match */
 	  else
 	    return 1;
@@ -2753,20 +2753,20 @@ breakpoint_inserted_here_p (struct address_space *aspace, CORE_ADDR pc)
 int
 software_breakpoint_inserted_here_p (struct address_space *aspace, CORE_ADDR pc)
 {
-  struct bp_location *bpt, **bptp_tmp;
+  struct bp_location *bl, **blp_tmp;
 
-  ALL_BP_LOCATIONS (bpt, bptp_tmp)
+  ALL_BP_LOCATIONS (bl, blp_tmp)
     {
-      if (bpt->loc_type != bp_loc_software_breakpoint)
+      if (bl->loc_type != bp_loc_software_breakpoint)
 	continue;
 
-      if (bpt->inserted
-	  && breakpoint_address_match (bpt->pspace->aspace, bpt->address,
+      if (bl->inserted
+	  && breakpoint_address_match (bl->pspace->aspace, bl->address,
 				       aspace, pc))
 	{
 	  if (overlay_debugging 
-	      && section_is_overlay (bpt->section) 
-	      && !section_is_mapped (bpt->section))
+	      && section_is_overlay (bl->section) 
+	      && !section_is_mapped (bl->section))
 	    continue;		/* unmapped overlay -- can't be a match */
 	  else
 	    return 1;
@@ -2819,51 +2819,51 @@ int
 breakpoint_thread_match (struct address_space *aspace, CORE_ADDR pc,
 			 ptid_t ptid)
 {
-  struct bp_location *bpt, **bptp_tmp;
+  struct bp_location *bl, **blp_tmp;
   /* The thread and task IDs associated to PTID, computed lazily.  */
   int thread = -1;
   int task = 0;
   
-  ALL_BP_LOCATIONS (bpt, bptp_tmp)
+  ALL_BP_LOCATIONS (bl, blp_tmp)
     {
-      if (bpt->loc_type != bp_loc_software_breakpoint
-	  && bpt->loc_type != bp_loc_hardware_breakpoint)
+      if (bl->loc_type != bp_loc_software_breakpoint
+	  && bl->loc_type != bp_loc_hardware_breakpoint)
 	continue;
 
-      /* ALL_BP_LOCATIONS bp_location has BPT->OWNER always non-NULL.  */
-      if (!breakpoint_enabled (bpt->owner)
-	  && bpt->owner->enable_state != bp_permanent)
+      /* ALL_BP_LOCATIONS bp_location has bl->OWNER always non-NULL.  */
+      if (!breakpoint_enabled (bl->owner)
+	  && bl->owner->enable_state != bp_permanent)
 	continue;
 
-      if (!breakpoint_address_match (bpt->pspace->aspace, bpt->address,
+      if (!breakpoint_address_match (bl->pspace->aspace, bl->address,
 				     aspace, pc))
 	continue;
 
-      if (bpt->owner->thread != -1)
+      if (bl->owner->thread != -1)
 	{
 	  /* This is a thread-specific breakpoint.  Check that ptid
 	     matches that thread.  If thread hasn't been computed yet,
 	     it is now time to do so.  */
 	  if (thread == -1)
 	    thread = pid_to_thread_id (ptid);
-	  if (bpt->owner->thread != thread)
+	  if (bl->owner->thread != thread)
 	    continue;
 	}
 
-      if (bpt->owner->task != 0)
+      if (bl->owner->task != 0)
         {
 	  /* This is a task-specific breakpoint.  Check that ptid
 	     matches that task.  If task hasn't been computed yet,
 	     it is now time to do so.  */
 	  if (task == 0)
 	    task = ada_get_task_number (ptid);
-	  if (bpt->owner->task != task)
+	  if (bl->owner->task != task)
 	    continue;
         }
 
       if (overlay_debugging 
-	  && section_is_overlay (bpt->section) 
-	  && !section_is_mapped (bpt->section))
+	  && section_is_overlay (bl->section) 
+	  && !section_is_mapped (bl->section))
 	continue;	    /* unmapped overlay -- can't be a match */
 
       return 1;
@@ -5654,13 +5654,13 @@ make_breakpoint_permanent (struct breakpoint *b)
 void
 set_longjmp_breakpoint (int thread)
 {
-  struct breakpoint *b, *temp;
+  struct breakpoint *b, *b_tmp;
 
   /* To avoid having to rescan all objfile symbols at every step,
      we maintain a list of continually-inserted but always disabled
      longjmp "master" breakpoints.  Here, we simply create momentary
      clones of those and enable them for the requested thread.  */
-  ALL_BREAKPOINTS_SAFE (b, temp)
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
     if (b->pspace == current_program_space
 	&& b->type == bp_longjmp_master)
       {
@@ -5675,9 +5675,9 @@ set_longjmp_breakpoint (int thread)
 void
 delete_longjmp_breakpoint (int thread)
 {
-  struct breakpoint *b, *temp;
+  struct breakpoint *b, *b_tmp;
 
-  ALL_BREAKPOINTS_SAFE (b, temp)
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
     if (b->type == bp_longjmp)
       {
 	if (b->thread == thread)
@@ -5718,9 +5718,9 @@ disable_overlay_breakpoints (void)
 void
 set_std_terminate_breakpoint (void)
 {
-  struct breakpoint *b, *temp;
+  struct breakpoint *b, *b_tmp;
 
-  ALL_BREAKPOINTS_SAFE (b, temp)
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
     if (b->pspace == current_program_space
 	&& b->type == bp_std_terminate_master)
       {
@@ -5733,9 +5733,9 @@ set_std_terminate_breakpoint (void)
 void
 delete_std_terminate_breakpoint (void)
 {
-  struct breakpoint *b, *temp;
+  struct breakpoint *b, *b_tmp;
 
-  ALL_BREAKPOINTS_SAFE (b, temp)
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
     if (b->type == bp_std_terminate)
       delete_breakpoint (b);
 }
@@ -5760,9 +5760,9 @@ create_thread_event_breakpoint (struct gdbarch *gdbarch, CORE_ADDR address)
 void
 remove_thread_event_breakpoints (void)
 {
-  struct breakpoint *b, *temp;
+  struct breakpoint *b, *b_tmp;
 
-  ALL_BREAKPOINTS_SAFE (b, temp)
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
     if (b->type == bp_thread_event
 	&& b->loc->pspace == current_program_space)
       delete_breakpoint (b);
@@ -5797,9 +5797,9 @@ create_jit_event_breakpoint (struct gdbarch *gdbarch, CORE_ADDR address)
 void
 remove_solib_event_breakpoints (void)
 {
-  struct breakpoint *b, *temp;
+  struct breakpoint *b, *b_tmp;
 
-  ALL_BREAKPOINTS_SAFE (b, temp)
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
     if (b->type == bp_shlib_event
 	&& b->loc->pspace == current_program_space)
       delete_breakpoint (b);
@@ -5903,7 +5903,7 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
 /* Implement the "insert" breakpoint_ops method for fork catchpoints.  */
 
 static int
-insert_catch_fork (struct bp_location *b)
+insert_catch_fork (struct bp_location *bl)
 {
   return target_insert_fork_catchpoint (PIDGET (inferior_ptid));
 }
@@ -5911,7 +5911,7 @@ insert_catch_fork (struct bp_location *b)
 /* Implement the "remove" breakpoint_ops method for fork catchpoints.  */
 
 static int
-remove_catch_fork (struct bp_location *b)
+remove_catch_fork (struct bp_location *bl)
 {
   return target_remove_fork_catchpoint (PIDGET (inferior_ptid));
 }
@@ -5998,7 +5998,7 @@ static struct breakpoint_ops catch_fork_breakpoint_ops =
 /* Implement the "insert" breakpoint_ops method for vfork catchpoints.  */
 
 static int
-insert_catch_vfork (struct bp_location *b)
+insert_catch_vfork (struct bp_location *bl)
 {
   return target_insert_vfork_catchpoint (PIDGET (inferior_ptid));
 }
@@ -6006,7 +6006,7 @@ insert_catch_vfork (struct bp_location *b)
 /* Implement the "remove" breakpoint_ops method for vfork catchpoints.  */
 
 static int
-remove_catch_vfork (struct bp_location *b)
+remove_catch_vfork (struct bp_location *bl)
 {
   return target_remove_vfork_catchpoint (PIDGET (inferior_ptid));
 }
@@ -6093,19 +6093,19 @@ static struct breakpoint_ops catch_vfork_breakpoint_ops =
    catchpoints.  */
 
 static int
-insert_catch_syscall (struct bp_location *b)
+insert_catch_syscall (struct bp_location *bl)
 {
   struct inferior *inf = current_inferior ();
 
   ++inf->total_syscalls_count;
-  if (!b->owner->syscalls_to_be_caught)
+  if (!bl->owner->syscalls_to_be_caught)
     ++inf->any_syscall_count;
   else
     {
       int i, iter;
 
       for (i = 0;
-           VEC_iterate (int, b->owner->syscalls_to_be_caught, i, iter);
+           VEC_iterate (int, bl->owner->syscalls_to_be_caught, i, iter);
            i++)
 	{
           int elem;
@@ -6137,19 +6137,19 @@ insert_catch_syscall (struct bp_location *b)
    catchpoints.  */
 
 static int
-remove_catch_syscall (struct bp_location *b)
+remove_catch_syscall (struct bp_location *bl)
 {
   struct inferior *inf = current_inferior ();
 
   --inf->total_syscalls_count;
-  if (!b->owner->syscalls_to_be_caught)
+  if (!bl->owner->syscalls_to_be_caught)
     --inf->any_syscall_count;
   else
     {
       int i, iter;
 
       for (i = 0;
-           VEC_iterate (int, b->owner->syscalls_to_be_caught, i, iter);
+           VEC_iterate (int, bl->owner->syscalls_to_be_caught, i, iter);
            i++)
 	{
           int elem;
@@ -6452,13 +6452,13 @@ create_fork_vfork_event_catchpoint (struct gdbarch *gdbarch,
 /* Exec catchpoints.  */
 
 static int
-insert_catch_exec (struct bp_location *b)
+insert_catch_exec (struct bp_location *bl)
 {
   return target_insert_exec_catchpoint (PIDGET (inferior_ptid));
 }
 
 static int
-remove_catch_exec (struct bp_location *b)
+remove_catch_exec (struct bp_location *bl)
 {
   return target_remove_exec_catchpoint (PIDGET (inferior_ptid));
 }
@@ -8115,19 +8115,19 @@ watchpoint_exp_is_const (const struct expression *exp)
 /* Implement the "insert" breakpoint_ops method for hardware watchpoints.  */
 
 static int
-insert_watchpoint (struct bp_location *bpt)
+insert_watchpoint (struct bp_location *bl)
 {
-  return target_insert_watchpoint (bpt->address, bpt->length,
-				   bpt->watchpoint_type, bpt->owner->cond_exp);
+  return target_insert_watchpoint (bl->address, bl->length,
+				   bl->watchpoint_type, bl->owner->cond_exp);
 }
 
 /* Implement the "remove" breakpoint_ops method for hardware watchpoints.  */
 
 static int
-remove_watchpoint (struct bp_location *bpt)
+remove_watchpoint (struct bp_location *bl)
 {
-  return target_remove_watchpoint (bpt->address, bpt->length,
-				   bpt->watchpoint_type, bpt->owner->cond_exp);
+  return target_remove_watchpoint (bl->address, bl->length,
+				   bl->watchpoint_type, bl->owner->cond_exp);
 }
 
 /* The breakpoint_ops structure to be used in hardware watchpoints.  */
@@ -8150,21 +8150,20 @@ static struct breakpoint_ops watchpoint_breakpoint_ops =
    ranged hardware watchpoints.  */
 
 static int
-insert_ranged_watchpoint (struct bp_location *bpt)
+insert_ranged_watchpoint (struct bp_location *bl)
 {
-  return target_insert_ranged_watchpoint (bpt->address,
-					  bpt->length,
-					  bpt->watchpoint_type);
+  return target_insert_ranged_watchpoint (bl->address, bl->length,
+					  bl->watchpoint_type);
 }
 
 /* Implement the "remove" breakpoint_ops method for
    ranged hardware watchpoints.  */
 
 static int
-remove_ranged_watchpoint (struct bp_location *bpt)
+remove_ranged_watchpoint (struct bp_location *bl)
 {
-  return target_remove_ranged_watchpoint (bpt->address, bpt->length,
-					  bpt->watchpoint_type);
+  return target_remove_ranged_watchpoint (bl->address, bl->length,
+					  bl->watchpoint_type);
 }
 
 /* Implement the "extra_resources_needed" breakpoint_ops method for
@@ -8295,20 +8294,20 @@ static struct breakpoint_ops ranged_watchpoint_breakpoint_ops =
    masked hardware watchpoints.  */
 
 static int
-insert_masked_watchpoint (struct bp_location *bpt)
+insert_masked_watchpoint (struct bp_location *bl)
 {
-  return target_insert_mask_watchpoint (bpt->address, bpt->owner->hw_wp_mask,
-					bpt->watchpoint_type);
+  return target_insert_mask_watchpoint (bl->address, bl->owner->hw_wp_mask,
+					bl->watchpoint_type);
 }
 
 /* Implement the "remove" breakpoint_ops method for
    masked hardware watchpoints.  */
 
 static int
-remove_masked_watchpoint (struct bp_location *bpt)
+remove_masked_watchpoint (struct bp_location *bl)
 {
-  return target_remove_mask_watchpoint (bpt->address, bpt->owner->hw_wp_mask,
-				        bpt->watchpoint_type);
+  return target_remove_mask_watchpoint (bl->address, bl->owner->hw_wp_mask,
+				        bl->watchpoint_type);
 }
 
 /* Implement the "extra_resources_needed" breakpoint_ops method for
@@ -9842,7 +9841,7 @@ clear_command (char *arg, int from_tty)
 void
 breakpoint_auto_delete (bpstat bs)
 {
-  struct breakpoint *b, *temp;
+  struct breakpoint *b, *b_tmp;
 
   for (; bs; bs = bs->next)
     if (bs->breakpoint_at
@@ -9850,7 +9849,7 @@ breakpoint_auto_delete (bpstat bs)
 	&& bs->stop)
       delete_breakpoint (bs->breakpoint_at);
 
-  ALL_BREAKPOINTS_SAFE (b, temp)
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
   {
     if (b->disposition == disp_del_at_next_stop)
       delete_breakpoint (b);
@@ -10401,7 +10400,7 @@ do_delete_breakpoint (struct breakpoint *b, void *ignore)
 void
 delete_command (char *arg, int from_tty)
 {
-  struct breakpoint *b, *temp;
+  struct breakpoint *b, *b_tmp;
 
   dont_repeat ();
 
@@ -10433,7 +10432,7 @@ delete_command (char *arg, int from_tty)
       if (!from_tty
 	  || (breaks_to_delete && query (_("Delete all breakpoints? "))))
 	{
-	  ALL_BREAKPOINTS_SAFE (b, temp)
+	  ALL_BREAKPOINTS_SAFE (b, b_tmp)
 	  {
 	    if (b->type != bp_call_dummy
 		&& b->type != bp_std_terminate
@@ -10938,7 +10937,7 @@ breakpoint_re_set_one (void *bint)
 void
 breakpoint_re_set (void)
 {
-  struct breakpoint *b, *temp;
+  struct breakpoint *b, *b_tmp;
   enum language save_language;
   int save_input_radix;
   struct cleanup *old_chain;
@@ -10947,7 +10946,7 @@ breakpoint_re_set (void)
   save_input_radix = input_radix;
   old_chain = save_current_program_space ();
 
-  ALL_BREAKPOINTS_SAFE (b, temp)
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
   {
     /* Format possible error msg */
     char *message = xstrprintf ("Error in re-setting breakpoint %d: ",
@@ -11837,7 +11836,7 @@ disable_trace_command (char *args, int from_tty)
 static void
 delete_trace_command (char *arg, int from_tty)
 {
-  struct breakpoint *b, *temp;
+  struct breakpoint *b, *b_tmp;
 
   dont_repeat ();
 
@@ -11861,7 +11860,7 @@ delete_trace_command (char *arg, int from_tty)
       if (!from_tty
 	  || (breaks_to_delete && query (_("Delete all tracepoints? "))))
 	{
-	  ALL_BREAKPOINTS_SAFE (b, temp)
+	  ALL_BREAKPOINTS_SAFE (b, b_tmp)
 	  {
 	    if (is_tracepoint (b)
 		&& b->number >= 0)
@@ -12279,9 +12278,9 @@ struct breakpoint *
 iterate_over_breakpoints (int (*callback) (struct breakpoint *, void *),
 			  void *data)
 {
-  struct breakpoint *b, *temp;
+  struct breakpoint *b, *b_tmp;
 
-  ALL_BREAKPOINTS_SAFE (b, temp)
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
     {
       if ((*callback) (b, data))
 	return b;

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [patch] Renaming: {insert,remove} += _location  [Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops]
  2010-11-16 18:51         ` Jan Kratochvil
@ 2010-11-17  3:47           ` Jan Kratochvil
  2010-11-18 17:13             ` Joel Brobecker
  2010-11-17  3:47           ` [patch] renaming: bp_location: b->bl &co. " Jan Kratochvil
  1 sibling, 1 reply; 24+ messages in thread
From: Jan Kratochvil @ 2010-11-17  3:47 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: Thiago Jung Bauermann, Pedro Alves, gdb-patches

On Tue, 16 Nov 2010 19:51:01 +0100, Jan Kratochvil wrote:
> On Tue, 16 Nov 2010 09:07:21 +0100, Joel Brobecker wrote:
> > How about doing such a rename as a patch on its own?
> 
> OK, going to send one today.

I will check it in after the Thiago's check-in.


Regards,
Jan


gdb/
2010-11-17  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Code cleanup - renaming.
	* breakpoint.c (insert_bp_location): Use the new name
	breakpoint_ops->insert_location.
	(remove_breakpoint_1): Use the new name
	breakpoint_ops->remove_location.
	* breakpoint.h (struct breakpoint_ops): Rename insert to
	insert_location and remove to remove_location.

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1757,7 +1757,7 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
 	      watchpoints.  It's not clear that it's necessary... */
 	   && bl->owner->disposition != disp_del_at_next_stop)
     {
-      val = bl->owner->ops->insert (bl);
+      val = bl->owner->ops->insert_location (bl);
 
       /* If trying to set a read-watchpoint, and it turns out it's not
 	 supported, try emulating one with an access watchpoint.  */
@@ -1784,7 +1784,7 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
 	  if (val == 1)
 	    {
 	      bl->watchpoint_type = hw_access;
-	      val = bl->owner->ops->insert (bl);
+	      val = bl->owner->ops->insert_location (bl);
 
 	      if (val)
 		/* Back to the original value.  */
@@ -1797,9 +1797,10 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
 
   else if (bl->owner->type == bp_catchpoint)
     {
-      gdb_assert (bl->owner->ops != NULL && bl->owner->ops->insert != NULL);
+      gdb_assert (bl->owner->ops != NULL
+		  && bl->owner->ops->insert_location != NULL);
 
-      val = bl->owner->ops->insert (bl);
+      val = bl->owner->ops->insert_location (bl);
       if (val)
 	{
 	  bl->owner->enable_state = bp_disabled;
@@ -2478,7 +2479,7 @@ remove_breakpoint_1 (struct bp_location *bl, insertion_state_t is)
   else if (bl->loc_type == bp_loc_hardware_watchpoint)
     {
       bl->inserted = (is == mark_inserted);
-      bl->owner->ops->remove (bl);
+      bl->owner->ops->remove_location (bl);
 
       /* Failure to remove any of the hardware watchpoints comes here.  */
       if ((is == mark_uninserted) && (bl->inserted))
@@ -2489,9 +2490,10 @@ remove_breakpoint_1 (struct bp_location *bl, insertion_state_t is)
            && breakpoint_enabled (bl->owner)
            && !bl->duplicate)
     {
-      gdb_assert (bl->owner->ops != NULL && bl->owner->ops->remove != NULL);
+      gdb_assert (bl->owner->ops != NULL
+		  && bl->owner->ops->remove_location != NULL);
 
-      val = bl->owner->ops->remove (bl);
+      val = bl->owner->ops->remove_location (bl);
       if (val)
 	return val;
 
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -355,13 +355,13 @@ struct breakpoint_ops
   /* Insert the breakpoint or watchpoint or activate the catchpoint.
      Return 0 for success, 1 if the breakpoint, watchpoint or catchpoint
      type is not supported, -1 for failure.  */
-  int (*insert) (struct bp_location *);
+  int (*insert_location) (struct bp_location *);
 
   /* Remove the breakpoint/catchpoint that was previously inserted
      with the "insert" method above.  Return 0 for success, 1 if the
      breakpoint, watchpoint or catchpoint type is not supported,
      -1 for failure.  */
-  int (*remove) (struct bp_location *);
+  int (*remove_location) (struct bp_location *);
 
   /* Return non-zero if the debugger should tell the user that this
      breakpoint was hit.  */

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [patch] Renaming: {insert,remove} += _location  [Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops]
  2010-11-17  3:47           ` [patch] Renaming: {insert,remove} += _location [Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops] Jan Kratochvil
@ 2010-11-18 17:13             ` Joel Brobecker
  0 siblings, 0 replies; 24+ messages in thread
From: Joel Brobecker @ 2010-11-18 17:13 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: Thiago Jung Bauermann, Pedro Alves, gdb-patches

> gdb/
> 2010-11-17  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Code cleanup - renaming.
> 	* breakpoint.c (insert_bp_location): Use the new name
> 	breakpoint_ops->insert_location.
> 	(remove_breakpoint_1): Use the new name
> 	breakpoint_ops->remove_location.
> 	* breakpoint.h (struct breakpoint_ops): Rename insert to
> 	insert_location and remove to remove_location.

I didn't think it would change much, but I changed my mind - I like
this change! Thanks for pursuing this.

-- 
Joel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [patch] renaming: bp_location: b->bl &co.  [Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops]
  2010-11-17  3:47           ` [patch] renaming: bp_location: b->bl &co. " Jan Kratochvil
@ 2010-11-18 17:15             ` Joel Brobecker
  2010-12-23 18:50             ` Thiago Jung Bauermann
  1 sibling, 0 replies; 24+ messages in thread
From: Joel Brobecker @ 2010-11-18 17:15 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: Thiago Jung Bauermann, Pedro Alves, gdb-patches

> gdb/
> 2010-11-17  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Code cleanup - renaming.
> 	* breakpoint.c: Use bl for `*bp_location' variables, blp_tmp for
> 	`**bp_location' helper variables, b_tmp for `*breakpoint' helper
> 	variables.

Just for the record, I did a quick scan of this one. Looks good to me.

-- 
Joel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops
  2010-11-16 19:03       ` Thiago Jung Bauermann
@ 2010-11-18 17:18         ` Joel Brobecker
  2010-11-19 20:10           ` Thiago Jung Bauermann
  0 siblings, 1 reply; 24+ messages in thread
From: Joel Brobecker @ 2010-11-18 17:18 UTC (permalink / raw)
  To: Thiago Jung Bauermann; +Cc: gdb-patches

> I kept the wording as similar as possible to the original. Your
> suggestion is then to have the code below instead?
> 
> +	  if (val == 1)
> +	    warning (_("\
> +Error inserting catchpoint %d: Your system does not support this type of catchpoint."),
> +		     bpt->owner->number);
> +	  else
> +	    warning (_("Error inserting catchpoint %d."), bpt->owner->number);

Yep!

> What about these additional comments (I'll send the updated patch after
> understanding your suggestion above)?

They look good to me.  IMO, you can even drop the first hunk,
documenting the return value inside the target_ops vector definition.
I'd rather have no documentation than an incomplete duplicate of
the actual documentation located elsewhere.  The idea is that, if
there is no documentation, then it might help trigger a search of
that documentation elsewhere.  If there is some, then we might not
have the idea to look elsewhere for the rest...

-- 
Joel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops
  2010-11-18 17:18         ` Joel Brobecker
@ 2010-11-19 20:10           ` Thiago Jung Bauermann
  2010-12-23 19:06             ` Thiago Jung Bauermann
  0 siblings, 1 reply; 24+ messages in thread
From: Thiago Jung Bauermann @ 2010-11-19 20:10 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: gdb-patches

On Thu, 2010-11-18 at 09:18 -0800, Joel Brobecker wrote:
> > I kept the wording as similar as possible to the original. Your
> > suggestion is then to have the code below instead?
> > 
> > +	  if (val == 1)
> > +	    warning (_("\
> > +Error inserting catchpoint %d: Your system does not support this type of catchpoint."),
> > +		     bpt->owner->number);
> > +	  else
> > +	    warning (_("Error inserting catchpoint %d."), bpt->owner->number);
> 
> Yep!

Great. I had to break the string right after "this type" since it would
go over 80 characters, but otherwise I used that string.

> > What about these additional comments (I'll send the updated patch after
> > understanding your suggestion above)?
> 
> They look good to me.  IMO, you can even drop the first hunk,
> documenting the return value inside the target_ops vector definition.
> I'd rather have no documentation than an incomplete duplicate of
> the actual documentation located elsewhere.  The idea is that, if
> there is no documentation, then it might help trigger a search of
> that documentation elsewhere.  If there is some, then we might not
> have the idea to look elsewhere for the rest...

Sounds reasonable.

Here's the new version. I understand you approved it already, but it
seems more useful to me to wait until patch 2/2 to be approved too so
that I can commit both together (this one doesn't do anything on its
own).
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


2010-11-19  Thiago Jung Bauermann  <bauerman@br.ibm.com>

	Convert hardware watchpoints to use breakpoint_ops.

gdb/
	* breakpoint.h (breakpoint_ops) <insert>: Return int instead of void.
	Accept pointer to struct bp_location instead of pointer to
	struct breakpoint.  Adapt all implementations.
	(breakpoint_ops) <remove>: Accept pointer to struct bp_location instead
	of pointer to struct breakpoint.  Adapt all implementations.
	* breakpoint.c (insert_catchpoint): Delete function.
	(insert_bp_location): Call the watchpoint or catchpoint's
	breakpoint_ops.insert method.
	(remove_breakpoint_1): Call the watchpoint or catchpoint's
	breakpoint_ops.remove method.
	(insert_watchpoint, remove_watchpoint): New functions.
	(watchpoint_breakpoint_ops): New structure.
	(watch_command_1): Initialize the OPS field.
	* inf-child.c (inf_child_insert_fork_catchpoint)
	(inf_child_remove_fork_catchpoint, inf_child_insert_vfork_catchpoint)
	(inf_child_remove_vfork_catchpoint, inf_child_insert_exec_catchpoint)
	(inf_child_remove_exec_catchpoint, inf_child_set_syscall_catchpoint):
	Delete functions.
	(inf_child_target): Remove initialization of to_insert_fork_catchpoint,
	to_remove_fork_catchpoint, to_insert_vfork_catchpoint,
	to_remove_vfork_catchpoint, to_insert_exec_catchpoint,
	to_remove_exec_catchpoint and to_set_syscall_catchpoint.
	* target.c (update_current_target): Change default implementation of
	to_insert_fork_catchpoint, to_remove_fork_catchpoint,
	to_insert_vfork_catchpoint, to_remove_vfork_catchpoint,
	to_insert_exec_catchpoint, to_remove_exec_catchpoint and
	to_set_syscall_catchpoint to return_one.
	(debug_to_insert_fork_catchpoint, debug_to_insert_vfork_catchpoint)
	(debug_to_insert_exec_catchpoint): Report return value.
	* target.h (to_insert_fork_catchpoint, to_insert_vfork_catchpoint)
	(to_insert_fork_catchpoint): Change declaration to return int instead
	of void.

gdb/testsuite/
	* gdb.base/foll-exec.exp: Adapt to new error string when the catchpoint
	type is not supported.
	* gdb.base/foll-fork.exp: Likewise.
	* gdb.base/foll-vfork.exp: Likewise.


diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 60bc3f8..55a08c7 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1191,18 +1191,6 @@ breakpoint_restore_shadows (gdb_byte *buf, ULONGEST memaddr, LONGEST len)
 }
 \f
 
-/* A wrapper function for inserting catchpoints.  */
-static void
-insert_catchpoint (struct ui_out *uo, void *args)
-{
-  struct breakpoint *b = (struct breakpoint *) args;
-
-  gdb_assert (b->type == bp_catchpoint);
-  gdb_assert (b->ops != NULL && b->ops->insert != NULL);
-
-  b->ops->insert (b);
-}
-
 /* Return true if BPT is of any hardware watchpoint kind.  */
 
 static int
@@ -1739,10 +1727,9 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
 	      watchpoints.  It's not clear that it's necessary... */
 	   && bpt->owner->disposition != disp_del_at_next_stop)
     {
-      val = target_insert_watchpoint (bpt->address,
-				      bpt->length,
-				      bpt->watchpoint_type,
-				      bpt->owner->cond_exp);
+      gdb_assert (bpt->owner->ops != NULL && bpt->owner->ops->insert != NULL);
+
+      val = bpt->owner->ops->insert (bpt);
 
       /* If trying to set a read-watchpoint, and it turns out it's not
 	 supported, try emulating one with an access watchpoint.  */
@@ -1768,12 +1755,12 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
 
 	  if (val == 1)
 	    {
-	      val = target_insert_watchpoint (bpt->address,
-					      bpt->length,
-					      hw_access,
-					      bpt->owner->cond_exp);
-	      if (val == 0)
-		bpt->watchpoint_type = hw_access;
+	      bpt->watchpoint_type = hw_access;
+	      val = bpt->owner->ops->insert (bpt);
+
+	      if (val)
+		/* Back to the original value.  */
+		bpt->watchpoint_type = hw_read;
 	    }
 	}
 
@@ -1785,14 +1772,22 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
 
   else if (bpt->owner->type == bp_catchpoint)
     {
-      struct gdb_exception e = catch_exception (uiout, insert_catchpoint,
-						bpt->owner, RETURN_MASK_ERROR);
-      exception_fprintf (gdb_stderr, e, "warning: inserting catchpoint %d: ",
-			 bpt->owner->number);
-      if (e.reason < 0)
-	bpt->owner->enable_state = bp_disabled;
-      else
-	bpt->inserted = 1;
+      gdb_assert (bpt->owner->ops != NULL && bpt->owner->ops->insert != NULL);
+
+      val = bpt->owner->ops->insert (bpt);
+      if (val)
+	{
+	  bpt->owner->enable_state = bp_disabled;
+
+	  if (val == 1)
+	    warning (_("\
+Error inserting catchpoint %d: Your system does not support this type\n\
+of catchpoint."), bpt->owner->number);
+	  else
+	    warning (_("Error inserting catchpoint %d."), bpt->owner->number);
+	}
+
+      bpt->inserted = (val == 0);
 
       /* We've already printed an error message if there was a problem
 	 inserting this catchpoint, and we've disabled the catchpoint,
@@ -2465,9 +2460,10 @@ remove_breakpoint_1 (struct bp_location *b, insertion_state_t is)
     }
   else if (b->loc_type == bp_loc_hardware_watchpoint)
     {
+      gdb_assert (b->owner->ops != NULL && b->owner->ops->remove != NULL);
+
       b->inserted = (is == mark_inserted);
-      val = target_remove_watchpoint (b->address, b->length,
-				      b->watchpoint_type, b->owner->cond_exp);
+      b->owner->ops->remove (b);
 
       /* Failure to remove any of the hardware watchpoints comes here.  */
       if ((is == mark_uninserted) && (b->inserted))
@@ -2480,9 +2476,10 @@ remove_breakpoint_1 (struct bp_location *b, insertion_state_t is)
     {
       gdb_assert (b->owner->ops != NULL && b->owner->ops->remove != NULL);
 
-      val = b->owner->ops->remove (b->owner);
+      val = b->owner->ops->remove (b);
       if (val)
 	return val;
+
       b->inserted = (is == mark_inserted);
     }
 
@@ -5867,16 +5864,16 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
 
 /* Implement the "insert" breakpoint_ops method for fork catchpoints.  */
 
-static void
-insert_catch_fork (struct breakpoint *b)
+static int
+insert_catch_fork (struct bp_location *b)
 {
-  target_insert_fork_catchpoint (PIDGET (inferior_ptid));
+  return target_insert_fork_catchpoint (PIDGET (inferior_ptid));
 }
 
 /* Implement the "remove" breakpoint_ops method for fork catchpoints.  */
 
 static int
-remove_catch_fork (struct breakpoint *b)
+remove_catch_fork (struct bp_location *b)
 {
   return target_remove_fork_catchpoint (PIDGET (inferior_ptid));
 }
@@ -5959,16 +5956,16 @@ static struct breakpoint_ops catch_fork_breakpoint_ops =
 
 /* Implement the "insert" breakpoint_ops method for vfork catchpoints.  */
 
-static void
-insert_catch_vfork (struct breakpoint *b)
+static int
+insert_catch_vfork (struct bp_location *b)
 {
-  target_insert_vfork_catchpoint (PIDGET (inferior_ptid));
+  return target_insert_vfork_catchpoint (PIDGET (inferior_ptid));
 }
 
 /* Implement the "remove" breakpoint_ops method for vfork catchpoints.  */
 
 static int
-remove_catch_vfork (struct breakpoint *b)
+remove_catch_vfork (struct bp_location *b)
 {
   return target_remove_vfork_catchpoint (PIDGET (inferior_ptid));
 }
@@ -6051,20 +6048,20 @@ static struct breakpoint_ops catch_vfork_breakpoint_ops =
 /* Implement the "insert" breakpoint_ops method for syscall
    catchpoints.  */
 
-static void
-insert_catch_syscall (struct breakpoint *b)
+static int
+insert_catch_syscall (struct bp_location *b)
 {
   struct inferior *inf = current_inferior ();
 
   ++inf->total_syscalls_count;
-  if (!b->syscalls_to_be_caught)
+  if (!b->owner->syscalls_to_be_caught)
     ++inf->any_syscall_count;
   else
     {
       int i, iter;
 
       for (i = 0;
-           VEC_iterate (int, b->syscalls_to_be_caught, i, iter);
+           VEC_iterate (int, b->owner->syscalls_to_be_caught, i, iter);
            i++)
 	{
           int elem;
@@ -6085,30 +6082,30 @@ insert_catch_syscall (struct breakpoint *b)
 	}
     }
 
-  target_set_syscall_catchpoint (PIDGET (inferior_ptid),
-				 inf->total_syscalls_count != 0,
-				 inf->any_syscall_count,
-				 VEC_length (int, inf->syscalls_counts),
-				 VEC_address (int, inf->syscalls_counts));
+  return target_set_syscall_catchpoint (PIDGET (inferior_ptid),
+					inf->total_syscalls_count != 0,
+					inf->any_syscall_count,
+					VEC_length (int, inf->syscalls_counts),
+					VEC_address (int, inf->syscalls_counts));
 }
 
 /* Implement the "remove" breakpoint_ops method for syscall
    catchpoints.  */
 
 static int
-remove_catch_syscall (struct breakpoint *b)
+remove_catch_syscall (struct bp_location *b)
 {
   struct inferior *inf = current_inferior ();
 
   --inf->total_syscalls_count;
-  if (!b->syscalls_to_be_caught)
+  if (!b->owner->syscalls_to_be_caught)
     --inf->any_syscall_count;
   else
     {
       int i, iter;
 
       for (i = 0;
-           VEC_iterate (int, b->syscalls_to_be_caught, i, iter);
+           VEC_iterate (int, b->owner->syscalls_to_be_caught, i, iter);
            i++)
 	{
           int elem;
@@ -6407,14 +6404,14 @@ create_fork_vfork_event_catchpoint (struct gdbarch *gdbarch,
 
 /* Exec catchpoints.  */
 
-static void
-insert_catch_exec (struct breakpoint *b)
+static int
+insert_catch_exec (struct bp_location *b)
 {
-  target_insert_exec_catchpoint (PIDGET (inferior_ptid));
+  return target_insert_exec_catchpoint (PIDGET (inferior_ptid));
 }
 
 static int
-remove_catch_exec (struct breakpoint *b)
+remove_catch_exec (struct bp_location *b)
 {
   return target_remove_exec_catchpoint (PIDGET (inferior_ptid));
 }
@@ -8058,6 +8055,37 @@ watchpoint_exp_is_const (const struct expression *exp)
   return 1;
 }
 
+/* Implement the "insert" breakpoint_ops method for hardware watchpoints.  */
+
+static int
+insert_watchpoint (struct bp_location *bpt)
+{
+  return target_insert_watchpoint (bpt->address, bpt->length,
+				   bpt->watchpoint_type, bpt->owner->cond_exp);
+}
+
+/* Implement the "remove" breakpoint_ops method for hardware watchpoints.  */
+
+static int
+remove_watchpoint (struct bp_location *bpt)
+{
+  return target_remove_watchpoint (bpt->address, bpt->length,
+				   bpt->watchpoint_type, bpt->owner->cond_exp);
+}
+
+/* The breakpoint_ops structure to be used in hardware watchpoints.  */
+
+static struct breakpoint_ops watchpoint_breakpoint_ops =
+{
+  insert_watchpoint,
+  remove_watchpoint,
+  NULL, /* breakpoint_hit */
+  NULL, /* print_it */
+  NULL, /* print_one */
+  NULL, /* print_mention */
+  NULL  /* print_recreate */
+};
+
 /* accessflag:  hw_write:  watch write, 
                 hw_read:   watch read, 
 		hw_access: watch access (read or write) */
@@ -8300,6 +8328,8 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
     b->exp_string = savestring (exp_start, exp_end - exp_start);
   b->val = val;
   b->val_valid = 1;
+  b->ops = &watchpoint_breakpoint_ops;
+
   if (cond_start)
     b->cond_string = savestring (cond_start, cond_end - cond_start);
   else
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index e34c2d3..84f8a39 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -349,16 +349,18 @@ struct bp_location
    will be called instead of the performing the default action for this
    bptype.  */
 
-struct breakpoint_ops 
+struct breakpoint_ops
 {
-  /* Insert the breakpoint or activate the catchpoint.  Should raise
-     an exception if the operation failed.  */
-  void (*insert) (struct breakpoint *);
+  /* Insert the breakpoint or watchpoint or activate the catchpoint.
+     Return 0 for success, 1 if the breakpoint, watchpoint or catchpoint
+     type is not supported, -1 for failure.  */
+  int (*insert) (struct bp_location *);
 
   /* Remove the breakpoint/catchpoint that was previously inserted
-     with the "insert" method above.  Return non-zero if the operation
-     succeeded.  */
-  int (*remove) (struct breakpoint *);
+     with the "insert" method above.  Return 0 for success, 1 if the
+     breakpoint, watchpoint or catchpoint type is not supported,
+     -1 for failure.  */
+  int (*remove) (struct bp_location *);
 
   /* Return non-zero if the debugger should tell the user that this
      breakpoint was hit.  */
diff --git a/gdb/inf-child.c b/gdb/inf-child.c
index 72a18e4..810f688 100644
--- a/gdb/inf-child.c
+++ b/gdb/inf-child.c
@@ -94,36 +94,6 @@ inf_child_acknowledge_created_inferior (int pid)
      created inferior" operation by a debugger.  */
 }
 
-static void
-inf_child_insert_fork_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of fork
-     events.  */
-}
-
-static int
-inf_child_remove_fork_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of fork
-     events.  */
-  return 0;
-}
-
-static void
-inf_child_insert_vfork_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of vfork
-     events.  */
-}
-
-static int
-inf_child_remove_vfork_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of vfork
-     events.  */
-  return 0;
-}
-
 static int
 inf_child_follow_fork (struct target_ops *ops, int follow_child)
 {
@@ -132,30 +102,6 @@ inf_child_follow_fork (struct target_ops *ops, int follow_child)
   return 0;
 }
 
-static void
-inf_child_insert_exec_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of exec
-     events.  */
-}
-
-static int
-inf_child_remove_exec_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of exec
-     events.  */
-  return 0;
-}
-
-static int
-inf_child_set_syscall_catchpoint (int pid, int needed, int any_count,
-				  int table_size, int *table)
-{
-  /* This version of Unix doesn't support notification of syscall
-     events.  */
-  return 0;
-}
-
 static int
 inf_child_can_run (void)
 {
@@ -193,14 +139,7 @@ inf_child_target (void)
   t->to_terminal_info = child_terminal_info;
   t->to_post_startup_inferior = inf_child_post_startup_inferior;
   t->to_acknowledge_created_inferior = inf_child_acknowledge_created_inferior;
-  t->to_insert_fork_catchpoint = inf_child_insert_fork_catchpoint;
-  t->to_remove_fork_catchpoint = inf_child_remove_fork_catchpoint;
-  t->to_insert_vfork_catchpoint = inf_child_insert_vfork_catchpoint;
-  t->to_remove_vfork_catchpoint = inf_child_remove_vfork_catchpoint;
   t->to_follow_fork = inf_child_follow_fork;
-  t->to_insert_exec_catchpoint = inf_child_insert_exec_catchpoint;
-  t->to_remove_exec_catchpoint = inf_child_remove_exec_catchpoint;
-  t->to_set_syscall_catchpoint = inf_child_set_syscall_catchpoint;
   t->to_can_run = inf_child_can_run;
   t->to_pid_to_exec_file = inf_child_pid_to_exec_file;
   t->to_stratum = process_stratum;
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 0e18034..4bea363 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -961,36 +961,34 @@ Attaching after process %d fork to child process %d.\n"),
 }
 
 \f
-static void
+static int
 linux_child_insert_fork_catchpoint (int pid)
 {
-  if (! linux_supports_tracefork (pid))
-    error (_("Your system does not support fork catchpoints."));
+  return !linux_supports_tracefork (pid);
 }
 
-static void
+static int
 linux_child_insert_vfork_catchpoint (int pid)
 {
-  if (!linux_supports_tracefork (pid))
-    error (_("Your system does not support vfork catchpoints."));
+  return !linux_supports_tracefork (pid);
 }
 
-static void
+static int
 linux_child_insert_exec_catchpoint (int pid)
 {
-  if (!linux_supports_tracefork (pid))
-    error (_("Your system does not support exec catchpoints."));
+  return !linux_supports_tracefork (pid);
 }
 
 static int
 linux_child_set_syscall_catchpoint (int pid, int needed, int any_count,
 				    int table_size, int *table)
 {
-  if (! linux_supports_tracesysgood (pid))
-    error (_("Your system does not support syscall catchpoints."));
+  if (!linux_supports_tracesysgood (pid))
+    return 1;
+
   /* On GNU/Linux, we ignore the arguments.  It means that we only
      enable the syscall catchpoints, but do not disable them.
-     
+
      Also, we do not use the `table' information because we do not
      filter system calls here.  We let GDB do the logic for us.  */
   return 0;
diff --git a/gdb/target.c b/gdb/target.c
index 5984125..dd976c9 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -782,26 +782,26 @@ update_current_target (void)
 	    (void (*) (int))
 	    target_ignore);
   de_fault (to_insert_fork_catchpoint,
-	    (void (*) (int))
-	    tcomplain);
+	    (int (*) (int))
+	    return_one);
   de_fault (to_remove_fork_catchpoint,
 	    (int (*) (int))
-	    tcomplain);
+	    return_one);
   de_fault (to_insert_vfork_catchpoint,
-	    (void (*) (int))
-	    tcomplain);
+	    (int (*) (int))
+	    return_one);
   de_fault (to_remove_vfork_catchpoint,
 	    (int (*) (int))
-	    tcomplain);
+	    return_one);
   de_fault (to_insert_exec_catchpoint,
-	    (void (*) (int))
-	    tcomplain);
+	    (int (*) (int))
+	    return_one);
   de_fault (to_remove_exec_catchpoint,
 	    (int (*) (int))
-	    tcomplain);
+	    return_one);
   de_fault (to_set_syscall_catchpoint,
 	    (int (*) (int, int, int, int, int *))
-	    tcomplain);
+	    return_one);
   de_fault (to_has_exited,
 	    (int (*) (int, int, int *))
 	    return_zero);
@@ -3657,13 +3657,17 @@ debug_to_acknowledge_created_inferior (int pid)
 		      pid);
 }
 
-static void
+static int
 debug_to_insert_fork_catchpoint (int pid)
 {
-  debug_target.to_insert_fork_catchpoint (pid);
+  int retval;
 
-  fprintf_unfiltered (gdb_stdlog, "target_insert_fork_catchpoint (%d)\n",
-		      pid);
+  retval = debug_target.to_insert_fork_catchpoint (pid);
+
+  fprintf_unfiltered (gdb_stdlog, "target_insert_fork_catchpoint (%d) = %d\n",
+		      pid, retval);
+
+  return retval;
 }
 
 static int
@@ -3679,13 +3683,17 @@ debug_to_remove_fork_catchpoint (int pid)
   return retval;
 }
 
-static void
+static int
 debug_to_insert_vfork_catchpoint (int pid)
 {
-  debug_target.to_insert_vfork_catchpoint (pid);
+  int retval;
 
-  fprintf_unfiltered (gdb_stdlog, "target_insert_vfork_catchpoint (%d)\n",
-		      pid);
+  retval = debug_target.to_insert_vfork_catchpoint (pid);
+
+  fprintf_unfiltered (gdb_stdlog, "target_insert_vfork_catchpoint (%d) = %d\n",
+		      pid, retval);
+
+  return retval;
 }
 
 static int
@@ -3701,13 +3709,17 @@ debug_to_remove_vfork_catchpoint (int pid)
   return retval;
 }
 
-static void
+static int
 debug_to_insert_exec_catchpoint (int pid)
 {
-  debug_target.to_insert_exec_catchpoint (pid);
+  int retval;
 
-  fprintf_unfiltered (gdb_stdlog, "target_insert_exec_catchpoint (%d)\n",
-		      pid);
+  retval = debug_target.to_insert_exec_catchpoint (pid);
+
+  fprintf_unfiltered (gdb_stdlog, "target_insert_exec_catchpoint (%d) = %d\n",
+		      pid, retval);
+
+  return retval;
 }
 
 static int
diff --git a/gdb/target.h b/gdb/target.h
index 7290d90..bc70107 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -468,12 +468,12 @@ struct target_ops
 				char *, char *, char **, int);
     void (*to_post_startup_inferior) (ptid_t);
     void (*to_acknowledge_created_inferior) (int);
-    void (*to_insert_fork_catchpoint) (int);
+    int (*to_insert_fork_catchpoint) (int);
     int (*to_remove_fork_catchpoint) (int);
-    void (*to_insert_vfork_catchpoint) (int);
+    int (*to_insert_vfork_catchpoint) (int);
     int (*to_remove_vfork_catchpoint) (int);
     int (*to_follow_fork) (struct target_ops *, int);
-    void (*to_insert_exec_catchpoint) (int);
+    int (*to_insert_exec_catchpoint) (int);
     int (*to_remove_exec_catchpoint) (int);
     int (*to_set_syscall_catchpoint) (int, int, int, int, int *);
     int (*to_has_exited) (int, int, int *);
@@ -1034,7 +1034,8 @@ void target_create_inferior (char *exec_file, char *args,
 
 /* On some targets, we can catch an inferior fork or vfork event when
    it occurs.  These functions insert/remove an already-created
-   catchpoint for such events.  */
+   catchpoint for such events.  They return  0 for success, 1 if the
+   catchpoint type is not supported and -1 for failure.  */
 
 #define target_insert_fork_catchpoint(pid) \
      (*current_target.to_insert_fork_catchpoint) (pid)
@@ -1060,7 +1061,8 @@ int target_follow_fork (int follow_child);
 
 /* On some targets, we can catch an inferior exec event when it
    occurs.  These functions insert/remove an already-created
-   catchpoint for such events.  */
+   catchpoint for such events.  They return  0 for success, 1 if the
+   catchpoint type is not supported and -1 for failure.  */
 
 #define target_insert_exec_catchpoint(pid) \
      (*current_target.to_insert_exec_catchpoint) (pid)
@@ -1083,7 +1085,10 @@ int target_follow_fork (int follow_child);
 
    TABLE is an array of ints, indexed by syscall number.  An element in
    this array is nonzero if that syscall should be caught.  This argument
-   only matters if ANY_COUNT is zero.  */
+   only matters if ANY_COUNT is zero.
+
+   Return 0 for success, 1 if syscall catchpoints are not supported or -1
+   for failure.  */
 
 #define target_set_syscall_catchpoint(pid, needed, any_count, table_size, table) \
      (*current_target.to_set_syscall_catchpoint) (pid, needed, any_count, \
diff --git a/gdb/testsuite/gdb.base/foll-exec.exp b/gdb/testsuite/gdb.base/foll-exec.exp
index 92fd105..e30334d 100644
--- a/gdb/testsuite/gdb.base/foll-exec.exp
+++ b/gdb/testsuite/gdb.base/foll-exec.exp
@@ -89,7 +89,7 @@ proc do_exec_tests {} {
    gdb_test "catch exec" "Catchpoint \[0-9\]* \\(exec\\)" "insert first exec catchpoint"
    set has_exec_catchpoints 0
    gdb_test_multiple "continue" "continue to first exec catchpoint" {
-     -re ".*Your system does not support exec catchpoints.*$gdb_prompt $" {
+     -re ".*Your system does not support this type\r\nof catchpoint.*$gdb_prompt $" {
        unsupported "continue to first exec catchpoint"
      }
      -re ".*Catchpoint.*$gdb_prompt $" {
diff --git a/gdb/testsuite/gdb.base/foll-fork.exp b/gdb/testsuite/gdb.base/foll-fork.exp
index 891aa38..6dcfca1 100644
--- a/gdb/testsuite/gdb.base/foll-fork.exp
+++ b/gdb/testsuite/gdb.base/foll-fork.exp
@@ -45,7 +45,7 @@ proc check_fork_catchpoints {} {
   gdb_test "catch fork" "Catchpoint \[0-9\]* \\(fork\\)" "insert first fork catchpoint"
   set has_fork_catchpoints 0
   gdb_test_multiple "continue" "continue to first fork catchpoint" {
-    -re ".*Your system does not support fork catchpoints.*$gdb_prompt $" {
+    -re ".*Your system does not support this type\r\nof catchpoint.*$gdb_prompt $" {
       unsupported "continue to first fork catchpoint"
     }
     -re ".*Catchpoint.*$gdb_prompt $" {
diff --git a/gdb/testsuite/gdb.base/foll-vfork.exp b/gdb/testsuite/gdb.base/foll-vfork.exp
index e55d700..1b479f2 100644
--- a/gdb/testsuite/gdb.base/foll-vfork.exp
+++ b/gdb/testsuite/gdb.base/foll-vfork.exp
@@ -74,7 +74,7 @@ proc check_vfork_catchpoints {} {
   gdb_test "catch vfork" "Catchpoint \[0-9\]* \\(vfork\\)" "insert first vfork catchpoint"
   set has_vfork_catchpoints 0
   gdb_test_multiple "continue" "continue to first vfork catchpoint" {
-    -re ".*Your system does not support vfork catchpoints.*$gdb_prompt $" {
+    -re ".*Your system does not support this type\r\nof catchpoint.*$gdb_prompt $" {
       unsupported "continue to first vfork catchpoint"
     }
     -re ".*Catchpoint.*$gdb_prompt $" {


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [patch] renaming: bp_location: b->bl &co.  [Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops]
  2010-11-17  3:47           ` [patch] renaming: bp_location: b->bl &co. " Jan Kratochvil
  2010-11-18 17:15             ` Joel Brobecker
@ 2010-12-23 18:50             ` Thiago Jung Bauermann
  2010-12-24  5:14               ` Joel Brobecker
  1 sibling, 1 reply; 24+ messages in thread
From: Thiago Jung Bauermann @ 2010-12-23 18:50 UTC (permalink / raw)
  To: Jan Kratochvil; +Cc: Joel Brobecker, Pedro Alves, gdb-patches

Hi Jan,

On Wed, 2010-11-17 at 04:45 +0100, Jan Kratochvil wrote:
> On Tue, 16 Nov 2010 19:51:01 +0100, Jan Kratochvil wrote:
> > On Tue, 16 Nov 2010 09:07:21 +0100, Joel Brobecker wrote:
> > > How about doing such a rename as a patch on its own?
> > 
> > OK, going to send one today.
> 
> I haven't written a ChangeLog entry but the change safety has been verified by
> stripped code differences.  Still GDB compiles out line numbers and assert
> expressions so some of the differences were roughly checked in objdump -d they
> are safe.
> 
> I will check it in after the Thiago's check-in.

Thanks for the patch! This version applies on the current CVS HEAD. I
incorporated the parts of your patch that touch my PowerPC patches in
the new versions that I'm posting today.

Ok to check this one in? Do you think a more elaborate ChangeLog entry
is needed?
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


2010-12-23  Jan Kratochvil  <jan.kratochvil@redhat.com>

	Code cleanup - renaming.
	* breakpoint.c: Use bl for `*bp_location' variables, blp_tmp for
	`**bp_location' helper variables, b_tmp for `*breakpoint' helper
	variables.


diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 6a51a3b..5736fbc 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1085,14 +1085,15 @@ bp_location_has_shadow (struct bp_location *bl)
    by replacing any memory breakpoints with their shadowed contents.
 
    The range of shadowed area by each bp_location is:
-     b->address - bp_location_placed_address_before_address_max
-     up to b->address + bp_location_shadow_len_after_address_max
+     bl->address - bp_location_placed_address_before_address_max
+     up to bl->address + bp_location_shadow_len_after_address_max
    The range we were requested to resolve shadows for is:
      memaddr ... memaddr + len
    Thus the safe cutoff boundaries for performance optimization are
-     memaddr + len <= b->address - bp_location_placed_address_before_address_max
+     memaddr + len <= (bl->address
+		       - bp_location_placed_address_before_address_max)
    and:
-     b->address + bp_location_shadow_len_after_address_max <= memaddr  */
+     bl->address + bp_location_shadow_len_after_address_max <= memaddr  */
 
 void
 breakpoint_restore_shadows (gdb_byte *buf, ULONGEST memaddr, LONGEST len)
@@ -1107,12 +1108,12 @@ breakpoint_restore_shadows (gdb_byte *buf, ULONGEST memaddr, LONGEST len)
   bc_r = bp_location_count;
   while (bc_l + 1 < bc_r)
     {
-      struct bp_location *b;
+      struct bp_location *bl;
 
       bc = (bc_l + bc_r) / 2;
-      b = bp_location[bc];
+      bl = bp_location[bc];
 
-      /* Check first B->ADDRESS will not overflow due to the added constant.
+      /* Check first BL->ADDRESS will not overflow due to the added constant.
 	 Then advance the left boundary only if we are sure the BC element can
 	 in no way affect the BUF content (MEMADDR to MEMADDR + LEN range).
 
@@ -1120,8 +1121,10 @@ breakpoint_restore_shadows (gdb_byte *buf, ULONGEST memaddr, LONGEST len)
 	 we cannot miss a breakpoint with its shadow range tail still reaching
 	 MEMADDR.  */
 
-      if (b->address + bp_location_shadow_len_after_address_max >= b->address
-	  && b->address + bp_location_shadow_len_after_address_max <= memaddr)
+      if ((bl->address + bp_location_shadow_len_after_address_max
+	   >= bl->address)
+	  && (bl->address + bp_location_shadow_len_after_address_max
+	      <= memaddr))
 	bc_l = bc;
       else
 	bc_r = bc;
@@ -1131,34 +1134,34 @@ breakpoint_restore_shadows (gdb_byte *buf, ULONGEST memaddr, LONGEST len)
 
   for (bc = bc_l; bc < bp_location_count; bc++)
   {
-    struct bp_location *b = bp_location[bc];
+    struct bp_location *bl = bp_location[bc];
     CORE_ADDR bp_addr = 0;
     int bp_size = 0;
     int bptoffset = 0;
 
-    /* bp_location array has B->OWNER always non-NULL.  */
-    if (b->owner->type == bp_none)
+    /* bp_location array has BL->OWNER always non-NULL.  */
+    if (bl->owner->type == bp_none)
       warning (_("reading through apparently deleted breakpoint #%d?"),
-              b->owner->number);
+	       bl->owner->number);
 
     /* Performance optimization: any futher element can no longer affect BUF
        content.  */
 
-    if (b->address >= bp_location_placed_address_before_address_max
-        && memaddr + len <= b->address
-			    - bp_location_placed_address_before_address_max)
+    if (bl->address >= bp_location_placed_address_before_address_max
+        && memaddr + len <= (bl->address
+			     - bp_location_placed_address_before_address_max))
       break;
 
-    if (!bp_location_has_shadow (b))
+    if (!bp_location_has_shadow (bl))
       continue;
-    if (!breakpoint_address_match (b->target_info.placed_address_space, 0,
+    if (!breakpoint_address_match (bl->target_info.placed_address_space, 0,
 				   current_program_space->aspace, 0))
       continue;
 
     /* Addresses and length of the part of the breakpoint that
        we need to copy.  */
-    bp_addr = b->target_info.placed_address;
-    bp_size = b->target_info.shadow_len;
+    bp_addr = bl->target_info.placed_address;
+    bp_size = bl->target_info.shadow_len;
 
     if (bp_addr + bp_size <= memaddr)
       /* The breakpoint is entirely before the chunk of memory we
@@ -1186,7 +1189,7 @@ breakpoint_restore_shadows (gdb_byte *buf, ULONGEST memaddr, LONGEST len)
       }
 
     memcpy (buf + bp_addr - memaddr,
-	    b->target_info.shadow_contents + bptoffset, bp_size);
+	    bl->target_info.shadow_contents + bptoffset, bp_size);
   }
 }
 \f
@@ -1518,15 +1521,15 @@ in which its expression is valid.\n"),
 /* Returns 1 iff breakpoint location should be
    inserted in the inferior.  */
 static int
-should_be_inserted (struct bp_location *bpt)
+should_be_inserted (struct bp_location *bl)
 {
-  if (bpt->owner == NULL || !breakpoint_enabled (bpt->owner))
+  if (bl->owner == NULL || !breakpoint_enabled (bl->owner))
     return 0;
 
-  if (bpt->owner->disposition == disp_del_at_next_stop)
+  if (bl->owner->disposition == disp_del_at_next_stop)
     return 0;
 
-  if (!bpt->enabled || bpt->shlib_disabled || bpt->duplicate)
+  if (!bl->enabled || bl->shlib_disabled || bl->duplicate)
     return 0;
 
   /* This is set for example, when we're attached to the parent of a
@@ -1537,43 +1540,43 @@ should_be_inserted (struct bp_location *bpt)
      memory region, do not insert breakpoints in the parent, otherwise
      the child could still trip on the parent's breakpoints.  Since
      the parent is blocked anyway, it won't miss any breakpoint.  */
-  if (bpt->pspace->breakpoints_not_allowed)
+  if (bl->pspace->breakpoints_not_allowed)
     return 0;
 
   /* Tracepoints are inserted by the target at a time of its choosing,
      not by us.  */
-  if (is_tracepoint (bpt->owner))
+  if (is_tracepoint (bl->owner))
     return 0;
 
   return 1;
 }
 
-/* Insert a low-level "breakpoint" of some type.  BPT is the breakpoint.
-   Any error messages are printed to TMP_ERROR_STREAM; and DISABLED_BREAKS,
-   and HW_BREAKPOINT_ERROR are used to report problems.
+/* Insert a low-level "breakpoint" of some type.  BL is the breakpoint
+   location.  Any error messages are printed to TMP_ERROR_STREAM; and
+   DISABLED_BREAKS, and HW_BREAKPOINT_ERROR are used to report problems.
 
    NOTE drow/2003-09-09: This routine could be broken down to an object-style
    method for each breakpoint or catchpoint type.  */
 static int
-insert_bp_location (struct bp_location *bpt,
+insert_bp_location (struct bp_location *bl,
 		    struct ui_file *tmp_error_stream,
 		    int *disabled_breaks,
 		    int *hw_breakpoint_error)
 {
   int val = 0;
 
-  if (!should_be_inserted (bpt) || bpt->inserted)
+  if (!should_be_inserted (bl) || bl->inserted)
     return 0;
 
   /* Initialize the target-specific information.  */
-  memset (&bpt->target_info, 0, sizeof (bpt->target_info));
-  bpt->target_info.placed_address = bpt->address;
-  bpt->target_info.placed_address_space = bpt->pspace->aspace;
+  memset (&bl->target_info, 0, sizeof (bl->target_info));
+  bl->target_info.placed_address = bl->address;
+  bl->target_info.placed_address_space = bl->pspace->aspace;
 
-  if (bpt->loc_type == bp_loc_software_breakpoint
-      || bpt->loc_type == bp_loc_hardware_breakpoint)
+  if (bl->loc_type == bp_loc_software_breakpoint
+      || bl->loc_type == bp_loc_hardware_breakpoint)
     {
-      if (bpt->owner->type != bp_hardware_breakpoint)
+      if (bl->owner->type != bp_hardware_breakpoint)
 	{
 	  /* If the explicitly specified breakpoint type
 	     is not hardware breakpoint, check the memory map to see
@@ -1591,7 +1594,7 @@ insert_bp_location (struct bp_location *bpt,
 	     problem is that memory map has changed during running program,
 	     but it's not going to work anyway with current gdb.  */
 	  struct mem_region *mr 
-	    = lookup_mem_region (bpt->target_info.placed_address);
+	    = lookup_mem_region (bl->target_info.placed_address);
 	  
 	  if (mr)
 	    {
@@ -1604,11 +1607,11 @@ insert_bp_location (struct bp_location *bpt,
 		  else 
 		    new_type = bp_loc_software_breakpoint;
 		  
-		  if (new_type != bpt->loc_type)
+		  if (new_type != bl->loc_type)
 		    {
 		      static int said = 0;
 
-		      bpt->loc_type = new_type;
+		      bl->loc_type = new_type;
 		      if (!said)
 			{
 			  fprintf_filtered (gdb_stdout, _("\
@@ -1617,26 +1620,26 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
 			}
 		    }
 		}
-	      else if (bpt->loc_type == bp_loc_software_breakpoint
+	      else if (bl->loc_type == bp_loc_software_breakpoint
 		       && mr->attrib.mode != MEM_RW)	    
 		warning (_("cannot set software breakpoint at readonly address %s"),
-			 paddress (bpt->gdbarch, bpt->address));
+			 paddress (bl->gdbarch, bl->address));
 	    }
 	}
         
       /* First check to see if we have to handle an overlay.  */
       if (overlay_debugging == ovly_off
-	  || bpt->section == NULL
-	  || !(section_is_overlay (bpt->section)))
+	  || bl->section == NULL
+	  || !(section_is_overlay (bl->section)))
 	{
 	  /* No overlay handling: just set the breakpoint.  */
 
-	  if (bpt->loc_type == bp_loc_hardware_breakpoint)
-	    val = target_insert_hw_breakpoint (bpt->gdbarch,
-					       &bpt->target_info);
+	  if (bl->loc_type == bp_loc_hardware_breakpoint)
+	    val = target_insert_hw_breakpoint (bl->gdbarch,
+					       &bl->target_info);
 	  else
-	    val = target_insert_breakpoint (bpt->gdbarch,
-					    &bpt->target_info);
+	    val = target_insert_breakpoint (bl->gdbarch,
+					    &bl->target_info);
 	}
       else
 	{
@@ -1647,34 +1650,34 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
 	      /* Yes -- overlay event support is not active, 
 		 so we must try to set a breakpoint at the LMA.
 		 This will not work for a hardware breakpoint.  */
-	      if (bpt->loc_type == bp_loc_hardware_breakpoint)
+	      if (bl->loc_type == bp_loc_hardware_breakpoint)
 		warning (_("hardware breakpoint %d not supported in overlay!"),
-			 bpt->owner->number);
+			 bl->owner->number);
 	      else
 		{
-		  CORE_ADDR addr = overlay_unmapped_address (bpt->address,
-							     bpt->section);
+		  CORE_ADDR addr = overlay_unmapped_address (bl->address,
+							     bl->section);
 		  /* Set a software (trap) breakpoint at the LMA.  */
-		  bpt->overlay_target_info = bpt->target_info;
-		  bpt->overlay_target_info.placed_address = addr;
-		  val = target_insert_breakpoint (bpt->gdbarch,
-						  &bpt->overlay_target_info);
+		  bl->overlay_target_info = bl->target_info;
+		  bl->overlay_target_info.placed_address = addr;
+		  val = target_insert_breakpoint (bl->gdbarch,
+						  &bl->overlay_target_info);
 		  if (val != 0)
 		    fprintf_unfiltered (tmp_error_stream,
 					"Overlay breakpoint %d failed: in ROM?\n",
-					bpt->owner->number);
+					bl->owner->number);
 		}
 	    }
 	  /* Shall we set a breakpoint at the VMA? */
-	  if (section_is_mapped (bpt->section))
+	  if (section_is_mapped (bl->section))
 	    {
 	      /* Yes.  This overlay section is mapped into memory.  */
-	      if (bpt->loc_type == bp_loc_hardware_breakpoint)
-		val = target_insert_hw_breakpoint (bpt->gdbarch,
-						   &bpt->target_info);
+	      if (bl->loc_type == bp_loc_hardware_breakpoint)
+		val = target_insert_hw_breakpoint (bl->gdbarch,
+						   &bl->target_info);
 	      else
-		val = target_insert_breakpoint (bpt->gdbarch,
-						&bpt->target_info);
+		val = target_insert_breakpoint (bl->gdbarch,
+						&bl->target_info);
 	    }
 	  else
 	    {
@@ -1687,40 +1690,40 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
       if (val)
 	{
 	  /* Can't set the breakpoint.  */
-	  if (solib_name_from_address (bpt->pspace, bpt->address))
+	  if (solib_name_from_address (bl->pspace, bl->address))
 	    {
 	      /* See also: disable_breakpoints_in_shlibs. */
 	      val = 0;
-	      bpt->shlib_disabled = 1;
+	      bl->shlib_disabled = 1;
 	      if (!*disabled_breaks)
 		{
 		  fprintf_unfiltered (tmp_error_stream, 
 				      "Cannot insert breakpoint %d.\n", 
-				      bpt->owner->number);
+				      bl->owner->number);
 		  fprintf_unfiltered (tmp_error_stream, 
 				      "Temporarily disabling shared library breakpoints:\n");
 		}
 	      *disabled_breaks = 1;
 	      fprintf_unfiltered (tmp_error_stream,
-				  "breakpoint #%d\n", bpt->owner->number);
+				  "breakpoint #%d\n", bl->owner->number);
 	    }
 	  else
 	    {
-	      if (bpt->loc_type == bp_loc_hardware_breakpoint)
+	      if (bl->loc_type == bp_loc_hardware_breakpoint)
 		{
 		  *hw_breakpoint_error = 1;
 		  fprintf_unfiltered (tmp_error_stream, 
 				      "Cannot insert hardware breakpoint %d.\n",
-				      bpt->owner->number);
+				      bl->owner->number);
 		}
 	      else
 		{
 		  fprintf_unfiltered (tmp_error_stream, 
 				      "Cannot insert breakpoint %d.\n", 
-				      bpt->owner->number);
+				      bl->owner->number);
 		  fprintf_filtered (tmp_error_stream, 
 				    "Error accessing memory address ");
-		  fputs_filtered (paddress (bpt->gdbarch, bpt->address),
+		  fputs_filtered (paddress (bl->gdbarch, bl->address),
 				  tmp_error_stream);
 		  fprintf_filtered (tmp_error_stream, ": %s.\n",
 				    safe_strerror (val));
@@ -1729,24 +1732,24 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
 	    }
 	}
       else
-	bpt->inserted = 1;
+	bl->inserted = 1;
 
       return val;
     }
 
-  else if (bpt->loc_type == bp_loc_hardware_watchpoint
+  else if (bl->loc_type == bp_loc_hardware_watchpoint
 	   /* NOTE drow/2003-09-08: This state only exists for removing
 	      watchpoints.  It's not clear that it's necessary... */
-	   && bpt->owner->disposition != disp_del_at_next_stop)
+	   && bl->owner->disposition != disp_del_at_next_stop)
     {
-      val = target_insert_watchpoint (bpt->address,
-				      bpt->length,
-				      bpt->watchpoint_type,
-				      bpt->owner->cond_exp);
+      val = target_insert_watchpoint (bl->address,
+				      bl->length,
+				      bl->watchpoint_type,
+				      bl->owner->cond_exp);
 
       /* If trying to set a read-watchpoint, and it turns out it's not
 	 supported, try emulating one with an access watchpoint.  */
-      if (val == 1 && bpt->watchpoint_type == hw_read)
+      if (val == 1 && bl->watchpoint_type == hw_read)
 	{
 	  struct bp_location *loc, **loc_temp;
 
@@ -1754,42 +1757,42 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
 	     hw_access location that would be considered a duplicate
 	     of this one.  */
 	  ALL_BP_LOCATIONS (loc, loc_temp)
-	    if (loc != bpt
+	    if (loc != bl
 		&& loc->watchpoint_type == hw_access
-		&& watchpoint_locations_match (bpt, loc))
+		&& watchpoint_locations_match (bl, loc))
 	      {
-		bpt->duplicate = 1;
-		bpt->inserted = 1;
-		bpt->target_info = loc->target_info;
-		bpt->watchpoint_type = hw_access;
+		bl->duplicate = 1;
+		bl->inserted = 1;
+		bl->target_info = loc->target_info;
+		bl->watchpoint_type = hw_access;
 		val = 0;
 		break;
 	      }
 
 	  if (val == 1)
 	    {
-	      val = target_insert_watchpoint (bpt->address,
-					      bpt->length,
+	      val = target_insert_watchpoint (bl->address,
+					      bl->length,
 					      hw_access,
-					      bpt->owner->cond_exp);
+					      bl->owner->cond_exp);
 	      if (val == 0)
-		bpt->watchpoint_type = hw_access;
+		bl->watchpoint_type = hw_access;
 	    }
 	}
 
-      bpt->inserted = (val == 0);
+      bl->inserted = (val == 0);
     }
 
-  else if (bpt->owner->type == bp_catchpoint)
+  else if (bl->owner->type == bp_catchpoint)
     {
       struct gdb_exception e = catch_exception (uiout, insert_catchpoint,
-						bpt->owner, RETURN_MASK_ERROR);
+						bl->owner, RETURN_MASK_ERROR);
       exception_fprintf (gdb_stderr, e, "warning: inserting catchpoint %d: ",
-			 bpt->owner->number);
+			 bl->owner->number);
       if (e.reason < 0)
-	bpt->owner->enable_state = bp_disabled;
+	bl->owner->enable_state = bp_disabled;
       else
-	bpt->inserted = 1;
+	bl->inserted = 1;
 
       /* We've already printed an error message if there was a problem
 	 inserting this catchpoint, and we've disabled the catchpoint,
@@ -1874,7 +1877,7 @@ static void
 insert_breakpoint_locations (void)
 {
   struct breakpoint *bpt;
-  struct bp_location *b, **bp_tmp;
+  struct bp_location *bl, **blp_tmp;
   int error = 0;
   int val = 0;
   int disabled_breaks = 0;
@@ -1889,19 +1892,19 @@ insert_breakpoint_locations (void)
 
   save_current_space_and_thread ();
 
-  ALL_BP_LOCATIONS (b, bp_tmp)
+  ALL_BP_LOCATIONS (bl, blp_tmp)
     {
-      if (!should_be_inserted (b) || b->inserted)
+      if (!should_be_inserted (bl) || bl->inserted)
 	continue;
 
       /* There is no point inserting thread-specific breakpoints if the
-	 thread no longer exists.  ALL_BP_LOCATIONS bp_location has B->OWNER
+	 thread no longer exists.  ALL_BP_LOCATIONS bp_location has BL->OWNER
 	 always non-NULL.  */
-      if (b->owner->thread != -1
-	  && !valid_thread_id (b->owner->thread))
+      if (bl->owner->thread != -1
+	  && !valid_thread_id (bl->owner->thread))
 	continue;
 
-      switch_to_program_space_and_thread (b->pspace);
+      switch_to_program_space_and_thread (bl->pspace);
 
       /* For targets that support global breakpoints, there's no need
 	 to select an inferior to insert breakpoint to.  In fact, even
@@ -1911,8 +1914,7 @@ insert_breakpoint_locations (void)
 	  && ptid_equal (inferior_ptid, null_ptid))
 	continue;
 
-      val = insert_bp_location (b, tmp_error_stream,
-				    &disabled_breaks,
+      val = insert_bp_location (bl, tmp_error_stream, &disabled_breaks,
 				    &hw_breakpoint_error);
       if (val)
 	error = val;
@@ -1974,13 +1976,13 @@ You may have requested too many hardware breakpoints/watchpoints.\n");
 int
 remove_breakpoints (void)
 {
-  struct bp_location *b, **bp_tmp;
+  struct bp_location *bl, **blp_tmp;
   int val = 0;
 
-  ALL_BP_LOCATIONS (b, bp_tmp)
+  ALL_BP_LOCATIONS (bl, blp_tmp)
   {
-    if (b->inserted)
-      val |= remove_breakpoint (b, mark_uninserted);
+    if (bl->inserted)
+      val |= remove_breakpoint (bl, mark_uninserted);
   }
   return val;
 }
@@ -1990,18 +1992,18 @@ remove_breakpoints (void)
 int
 remove_breakpoints_pid (int pid)
 {
-  struct bp_location *b, **b_tmp;
+  struct bp_location *bl, **blp_tmp;
   int val;
   struct inferior *inf = find_inferior_pid (pid);
 
-  ALL_BP_LOCATIONS (b, b_tmp)
+  ALL_BP_LOCATIONS (bl, blp_tmp)
   {
-    if (b->pspace != inf->pspace)
+    if (bl->pspace != inf->pspace)
       continue;
 
-    if (b->inserted)
+    if (bl->inserted)
       {
-	val = remove_breakpoint (b, mark_uninserted);
+	val = remove_breakpoint (bl, mark_uninserted);
 	if (val != 0)
 	  return val;
       }
@@ -2012,13 +2014,13 @@ remove_breakpoints_pid (int pid)
 int
 remove_hw_watchpoints (void)
 {
-  struct bp_location *b, **bp_tmp;
+  struct bp_location *bl, **blp_tmp;
   int val = 0;
 
-  ALL_BP_LOCATIONS (b, bp_tmp)
+  ALL_BP_LOCATIONS (bl, blp_tmp)
   {
-    if (b->inserted && b->loc_type == bp_loc_hardware_watchpoint)
-      val |= remove_breakpoint (b, mark_uninserted);
+    if (bl->inserted && bl->loc_type == bp_loc_hardware_watchpoint)
+      val |= remove_breakpoint (bl, mark_uninserted);
   }
   return val;
 }
@@ -2027,7 +2029,7 @@ int
 reattach_breakpoints (int pid)
 {
   struct cleanup *old_chain;
-  struct bp_location *b, **bp_tmp;
+  struct bp_location *bl, **blp_tmp;
   int val;
   struct ui_file *tmp_error_stream = mem_fileopen ();
   int dummy1 = 0, dummy2 = 0;
@@ -2045,16 +2047,15 @@ reattach_breakpoints (int pid)
 
   make_cleanup_ui_file_delete (tmp_error_stream);
 
-  ALL_BP_LOCATIONS (b, bp_tmp)
+  ALL_BP_LOCATIONS (bl, blp_tmp)
   {
-    if (b->pspace != inf->pspace)
+    if (bl->pspace != inf->pspace)
       continue;
 
-    if (b->inserted)
+    if (bl->inserted)
       {
-	b->inserted = 0;
-	val = insert_bp_location (b, tmp_error_stream,
-				  &dummy1, &dummy2);
+	bl->inserted = 0;
+	val = insert_bp_location (bl, tmp_error_stream, &dummy1, &dummy2);
 	if (val != 0)
 	  {
 	    do_cleanups (old_chain);
@@ -2241,8 +2242,7 @@ create_exception_master_breakpoint (void)
 void
 update_breakpoints_after_exec (void)
 {
-  struct breakpoint *b;
-  struct breakpoint *temp;
+  struct breakpoint *b, *b_tmp;
   struct bp_location *bploc, **bplocp_tmp;
 
   /* We're about to delete breakpoints from GDB's lists.  If the
@@ -2257,7 +2257,7 @@ update_breakpoints_after_exec (void)
     if (bploc->pspace == current_program_space)
       gdb_assert (!bploc->inserted);
 
-  ALL_BREAKPOINTS_SAFE (b, temp)
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
   {
     if (b->pspace != current_program_space)
       continue;
@@ -2364,7 +2364,7 @@ update_breakpoints_after_exec (void)
 int
 detach_breakpoints (int pid)
 {
-  struct bp_location *b, **bp_tmp;
+  struct bp_location *bl, **blp_tmp;
   int val = 0;
   struct cleanup *old_chain = save_inferior_ptid ();
   struct inferior *inf = current_inferior ();
@@ -2374,13 +2374,13 @@ detach_breakpoints (int pid)
 
   /* Set inferior_ptid; remove_breakpoint_1 uses this global.  */
   inferior_ptid = pid_to_ptid (pid);
-  ALL_BP_LOCATIONS (b, bp_tmp)
+  ALL_BP_LOCATIONS (bl, blp_tmp)
   {
-    if (b->pspace != inf->pspace)
+    if (bl->pspace != inf->pspace)
       continue;
 
-    if (b->inserted)
-      val |= remove_breakpoint_1 (b, mark_inserted);
+    if (bl->inserted)
+      val |= remove_breakpoint_1 (bl, mark_inserted);
   }
 
   /* Detach single-step breakpoints as well.  */
@@ -2390,30 +2390,30 @@ detach_breakpoints (int pid)
   return val;
 }
 
-/* Remove the breakpoint location B from the current address space.
+/* Remove the breakpoint location BL from the current address space.
    Note that this is used to detach breakpoints from a child fork.
    When we get here, the child isn't in the inferior list, and neither
    do we have objects to represent its address space --- we should
-   *not* look at b->pspace->aspace here.  */
+   *not* look at bl->pspace->aspace here.  */
 
 static int
-remove_breakpoint_1 (struct bp_location *b, insertion_state_t is)
+remove_breakpoint_1 (struct bp_location *bl, insertion_state_t is)
 {
   int val;
 
-  /* B is never in moribund_locations by our callers.  */
-  gdb_assert (b->owner != NULL);
+  /* BL is never in moribund_locations by our callers.  */
+  gdb_assert (bl->owner != NULL);
 
-  if (b->owner->enable_state == bp_permanent)
+  if (bl->owner->enable_state == bp_permanent)
     /* Permanent breakpoints cannot be inserted or removed.  */
     return 0;
 
   /* The type of none suggests that owner is actually deleted.
      This should not ever happen.  */
-  gdb_assert (b->owner->type != bp_none);
+  gdb_assert (bl->owner->type != bp_none);
 
-  if (b->loc_type == bp_loc_software_breakpoint
-      || b->loc_type == bp_loc_hardware_breakpoint)
+  if (bl->loc_type == bp_loc_software_breakpoint
+      || bl->loc_type == bp_loc_hardware_breakpoint)
     {
       /* "Normal" instruction breakpoint: either the standard
 	 trap-instruction bp (bp_breakpoint), or a
@@ -2421,15 +2421,15 @@ remove_breakpoint_1 (struct bp_location *b, insertion_state_t is)
 
       /* First check to see if we have to handle an overlay.  */
       if (overlay_debugging == ovly_off
-	  || b->section == NULL
-	  || !(section_is_overlay (b->section)))
+	  || bl->section == NULL
+	  || !(section_is_overlay (bl->section)))
 	{
 	  /* No overlay handling: just remove the breakpoint.  */
 
-	  if (b->loc_type == bp_loc_hardware_breakpoint)
-	    val = target_remove_hw_breakpoint (b->gdbarch, &b->target_info);
+	  if (bl->loc_type == bp_loc_hardware_breakpoint)
+	    val = target_remove_hw_breakpoint (bl->gdbarch, &bl->target_info);
 	  else
-	    val = target_remove_breakpoint (b->gdbarch, &b->target_info);
+	    val = target_remove_breakpoint (bl->gdbarch, &bl->target_info);
 	}
       else
 	{
@@ -2442,31 +2442,31 @@ remove_breakpoint_1 (struct bp_location *b, insertion_state_t is)
 		*/
 		/* Ignore any failures: if the LMA is in ROM, we will
 		   have already warned when we failed to insert it.  */
-		if (b->loc_type == bp_loc_hardware_breakpoint)
-		  target_remove_hw_breakpoint (b->gdbarch,
-					       &b->overlay_target_info);
+		if (bl->loc_type == bp_loc_hardware_breakpoint)
+		  target_remove_hw_breakpoint (bl->gdbarch,
+					       &bl->overlay_target_info);
 		else
-		  target_remove_breakpoint (b->gdbarch,
-					    &b->overlay_target_info);
+		  target_remove_breakpoint (bl->gdbarch,
+					    &bl->overlay_target_info);
 	      }
 	  /* Did we set a breakpoint at the VMA? 
 	     If so, we will have marked the breakpoint 'inserted'.  */
-	  if (b->inserted)
+	  if (bl->inserted)
 	    {
 	      /* Yes -- remove it.  Previously we did not bother to
 		 remove the breakpoint if the section had been
 		 unmapped, but let's not rely on that being safe.  We
 		 don't know what the overlay manager might do.  */
-	      if (b->loc_type == bp_loc_hardware_breakpoint)
-		val = target_remove_hw_breakpoint (b->gdbarch,
-						   &b->target_info);
+	      if (bl->loc_type == bp_loc_hardware_breakpoint)
+		val = target_remove_hw_breakpoint (bl->gdbarch,
+						   &bl->target_info);
 
 	      /* However, we should remove *software* breakpoints only
 		 if the section is still mapped, or else we overwrite
 		 wrong code with the saved shadow contents.  */
-	      else if (section_is_mapped (b->section))
-		val = target_remove_breakpoint (b->gdbarch,
-						&b->target_info);
+	      else if (section_is_mapped (bl->section))
+		val = target_remove_breakpoint (bl->gdbarch,
+						&bl->target_info);
 	      else
 		val = 0;
 	    }
@@ -2480,61 +2480,61 @@ remove_breakpoint_1 (struct bp_location *b, insertion_state_t is)
       /* In some cases, we might not be able to remove a breakpoint
 	 in a shared library that has already been removed, but we
 	 have not yet processed the shlib unload event.  */
-      if (val && solib_name_from_address (b->pspace, b->address))
+      if (val && solib_name_from_address (bl->pspace, bl->address))
 	val = 0;
 
       if (val)
 	return val;
-      b->inserted = (is == mark_inserted);
+      bl->inserted = (is == mark_inserted);
     }
-  else if (b->loc_type == bp_loc_hardware_watchpoint)
+  else if (bl->loc_type == bp_loc_hardware_watchpoint)
     {
-      b->inserted = (is == mark_inserted);
-      val = target_remove_watchpoint (b->address, b->length,
-				      b->watchpoint_type, b->owner->cond_exp);
+      bl->inserted = (is == mark_inserted);
+      val = target_remove_watchpoint (bl->address, bl->length,
+				      bl->watchpoint_type, bl->owner->cond_exp);
 
       /* Failure to remove any of the hardware watchpoints comes here.  */
-      if ((is == mark_uninserted) && (b->inserted))
+      if ((is == mark_uninserted) && (bl->inserted))
 	warning (_("Could not remove hardware watchpoint %d."),
-		 b->owner->number);
+		 bl->owner->number);
     }
-  else if (b->owner->type == bp_catchpoint
-           && breakpoint_enabled (b->owner)
-           && !b->duplicate)
+  else if (bl->owner->type == bp_catchpoint
+           && breakpoint_enabled (bl->owner)
+           && !bl->duplicate)
     {
-      gdb_assert (b->owner->ops != NULL && b->owner->ops->remove != NULL);
+      gdb_assert (bl->owner->ops != NULL && bl->owner->ops->remove != NULL);
 
-      val = b->owner->ops->remove (b->owner);
+      val = bl->owner->ops->remove (bl->owner);
       if (val)
 	return val;
-      b->inserted = (is == mark_inserted);
+      bl->inserted = (is == mark_inserted);
     }
 
   return 0;
 }
 
 static int
-remove_breakpoint (struct bp_location *b, insertion_state_t is)
+remove_breakpoint (struct bp_location *bl, insertion_state_t is)
 {
   int ret;
   struct cleanup *old_chain;
 
-  /* B is never in moribund_locations by our callers.  */
-  gdb_assert (b->owner != NULL);
+  /* BL is never in moribund_locations by our callers.  */
+  gdb_assert (bl->owner != NULL);
 
-  if (b->owner->enable_state == bp_permanent)
+  if (bl->owner->enable_state == bp_permanent)
     /* Permanent breakpoints cannot be inserted or removed.  */
     return 0;
 
   /* The type of none suggests that owner is actually deleted.
      This should not ever happen.  */
-  gdb_assert (b->owner->type != bp_none);
+  gdb_assert (bl->owner->type != bp_none);
 
   old_chain = save_current_space_and_thread ();
 
-  switch_to_program_space_and_thread (b->pspace);
+  switch_to_program_space_and_thread (bl->pspace);
 
-  ret = remove_breakpoint_1 (b, is);
+  ret = remove_breakpoint_1 (bl, is);
 
   do_cleanups (old_chain);
   return ret;
@@ -2545,11 +2545,11 @@ remove_breakpoint (struct bp_location *b, insertion_state_t is)
 void
 mark_breakpoints_out (void)
 {
-  struct bp_location *bpt, **bptp_tmp;
+  struct bp_location *bl, **blp_tmp;
 
-  ALL_BP_LOCATIONS (bpt, bptp_tmp)
-    if (bpt->pspace == current_program_space)
-      bpt->inserted = 0;
+  ALL_BP_LOCATIONS (bl, blp_tmp)
+    if (bl->pspace == current_program_space)
+      bl->inserted = 0;
 }
 
 /* Clear the "inserted" flag in all breakpoints and delete any
@@ -2567,8 +2567,8 @@ mark_breakpoints_out (void)
 void
 breakpoint_init_inferior (enum inf_context context)
 {
-  struct breakpoint *b, *temp;
-  struct bp_location *bpt, **bptp_tmp;
+  struct breakpoint *b, *b_tmp;
+  struct bp_location *bl, **blp_tmp;
   int ix;
   struct program_space *pspace = current_program_space;
 
@@ -2577,15 +2577,15 @@ breakpoint_init_inferior (enum inf_context context)
   if (gdbarch_has_global_breakpoints (target_gdbarch))
     return;
 
-  ALL_BP_LOCATIONS (bpt, bptp_tmp)
+  ALL_BP_LOCATIONS (bl, blp_tmp)
   {
-    /* ALL_BP_LOCATIONS bp_location has BPT->OWNER always non-NULL.  */
-    if (bpt->pspace == pspace
-	&& bpt->owner->enable_state != bp_permanent)
-      bpt->inserted = 0;
+    /* ALL_BP_LOCATIONS bp_location has BL->OWNER always non-NULL.  */
+    if (bl->pspace == pspace
+	&& bl->owner->enable_state != bp_permanent)
+      bl->inserted = 0;
   }
 
-  ALL_BREAKPOINTS_SAFE (b, temp)
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
   {
     if (b->loc && b->loc->pspace != pspace)
       continue;
@@ -2645,8 +2645,8 @@ breakpoint_init_inferior (enum inf_context context)
   }
 
   /* Get rid of the moribund locations.  */
-  for (ix = 0; VEC_iterate (bp_location_p, moribund_locations, ix, bpt); ++ix)
-    decref_bp_location (&bpt);
+  for (ix = 0; VEC_iterate (bp_location_p, moribund_locations, ix, bl); ++ix)
+    decref_bp_location (&bl);
   VEC_free (bp_location_p, moribund_locations);
 }
 
@@ -2668,26 +2668,26 @@ breakpoint_init_inferior (enum inf_context context)
 enum breakpoint_here
 breakpoint_here_p (struct address_space *aspace, CORE_ADDR pc)
 {
-  struct bp_location *bpt, **bptp_tmp;
+  struct bp_location *bl, **blp_tmp;
   int any_breakpoint_here = 0;
 
-  ALL_BP_LOCATIONS (bpt, bptp_tmp)
+  ALL_BP_LOCATIONS (bl, blp_tmp)
     {
-      if (bpt->loc_type != bp_loc_software_breakpoint
-	  && bpt->loc_type != bp_loc_hardware_breakpoint)
+      if (bl->loc_type != bp_loc_software_breakpoint
+	  && bl->loc_type != bp_loc_hardware_breakpoint)
 	continue;
 
-      /* ALL_BP_LOCATIONS bp_location has BPT->OWNER always non-NULL.  */
-      if ((breakpoint_enabled (bpt->owner)
-	   || bpt->owner->enable_state == bp_permanent)
-	  && breakpoint_address_match (bpt->pspace->aspace, bpt->address,
+      /* ALL_BP_LOCATIONS bp_location has bl->OWNER always non-NULL.  */
+      if ((breakpoint_enabled (bl->owner)
+	   || bl->owner->enable_state == bp_permanent)
+	  && breakpoint_address_match (bl->pspace->aspace, bl->address,
 				       aspace, pc))
 	{
 	  if (overlay_debugging 
-	      && section_is_overlay (bpt->section) 
-	      && !section_is_mapped (bpt->section))
+	      && section_is_overlay (bl->section)
+	      && !section_is_mapped (bl->section))
 	    continue;		/* unmapped overlay -- can't be a match */
-	  else if (bpt->owner->enable_state == bp_permanent)
+	  else if (bl->owner->enable_state == bp_permanent)
 	    return permanent_breakpoint_here;
 	  else
 	    any_breakpoint_here = 1;
@@ -2721,21 +2721,21 @@ moribund_breakpoint_here_p (struct address_space *aspace, CORE_ADDR pc)
 int
 regular_breakpoint_inserted_here_p (struct address_space *aspace, CORE_ADDR pc)
 {
-  struct bp_location *bpt, **bptp_tmp;
+  struct bp_location *bl, **blp_tmp;
 
-  ALL_BP_LOCATIONS (bpt, bptp_tmp)
+  ALL_BP_LOCATIONS (bl, blp_tmp)
     {
-      if (bpt->loc_type != bp_loc_software_breakpoint
-	  && bpt->loc_type != bp_loc_hardware_breakpoint)
+      if (bl->loc_type != bp_loc_software_breakpoint
+	  && bl->loc_type != bp_loc_hardware_breakpoint)
 	continue;
 
-      if (bpt->inserted
-	  && breakpoint_address_match (bpt->pspace->aspace, bpt->address,
+      if (bl->inserted
+	  && breakpoint_address_match (bl->pspace->aspace, bl->address,
 				       aspace, pc))
 	{
 	  if (overlay_debugging 
-	      && section_is_overlay (bpt->section) 
-	      && !section_is_mapped (bpt->section))
+	      && section_is_overlay (bl->section)
+	      && !section_is_mapped (bl->section))
 	    continue;		/* unmapped overlay -- can't be a match */
 	  else
 	    return 1;
@@ -2765,20 +2765,20 @@ breakpoint_inserted_here_p (struct address_space *aspace, CORE_ADDR pc)
 int
 software_breakpoint_inserted_here_p (struct address_space *aspace, CORE_ADDR pc)
 {
-  struct bp_location *bpt, **bptp_tmp;
+  struct bp_location *bl, **blp_tmp;
 
-  ALL_BP_LOCATIONS (bpt, bptp_tmp)
+  ALL_BP_LOCATIONS (bl, blp_tmp)
     {
-      if (bpt->loc_type != bp_loc_software_breakpoint)
+      if (bl->loc_type != bp_loc_software_breakpoint)
 	continue;
 
-      if (bpt->inserted
-	  && breakpoint_address_match (bpt->pspace->aspace, bpt->address,
+      if (bl->inserted
+	  && breakpoint_address_match (bl->pspace->aspace, bl->address,
 				       aspace, pc))
 	{
 	  if (overlay_debugging 
-	      && section_is_overlay (bpt->section) 
-	      && !section_is_mapped (bpt->section))
+	      && section_is_overlay (bl->section)
+	      && !section_is_mapped (bl->section))
 	    continue;		/* unmapped overlay -- can't be a match */
 	  else
 	    return 1;
@@ -2831,51 +2831,51 @@ int
 breakpoint_thread_match (struct address_space *aspace, CORE_ADDR pc,
 			 ptid_t ptid)
 {
-  struct bp_location *bpt, **bptp_tmp;
+  struct bp_location *bl, **blp_tmp;
   /* The thread and task IDs associated to PTID, computed lazily.  */
   int thread = -1;
   int task = 0;
   
-  ALL_BP_LOCATIONS (bpt, bptp_tmp)
+  ALL_BP_LOCATIONS (bl, blp_tmp)
     {
-      if (bpt->loc_type != bp_loc_software_breakpoint
-	  && bpt->loc_type != bp_loc_hardware_breakpoint)
+      if (bl->loc_type != bp_loc_software_breakpoint
+	  && bl->loc_type != bp_loc_hardware_breakpoint)
 	continue;
 
-      /* ALL_BP_LOCATIONS bp_location has BPT->OWNER always non-NULL.  */
-      if (!breakpoint_enabled (bpt->owner)
-	  && bpt->owner->enable_state != bp_permanent)
+      /* ALL_BP_LOCATIONS bp_location has bl->OWNER always non-NULL.  */
+      if (!breakpoint_enabled (bl->owner)
+	  && bl->owner->enable_state != bp_permanent)
 	continue;
 
-      if (!breakpoint_address_match (bpt->pspace->aspace, bpt->address,
+      if (!breakpoint_address_match (bl->pspace->aspace, bl->address,
 				     aspace, pc))
 	continue;
 
-      if (bpt->owner->thread != -1)
+      if (bl->owner->thread != -1)
 	{
 	  /* This is a thread-specific breakpoint.  Check that ptid
 	     matches that thread.  If thread hasn't been computed yet,
 	     it is now time to do so.  */
 	  if (thread == -1)
 	    thread = pid_to_thread_id (ptid);
-	  if (bpt->owner->thread != thread)
+	  if (bl->owner->thread != thread)
 	    continue;
 	}
 
-      if (bpt->owner->task != 0)
+      if (bl->owner->task != 0)
         {
 	  /* This is a task-specific breakpoint.  Check that ptid
 	     matches that task.  If task hasn't been computed yet,
 	     it is now time to do so.  */
 	  if (task == 0)
 	    task = ada_get_task_number (ptid);
-	  if (bpt->owner->task != task)
+	  if (bl->owner->task != task)
 	    continue;
         }
 
       if (overlay_debugging 
-	  && section_is_overlay (bpt->section) 
-	  && !section_is_mapped (bpt->section))
+	  && section_is_overlay (bl->section)
+	  && !section_is_mapped (bl->section))
 	continue;	    /* unmapped overlay -- can't be a match */
 
       return 1;
@@ -3745,7 +3745,7 @@ bpstat_check_location (const struct bp_location *bl,
 				     aspace, bp_addr))
 	return 0;
       if (overlay_debugging		/* unmapped overlay section */
-	  && section_is_overlay (bl->section) 
+	  && section_is_overlay (bl->section)
 	  && !section_is_mapped (bl->section))
 	return 0;
     }
@@ -3766,7 +3766,7 @@ bpstat_check_location (const struct bp_location *bl,
       if (bl->address != bp_addr)
 	return 0;
       if (overlay_debugging		/* unmapped overlay section */
-	  && section_is_overlay (bl->section) 
+	  && section_is_overlay (bl->section)
 	  && !section_is_mapped (bl->section))
 	return 0;
     }
@@ -5667,14 +5667,14 @@ make_breakpoint_permanent (struct breakpoint *b)
 void
 set_longjmp_breakpoint (struct thread_info *tp, struct frame_id frame)
 {
-  struct breakpoint *b, *temp;
+  struct breakpoint *b, *b_tmp;
   int thread = tp->num;
 
   /* To avoid having to rescan all objfile symbols at every step,
      we maintain a list of continually-inserted but always disabled
      longjmp "master" breakpoints.  Here, we simply create momentary
      clones of those and enable them for the requested thread.  */
-  ALL_BREAKPOINTS_SAFE (b, temp)
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
     if (b->pspace == current_program_space
 	&& (b->type == bp_longjmp_master
 	    || b->type == bp_exception_master))
@@ -5692,9 +5692,9 @@ set_longjmp_breakpoint (struct thread_info *tp, struct frame_id frame)
 void
 delete_longjmp_breakpoint (int thread)
 {
-  struct breakpoint *b, *temp;
+  struct breakpoint *b, *b_tmp;
 
-  ALL_BREAKPOINTS_SAFE (b, temp)
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
     if (b->type == bp_longjmp || b->type == bp_exception)
       {
 	if (b->thread == thread)
@@ -5735,9 +5735,9 @@ disable_overlay_breakpoints (void)
 void
 set_std_terminate_breakpoint (void)
 {
-  struct breakpoint *b, *temp;
+  struct breakpoint *b, *b_tmp;
 
-  ALL_BREAKPOINTS_SAFE (b, temp)
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
     if (b->pspace == current_program_space
 	&& b->type == bp_std_terminate_master)
       {
@@ -5750,9 +5750,9 @@ set_std_terminate_breakpoint (void)
 void
 delete_std_terminate_breakpoint (void)
 {
-  struct breakpoint *b, *temp;
+  struct breakpoint *b, *b_tmp;
 
-  ALL_BREAKPOINTS_SAFE (b, temp)
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
     if (b->type == bp_std_terminate)
       delete_breakpoint (b);
 }
@@ -5777,9 +5777,9 @@ create_thread_event_breakpoint (struct gdbarch *gdbarch, CORE_ADDR address)
 void
 remove_thread_event_breakpoints (void)
 {
-  struct breakpoint *b, *temp;
+  struct breakpoint *b, *b_tmp;
 
-  ALL_BREAKPOINTS_SAFE (b, temp)
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
     if (b->type == bp_thread_event
 	&& b->loc->pspace == current_program_space)
       delete_breakpoint (b);
@@ -5814,9 +5814,9 @@ create_jit_event_breakpoint (struct gdbarch *gdbarch, CORE_ADDR address)
 void
 remove_solib_event_breakpoints (void)
 {
-  struct breakpoint *b, *temp;
+  struct breakpoint *b, *b_tmp;
 
-  ALL_BREAKPOINTS_SAFE (b, temp)
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
     if (b->type == bp_shlib_event
 	&& b->loc->pspace == current_program_space)
       delete_breakpoint (b);
@@ -9314,7 +9314,7 @@ clear_command (char *arg, int from_tty)
 void
 breakpoint_auto_delete (bpstat bs)
 {
-  struct breakpoint *b, *temp;
+  struct breakpoint *b, *b_tmp;
 
   for (; bs; bs = bs->next)
     if (bs->breakpoint_at
@@ -9322,7 +9322,7 @@ breakpoint_auto_delete (bpstat bs)
 	&& bs->stop)
       delete_breakpoint (bs->breakpoint_at);
 
-  ALL_BREAKPOINTS_SAFE (b, temp)
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
   {
     if (b->disposition == disp_del_at_next_stop)
       delete_breakpoint (b);
@@ -9873,7 +9873,7 @@ do_delete_breakpoint (struct breakpoint *b, void *ignore)
 void
 delete_command (char *arg, int from_tty)
 {
-  struct breakpoint *b, *temp;
+  struct breakpoint *b, *b_tmp;
 
   dont_repeat ();
 
@@ -9906,7 +9906,7 @@ delete_command (char *arg, int from_tty)
       if (!from_tty
 	  || (breaks_to_delete && query (_("Delete all breakpoints? "))))
 	{
-	  ALL_BREAKPOINTS_SAFE (b, temp)
+	  ALL_BREAKPOINTS_SAFE (b, b_tmp)
 	  {
 	    if (b->type != bp_call_dummy
 		&& b->type != bp_std_terminate
@@ -10415,7 +10415,7 @@ breakpoint_re_set_one (void *bint)
 void
 breakpoint_re_set (void)
 {
-  struct breakpoint *b, *temp;
+  struct breakpoint *b, *b_tmp;
   enum language save_language;
   int save_input_radix;
   struct cleanup *old_chain;
@@ -10424,7 +10424,7 @@ breakpoint_re_set (void)
   save_input_radix = input_radix;
   old_chain = save_current_program_space ();
 
-  ALL_BREAKPOINTS_SAFE (b, temp)
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
   {
     /* Format possible error msg */
     char *message = xstrprintf ("Error in re-setting breakpoint %d: ",
@@ -11315,7 +11315,7 @@ disable_trace_command (char *args, int from_tty)
 static void
 delete_trace_command (char *arg, int from_tty)
 {
-  struct breakpoint *b, *temp;
+  struct breakpoint *b, *b_tmp;
 
   dont_repeat ();
 
@@ -11339,7 +11339,7 @@ delete_trace_command (char *arg, int from_tty)
       if (!from_tty
 	  || (breaks_to_delete && query (_("Delete all tracepoints? "))))
 	{
-	  ALL_BREAKPOINTS_SAFE (b, temp)
+	  ALL_BREAKPOINTS_SAFE (b, b_tmp)
 	  {
 	    if (is_tracepoint (b)
 		&& b->number >= 0)
@@ -11757,9 +11757,9 @@ struct breakpoint *
 iterate_over_breakpoints (int (*callback) (struct breakpoint *, void *),
 			  void *data)
 {
-  struct breakpoint *b, *temp;
+  struct breakpoint *b, *b_tmp;
 
-  ALL_BREAKPOINTS_SAFE (b, temp)
+  ALL_BREAKPOINTS_SAFE (b, b_tmp)
     {
       if ((*callback) (b, data))
 	return b;


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops
  2010-11-19 20:10           ` Thiago Jung Bauermann
@ 2010-12-23 19:06             ` Thiago Jung Bauermann
  2011-01-11 19:31               ` Thiago Jung Bauermann
  0 siblings, 1 reply; 24+ messages in thread
From: Thiago Jung Bauermann @ 2010-12-23 19:06 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: gdb-patches, Jan Kratochvil

Hi,

On Fri, 2010-11-19 at 18:09 -0200, Thiago Jung Bauermann wrote:
> Here's the new version. I understand you approved it already, but it
> seems more useful to me to wait until patch 2/2 to be approved too so
> that I can commit both together (this one doesn't do anything on its
> own).

This is the same patch, refreshed to apply on top of Jan's bp_location
renaming patch. It also incorporates some additional renaming he did
that touched this patch. I understand that this patch is approved. I'll
commit it when the ranged and/or the masked watchpoints patch get in.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


2010-12-23  Thiago Jung Bauermann  <bauerman@br.ibm.com>

	Convert hardware watchpoints to use breakpoint_ops.

gdb/
	* breakpoint.h (breakpoint_ops) <insert>: Rename to...
	<insert_location>: ... this.  Return int instead of void.
	Accept pointer to struct bp_location instead of pointer to
	struct breakpoint.  Adapt all implementations.
	(breakpoint_ops) <remove>: Rename to... 
	<remove_location>: ... this.  Accept pointer to struct bp_location
	instead of pointer to struct breakpoint.  Adapt all implementations.
	* breakpoint.c (insert_catchpoint): Delete function.
	(insert_bp_location): Call the watchpoint or catchpoint's
	breakpoint_ops.insert method.
	(remove_breakpoint_1): Call the watchpoint or catchpoint's
	breakpoint_ops.remove method.
	(insert_watchpoint, remove_watchpoint): New functions.
	(watchpoint_breakpoint_ops): New structure.
	(watch_command_1): Initialize the OPS field.
	* inf-child.c (inf_child_insert_fork_catchpoint)
	(inf_child_remove_fork_catchpoint, inf_child_insert_vfork_catchpoint)
	(inf_child_remove_vfork_catchpoint, inf_child_insert_exec_catchpoint)
	(inf_child_remove_exec_catchpoint, inf_child_set_syscall_catchpoint):
	Delete functions.
	(inf_child_target): Remove initialization of to_insert_fork_catchpoint,
	to_remove_fork_catchpoint, to_insert_vfork_catchpoint,
	to_remove_vfork_catchpoint, to_insert_exec_catchpoint,
	to_remove_exec_catchpoint and to_set_syscall_catchpoint.
	* target.c (update_current_target): Change default implementation of
	to_insert_fork_catchpoint, to_remove_fork_catchpoint,
	to_insert_vfork_catchpoint, to_remove_vfork_catchpoint,
	to_insert_exec_catchpoint, to_remove_exec_catchpoint and
	to_set_syscall_catchpoint to return_one.
	(debug_to_insert_fork_catchpoint, debug_to_insert_vfork_catchpoint)
	(debug_to_insert_exec_catchpoint): Report return value.
	* target.h (to_insert_fork_catchpoint, to_insert_vfork_catchpoint)
	(to_insert_exec_catchpoint): Change declaration to return int instead
	of void.

gdb/testsuite/
	* gdb.base/foll-exec.exp: Adapt to new error string when the catchpoint
	type is not supported.
	* gdb.base/foll-fork.exp: Likewise.
	* gdb.base/foll-vfork.exp: Likewise.


diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 5736fbc..003899c 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1194,18 +1194,6 @@ breakpoint_restore_shadows (gdb_byte *buf, ULONGEST memaddr, LONGEST len)
 }
 \f
 
-/* A wrapper function for inserting catchpoints.  */
-static void
-insert_catchpoint (struct ui_out *uo, void *args)
-{
-  struct breakpoint *b = (struct breakpoint *) args;
-
-  gdb_assert (b->type == bp_catchpoint);
-  gdb_assert (b->ops != NULL && b->ops->insert != NULL);
-
-  b->ops->insert (b);
-}
-
 /* Return true if BPT is of any hardware watchpoint kind.  */
 
 static int
@@ -1742,10 +1730,10 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
 	      watchpoints.  It's not clear that it's necessary... */
 	   && bl->owner->disposition != disp_del_at_next_stop)
     {
-      val = target_insert_watchpoint (bl->address,
-				      bl->length,
-				      bl->watchpoint_type,
-				      bl->owner->cond_exp);
+      gdb_assert (bl->owner->ops != NULL
+		  && bl->owner->ops->insert_location != NULL);
+
+      val = bl->owner->ops->insert_location (bl);
 
       /* If trying to set a read-watchpoint, and it turns out it's not
 	 supported, try emulating one with an access watchpoint.  */
@@ -1771,12 +1759,12 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
 
 	  if (val == 1)
 	    {
-	      val = target_insert_watchpoint (bl->address,
-					      bl->length,
-					      hw_access,
-					      bl->owner->cond_exp);
-	      if (val == 0)
-		bl->watchpoint_type = hw_access;
+	      bl->watchpoint_type = hw_access;
+	      val = bl->owner->ops->insert_location (bl);
+
+	      if (val)
+		/* Back to the original value.  */
+		bl->watchpoint_type = hw_read;
 	    }
 	}
 
@@ -1785,14 +1773,23 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
 
   else if (bl->owner->type == bp_catchpoint)
     {
-      struct gdb_exception e = catch_exception (uiout, insert_catchpoint,
-						bl->owner, RETURN_MASK_ERROR);
-      exception_fprintf (gdb_stderr, e, "warning: inserting catchpoint %d: ",
-			 bl->owner->number);
-      if (e.reason < 0)
-	bl->owner->enable_state = bp_disabled;
-      else
-	bl->inserted = 1;
+      gdb_assert (bl->owner->ops != NULL
+		  && bl->owner->ops->insert_location != NULL);
+
+      val = bl->owner->ops->insert_location (bl);
+      if (val)
+	{
+	  bl->owner->enable_state = bp_disabled;
+
+	  if (val == 1)
+	    warning (_("\
+Error inserting catchpoint %d: Your system does not support this type\n\
+of catchpoint."), bl->owner->number);
+	  else
+	    warning (_("Error inserting catchpoint %d."), bl->owner->number);
+	}
+
+      bl->inserted = (val == 0);
 
       /* We've already printed an error message if there was a problem
 	 inserting this catchpoint, and we've disabled the catchpoint,
@@ -2489,9 +2486,11 @@ remove_breakpoint_1 (struct bp_location *bl, insertion_state_t is)
     }
   else if (bl->loc_type == bp_loc_hardware_watchpoint)
     {
+      gdb_assert (bl->owner->ops != NULL
+		  && bl->owner->ops->remove_location != NULL);
+
       bl->inserted = (is == mark_inserted);
-      val = target_remove_watchpoint (bl->address, bl->length,
-				      bl->watchpoint_type, bl->owner->cond_exp);
+      bl->owner->ops->remove_location (bl);
 
       /* Failure to remove any of the hardware watchpoints comes here.  */
       if ((is == mark_uninserted) && (bl->inserted))
@@ -2502,11 +2501,13 @@ remove_breakpoint_1 (struct bp_location *bl, insertion_state_t is)
            && breakpoint_enabled (bl->owner)
            && !bl->duplicate)
     {
-      gdb_assert (bl->owner->ops != NULL && bl->owner->ops->remove != NULL);
+      gdb_assert (bl->owner->ops != NULL
+		  && bl->owner->ops->remove_location != NULL);
 
-      val = bl->owner->ops->remove (bl->owner);
+      val = bl->owner->ops->remove_location (bl);
       if (val)
 	return val;
+
       bl->inserted = (is == mark_inserted);
     }
 
@@ -5919,16 +5920,16 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
 
 /* Implement the "insert" breakpoint_ops method for fork catchpoints.  */
 
-static void
-insert_catch_fork (struct breakpoint *b)
+static int
+insert_catch_fork (struct bp_location *bl)
 {
-  target_insert_fork_catchpoint (PIDGET (inferior_ptid));
+  return target_insert_fork_catchpoint (PIDGET (inferior_ptid));
 }
 
 /* Implement the "remove" breakpoint_ops method for fork catchpoints.  */
 
 static int
-remove_catch_fork (struct breakpoint *b)
+remove_catch_fork (struct bp_location *bl)
 {
   return target_remove_fork_catchpoint (PIDGET (inferior_ptid));
 }
@@ -6011,16 +6012,16 @@ static struct breakpoint_ops catch_fork_breakpoint_ops =
 
 /* Implement the "insert" breakpoint_ops method for vfork catchpoints.  */
 
-static void
-insert_catch_vfork (struct breakpoint *b)
+static int
+insert_catch_vfork (struct bp_location *bl)
 {
-  target_insert_vfork_catchpoint (PIDGET (inferior_ptid));
+  return target_insert_vfork_catchpoint (PIDGET (inferior_ptid));
 }
 
 /* Implement the "remove" breakpoint_ops method for vfork catchpoints.  */
 
 static int
-remove_catch_vfork (struct breakpoint *b)
+remove_catch_vfork (struct bp_location *bl)
 {
   return target_remove_vfork_catchpoint (PIDGET (inferior_ptid));
 }
@@ -6103,20 +6104,20 @@ static struct breakpoint_ops catch_vfork_breakpoint_ops =
 /* Implement the "insert" breakpoint_ops method for syscall
    catchpoints.  */
 
-static void
-insert_catch_syscall (struct breakpoint *b)
+static int
+insert_catch_syscall (struct bp_location *bl)
 {
   struct inferior *inf = current_inferior ();
 
   ++inf->total_syscalls_count;
-  if (!b->syscalls_to_be_caught)
+  if (!bl->owner->syscalls_to_be_caught)
     ++inf->any_syscall_count;
   else
     {
       int i, iter;
 
       for (i = 0;
-           VEC_iterate (int, b->syscalls_to_be_caught, i, iter);
+           VEC_iterate (int, bl->owner->syscalls_to_be_caught, i, iter);
            i++)
 	{
           int elem;
@@ -6137,30 +6138,30 @@ insert_catch_syscall (struct breakpoint *b)
 	}
     }
 
-  target_set_syscall_catchpoint (PIDGET (inferior_ptid),
-				 inf->total_syscalls_count != 0,
-				 inf->any_syscall_count,
-				 VEC_length (int, inf->syscalls_counts),
-				 VEC_address (int, inf->syscalls_counts));
+  return target_set_syscall_catchpoint (PIDGET (inferior_ptid),
+					inf->total_syscalls_count != 0,
+					inf->any_syscall_count,
+					VEC_length (int, inf->syscalls_counts),
+					VEC_address (int, inf->syscalls_counts));
 }
 
 /* Implement the "remove" breakpoint_ops method for syscall
    catchpoints.  */
 
 static int
-remove_catch_syscall (struct breakpoint *b)
+remove_catch_syscall (struct bp_location *bl)
 {
   struct inferior *inf = current_inferior ();
 
   --inf->total_syscalls_count;
-  if (!b->syscalls_to_be_caught)
+  if (!bl->owner->syscalls_to_be_caught)
     --inf->any_syscall_count;
   else
     {
       int i, iter;
 
       for (i = 0;
-           VEC_iterate (int, b->syscalls_to_be_caught, i, iter);
+           VEC_iterate (int, bl->owner->syscalls_to_be_caught, i, iter);
            i++)
 	{
           int elem;
@@ -6459,14 +6460,14 @@ create_fork_vfork_event_catchpoint (struct gdbarch *gdbarch,
 
 /* Exec catchpoints.  */
 
-static void
-insert_catch_exec (struct breakpoint *b)
+static int
+insert_catch_exec (struct bp_location *bl)
 {
-  target_insert_exec_catchpoint (PIDGET (inferior_ptid));
+  return target_insert_exec_catchpoint (PIDGET (inferior_ptid));
 }
 
 static int
-remove_catch_exec (struct breakpoint *b)
+remove_catch_exec (struct bp_location *bl)
 {
   return target_remove_exec_catchpoint (PIDGET (inferior_ptid));
 }
@@ -8113,6 +8114,37 @@ watchpoint_exp_is_const (const struct expression *exp)
   return 1;
 }
 
+/* Implement the "insert" breakpoint_ops method for hardware watchpoints.  */
+
+static int
+insert_watchpoint (struct bp_location *bl)
+{
+  return target_insert_watchpoint (bl->address, bl->length,
+				   bl->watchpoint_type, bl->owner->cond_exp);
+}
+
+/* Implement the "remove" breakpoint_ops method for hardware watchpoints.  */
+
+static int
+remove_watchpoint (struct bp_location *bl)
+{
+  return target_remove_watchpoint (bl->address, bl->length,
+				   bl->watchpoint_type, bl->owner->cond_exp);
+}
+
+/* The breakpoint_ops structure to be used in hardware watchpoints.  */
+
+static struct breakpoint_ops watchpoint_breakpoint_ops =
+{
+  insert_watchpoint,
+  remove_watchpoint,
+  NULL, /* breakpoint_hit */
+  NULL, /* print_it */
+  NULL, /* print_one */
+  NULL, /* print_mention */
+  NULL  /* print_recreate */
+};
+
 /* accessflag:  hw_write:  watch write, 
                 hw_read:   watch read, 
 		hw_access: watch access (read or write) */
@@ -8355,6 +8387,8 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
     b->exp_string = savestring (exp_start, exp_end - exp_start);
   b->val = val;
   b->val_valid = 1;
+  b->ops = &watchpoint_breakpoint_ops;
+
   if (cond_start)
     b->cond_string = savestring (cond_start, cond_end - cond_start);
   else
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index a044c6b..12c2df2 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -355,16 +355,18 @@ struct bp_location
    will be called instead of the performing the default action for this
    bptype.  */
 
-struct breakpoint_ops 
+struct breakpoint_ops
 {
-  /* Insert the breakpoint or activate the catchpoint.  Should raise
-     an exception if the operation failed.  */
-  void (*insert) (struct breakpoint *);
+  /* Insert the breakpoint or watchpoint or activate the catchpoint.
+     Return 0 for success, 1 if the breakpoint, watchpoint or catchpoint
+     type is not supported, -1 for failure.  */
+  int (*insert_location) (struct bp_location *);
 
   /* Remove the breakpoint/catchpoint that was previously inserted
-     with the "insert" method above.  Return non-zero if the operation
-     succeeded.  */
-  int (*remove) (struct breakpoint *);
+     with the "insert" method above.  Return 0 for success, 1 if the
+     breakpoint, watchpoint or catchpoint type is not supported,
+     -1 for failure.  */
+  int (*remove_location) (struct bp_location *);
 
   /* Return non-zero if the debugger should tell the user that this
      breakpoint was hit.  */
diff --git a/gdb/inf-child.c b/gdb/inf-child.c
index 72a18e4..810f688 100644
--- a/gdb/inf-child.c
+++ b/gdb/inf-child.c
@@ -94,36 +94,6 @@ inf_child_acknowledge_created_inferior (int pid)
      created inferior" operation by a debugger.  */
 }
 
-static void
-inf_child_insert_fork_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of fork
-     events.  */
-}
-
-static int
-inf_child_remove_fork_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of fork
-     events.  */
-  return 0;
-}
-
-static void
-inf_child_insert_vfork_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of vfork
-     events.  */
-}
-
-static int
-inf_child_remove_vfork_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of vfork
-     events.  */
-  return 0;
-}
-
 static int
 inf_child_follow_fork (struct target_ops *ops, int follow_child)
 {
@@ -132,30 +102,6 @@ inf_child_follow_fork (struct target_ops *ops, int follow_child)
   return 0;
 }
 
-static void
-inf_child_insert_exec_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of exec
-     events.  */
-}
-
-static int
-inf_child_remove_exec_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of exec
-     events.  */
-  return 0;
-}
-
-static int
-inf_child_set_syscall_catchpoint (int pid, int needed, int any_count,
-				  int table_size, int *table)
-{
-  /* This version of Unix doesn't support notification of syscall
-     events.  */
-  return 0;
-}
-
 static int
 inf_child_can_run (void)
 {
@@ -193,14 +139,7 @@ inf_child_target (void)
   t->to_terminal_info = child_terminal_info;
   t->to_post_startup_inferior = inf_child_post_startup_inferior;
   t->to_acknowledge_created_inferior = inf_child_acknowledge_created_inferior;
-  t->to_insert_fork_catchpoint = inf_child_insert_fork_catchpoint;
-  t->to_remove_fork_catchpoint = inf_child_remove_fork_catchpoint;
-  t->to_insert_vfork_catchpoint = inf_child_insert_vfork_catchpoint;
-  t->to_remove_vfork_catchpoint = inf_child_remove_vfork_catchpoint;
   t->to_follow_fork = inf_child_follow_fork;
-  t->to_insert_exec_catchpoint = inf_child_insert_exec_catchpoint;
-  t->to_remove_exec_catchpoint = inf_child_remove_exec_catchpoint;
-  t->to_set_syscall_catchpoint = inf_child_set_syscall_catchpoint;
   t->to_can_run = inf_child_can_run;
   t->to_pid_to_exec_file = inf_child_pid_to_exec_file;
   t->to_stratum = process_stratum;
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 56490cc..41dfd24 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -961,36 +961,34 @@ Attaching after process %d fork to child process %d.\n"),
 }
 
 \f
-static void
+static int
 linux_child_insert_fork_catchpoint (int pid)
 {
-  if (! linux_supports_tracefork (pid))
-    error (_("Your system does not support fork catchpoints."));
+  return !linux_supports_tracefork (pid);
 }
 
-static void
+static int
 linux_child_insert_vfork_catchpoint (int pid)
 {
-  if (!linux_supports_tracefork (pid))
-    error (_("Your system does not support vfork catchpoints."));
+  return !linux_supports_tracefork (pid);
 }
 
-static void
+static int
 linux_child_insert_exec_catchpoint (int pid)
 {
-  if (!linux_supports_tracefork (pid))
-    error (_("Your system does not support exec catchpoints."));
+  return !linux_supports_tracefork (pid);
 }
 
 static int
 linux_child_set_syscall_catchpoint (int pid, int needed, int any_count,
 				    int table_size, int *table)
 {
-  if (! linux_supports_tracesysgood (pid))
-    error (_("Your system does not support syscall catchpoints."));
+  if (!linux_supports_tracesysgood (pid))
+    return 1;
+
   /* On GNU/Linux, we ignore the arguments.  It means that we only
      enable the syscall catchpoints, but do not disable them.
-     
+
      Also, we do not use the `table' information because we do not
      filter system calls here.  We let GDB do the logic for us.  */
   return 0;
diff --git a/gdb/target.c b/gdb/target.c
index 5984125..dd976c9 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -782,26 +782,26 @@ update_current_target (void)
 	    (void (*) (int))
 	    target_ignore);
   de_fault (to_insert_fork_catchpoint,
-	    (void (*) (int))
-	    tcomplain);
+	    (int (*) (int))
+	    return_one);
   de_fault (to_remove_fork_catchpoint,
 	    (int (*) (int))
-	    tcomplain);
+	    return_one);
   de_fault (to_insert_vfork_catchpoint,
-	    (void (*) (int))
-	    tcomplain);
+	    (int (*) (int))
+	    return_one);
   de_fault (to_remove_vfork_catchpoint,
 	    (int (*) (int))
-	    tcomplain);
+	    return_one);
   de_fault (to_insert_exec_catchpoint,
-	    (void (*) (int))
-	    tcomplain);
+	    (int (*) (int))
+	    return_one);
   de_fault (to_remove_exec_catchpoint,
 	    (int (*) (int))
-	    tcomplain);
+	    return_one);
   de_fault (to_set_syscall_catchpoint,
 	    (int (*) (int, int, int, int, int *))
-	    tcomplain);
+	    return_one);
   de_fault (to_has_exited,
 	    (int (*) (int, int, int *))
 	    return_zero);
@@ -3657,13 +3657,17 @@ debug_to_acknowledge_created_inferior (int pid)
 		      pid);
 }
 
-static void
+static int
 debug_to_insert_fork_catchpoint (int pid)
 {
-  debug_target.to_insert_fork_catchpoint (pid);
+  int retval;
 
-  fprintf_unfiltered (gdb_stdlog, "target_insert_fork_catchpoint (%d)\n",
-		      pid);
+  retval = debug_target.to_insert_fork_catchpoint (pid);
+
+  fprintf_unfiltered (gdb_stdlog, "target_insert_fork_catchpoint (%d) = %d\n",
+		      pid, retval);
+
+  return retval;
 }
 
 static int
@@ -3679,13 +3683,17 @@ debug_to_remove_fork_catchpoint (int pid)
   return retval;
 }
 
-static void
+static int
 debug_to_insert_vfork_catchpoint (int pid)
 {
-  debug_target.to_insert_vfork_catchpoint (pid);
+  int retval;
 
-  fprintf_unfiltered (gdb_stdlog, "target_insert_vfork_catchpoint (%d)\n",
-		      pid);
+  retval = debug_target.to_insert_vfork_catchpoint (pid);
+
+  fprintf_unfiltered (gdb_stdlog, "target_insert_vfork_catchpoint (%d) = %d\n",
+		      pid, retval);
+
+  return retval;
 }
 
 static int
@@ -3701,13 +3709,17 @@ debug_to_remove_vfork_catchpoint (int pid)
   return retval;
 }
 
-static void
+static int
 debug_to_insert_exec_catchpoint (int pid)
 {
-  debug_target.to_insert_exec_catchpoint (pid);
+  int retval;
 
-  fprintf_unfiltered (gdb_stdlog, "target_insert_exec_catchpoint (%d)\n",
-		      pid);
+  retval = debug_target.to_insert_exec_catchpoint (pid);
+
+  fprintf_unfiltered (gdb_stdlog, "target_insert_exec_catchpoint (%d) = %d\n",
+		      pid, retval);
+
+  return retval;
 }
 
 static int
diff --git a/gdb/target.h b/gdb/target.h
index 7290d90..bc70107 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -468,12 +468,12 @@ struct target_ops
 				char *, char *, char **, int);
     void (*to_post_startup_inferior) (ptid_t);
     void (*to_acknowledge_created_inferior) (int);
-    void (*to_insert_fork_catchpoint) (int);
+    int (*to_insert_fork_catchpoint) (int);
     int (*to_remove_fork_catchpoint) (int);
-    void (*to_insert_vfork_catchpoint) (int);
+    int (*to_insert_vfork_catchpoint) (int);
     int (*to_remove_vfork_catchpoint) (int);
     int (*to_follow_fork) (struct target_ops *, int);
-    void (*to_insert_exec_catchpoint) (int);
+    int (*to_insert_exec_catchpoint) (int);
     int (*to_remove_exec_catchpoint) (int);
     int (*to_set_syscall_catchpoint) (int, int, int, int, int *);
     int (*to_has_exited) (int, int, int *);
@@ -1034,7 +1034,8 @@ void target_create_inferior (char *exec_file, char *args,
 
 /* On some targets, we can catch an inferior fork or vfork event when
    it occurs.  These functions insert/remove an already-created
-   catchpoint for such events.  */
+   catchpoint for such events.  They return  0 for success, 1 if the
+   catchpoint type is not supported and -1 for failure.  */
 
 #define target_insert_fork_catchpoint(pid) \
      (*current_target.to_insert_fork_catchpoint) (pid)
@@ -1060,7 +1061,8 @@ int target_follow_fork (int follow_child);
 
 /* On some targets, we can catch an inferior exec event when it
    occurs.  These functions insert/remove an already-created
-   catchpoint for such events.  */
+   catchpoint for such events.  They return  0 for success, 1 if the
+   catchpoint type is not supported and -1 for failure.  */
 
 #define target_insert_exec_catchpoint(pid) \
      (*current_target.to_insert_exec_catchpoint) (pid)
@@ -1083,7 +1085,10 @@ int target_follow_fork (int follow_child);
 
    TABLE is an array of ints, indexed by syscall number.  An element in
    this array is nonzero if that syscall should be caught.  This argument
-   only matters if ANY_COUNT is zero.  */
+   only matters if ANY_COUNT is zero.
+
+   Return 0 for success, 1 if syscall catchpoints are not supported or -1
+   for failure.  */
 
 #define target_set_syscall_catchpoint(pid, needed, any_count, table_size, table) \
      (*current_target.to_set_syscall_catchpoint) (pid, needed, any_count, \
diff --git a/gdb/testsuite/gdb.base/foll-exec.exp b/gdb/testsuite/gdb.base/foll-exec.exp
index 92fd105..e30334d 100644
--- a/gdb/testsuite/gdb.base/foll-exec.exp
+++ b/gdb/testsuite/gdb.base/foll-exec.exp
@@ -89,7 +89,7 @@ proc do_exec_tests {} {
    gdb_test "catch exec" "Catchpoint \[0-9\]* \\(exec\\)" "insert first exec catchpoint"
    set has_exec_catchpoints 0
    gdb_test_multiple "continue" "continue to first exec catchpoint" {
-     -re ".*Your system does not support exec catchpoints.*$gdb_prompt $" {
+     -re ".*Your system does not support this type\r\nof catchpoint.*$gdb_prompt $" {
        unsupported "continue to first exec catchpoint"
      }
      -re ".*Catchpoint.*$gdb_prompt $" {
diff --git a/gdb/testsuite/gdb.base/foll-fork.exp b/gdb/testsuite/gdb.base/foll-fork.exp
index 891aa38..6dcfca1 100644
--- a/gdb/testsuite/gdb.base/foll-fork.exp
+++ b/gdb/testsuite/gdb.base/foll-fork.exp
@@ -45,7 +45,7 @@ proc check_fork_catchpoints {} {
   gdb_test "catch fork" "Catchpoint \[0-9\]* \\(fork\\)" "insert first fork catchpoint"
   set has_fork_catchpoints 0
   gdb_test_multiple "continue" "continue to first fork catchpoint" {
-    -re ".*Your system does not support fork catchpoints.*$gdb_prompt $" {
+    -re ".*Your system does not support this type\r\nof catchpoint.*$gdb_prompt $" {
       unsupported "continue to first fork catchpoint"
     }
     -re ".*Catchpoint.*$gdb_prompt $" {
diff --git a/gdb/testsuite/gdb.base/foll-vfork.exp b/gdb/testsuite/gdb.base/foll-vfork.exp
index e55d700..1b479f2 100644
--- a/gdb/testsuite/gdb.base/foll-vfork.exp
+++ b/gdb/testsuite/gdb.base/foll-vfork.exp
@@ -74,7 +74,7 @@ proc check_vfork_catchpoints {} {
   gdb_test "catch vfork" "Catchpoint \[0-9\]* \\(vfork\\)" "insert first vfork catchpoint"
   set has_vfork_catchpoints 0
   gdb_test_multiple "continue" "continue to first vfork catchpoint" {
-    -re ".*Your system does not support vfork catchpoints.*$gdb_prompt $" {
+    -re ".*Your system does not support this type\r\nof catchpoint.*$gdb_prompt $" {
       unsupported "continue to first vfork catchpoint"
     }
     -re ".*Catchpoint.*$gdb_prompt $" {


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [patch] renaming: bp_location: b->bl &co.  [Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops]
  2010-12-23 18:50             ` Thiago Jung Bauermann
@ 2010-12-24  5:14               ` Joel Brobecker
  2010-12-27 20:04                 ` Thiago Jung Bauermann
  0 siblings, 1 reply; 24+ messages in thread
From: Joel Brobecker @ 2010-12-24  5:14 UTC (permalink / raw)
  To: Thiago Jung Bauermann; +Cc: Jan Kratochvil, Pedro Alves, gdb-patches

> 2010-12-23  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	Code cleanup - renaming.
> 	* breakpoint.c: Use bl for `*bp_location' variables, blp_tmp for
> 	`**bp_location' helper variables, b_tmp for `*breakpoint' helper
> 	variables.
> 
> Ok to check this one in? Do you think a more elaborate ChangeLog entry
> is needed?

I think that's fine. These are just renames, so as long as the renames
have been agreed on, it's OK to treat them as obvious changes.

-- 
Joel

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [patch] renaming: bp_location: b->bl &co.  [Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops]
  2010-12-24  5:14               ` Joel Brobecker
@ 2010-12-27 20:04                 ` Thiago Jung Bauermann
  0 siblings, 0 replies; 24+ messages in thread
From: Thiago Jung Bauermann @ 2010-12-27 20:04 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: Jan Kratochvil, Pedro Alves, gdb-patches

Hi Joel,

On Fri, 2010-12-24 at 06:55 +0400, Joel Brobecker wrote:
> > 2010-12-23  Jan Kratochvil  <jan.kratochvil@redhat.com>
> > 
> > 	Code cleanup - renaming.
> > 	* breakpoint.c: Use bl for `*bp_location' variables, blp_tmp for
> > 	`**bp_location' helper variables, b_tmp for `*breakpoint' helper
> > 	variables.
> > 
> > Ok to check this one in? Do you think a more elaborate ChangeLog entry
> > is needed?
> 
> I think that's fine. These are just renames, so as long as the renames
> have been agreed on, it's OK to treat them as obvious changes.

You're right. I just committed the patch. Thanks!
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops
  2010-12-23 19:06             ` Thiago Jung Bauermann
@ 2011-01-11 19:31               ` Thiago Jung Bauermann
  2011-04-29 15:51                 ` "Cannot remove breakpoints because program is no longer writable" & catchpoints (was: Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops) Pedro Alves
  0 siblings, 1 reply; 24+ messages in thread
From: Thiago Jung Bauermann @ 2011-01-11 19:31 UTC (permalink / raw)
  To: Joel Brobecker; +Cc: gdb-patches, Jan Kratochvil

Hi,

I just committed this refreshed version.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


2010-01-11  Thiago Jung Bauermann  <bauerman@br.ibm.com>

	Convert hardware watchpoints to use breakpoint_ops.

gdb/
	* breakpoint.h (breakpoint_ops) <insert>: Rename to...
	<insert_location>: ... this.  Return int instead of void.
	Accept pointer to struct bp_location instead of pointer to
	struct breakpoint.  Adapt all implementations.
	(breakpoint_ops) <remove>: Rename to... 
	<remove_location>: ... this.  Accept pointer to struct bp_location
	instead of pointer to struct breakpoint.  Adapt all implementations.
	* breakpoint.c (insert_catchpoint): Delete function.
	(insert_bp_location): Call the watchpoint or catchpoint's
	breakpoint_ops.insert method.
	(remove_breakpoint_1): Call the watchpoint or catchpoint's
	breakpoint_ops.remove method.
	(insert_watchpoint, remove_watchpoint): New functions.
	(watchpoint_breakpoint_ops): New structure.
	(watch_command_1): Initialize the OPS field.
	* inf-child.c (inf_child_insert_fork_catchpoint)
	(inf_child_remove_fork_catchpoint, inf_child_insert_vfork_catchpoint)
	(inf_child_remove_vfork_catchpoint, inf_child_insert_exec_catchpoint)
	(inf_child_remove_exec_catchpoint, inf_child_set_syscall_catchpoint):
	Delete functions.
	(inf_child_target): Remove initialization of to_insert_fork_catchpoint,
	to_remove_fork_catchpoint, to_insert_vfork_catchpoint,
	to_remove_vfork_catchpoint, to_insert_exec_catchpoint,
	to_remove_exec_catchpoint and to_set_syscall_catchpoint.
	* target.c (update_current_target): Change default implementation of
	to_insert_fork_catchpoint, to_remove_fork_catchpoint,
	to_insert_vfork_catchpoint, to_remove_vfork_catchpoint,
	to_insert_exec_catchpoint, to_remove_exec_catchpoint and
	to_set_syscall_catchpoint to return_one.
	(debug_to_insert_fork_catchpoint, debug_to_insert_vfork_catchpoint)
	(debug_to_insert_exec_catchpoint): Report return value.
	* target.h (to_insert_fork_catchpoint, to_insert_vfork_catchpoint)
	(to_insert_exec_catchpoint): Change declaration to return int instead
	of void.

gdb/testsuite/
	* gdb.base/foll-exec.exp: Adapt to new error string when the catchpoint
	type is not supported.
	* gdb.base/foll-fork.exp: Likewise.
	* gdb.base/foll-vfork.exp: Likewise.


diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index a909aa3..fc66e9b 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1228,18 +1228,6 @@ breakpoint_restore_shadows (gdb_byte *buf, ULONGEST memaddr, LONGEST len)
 }
 \f
 
-/* A wrapper function for inserting catchpoints.  */
-static void
-insert_catchpoint (struct ui_out *uo, void *args)
-{
-  struct breakpoint *b = (struct breakpoint *) args;
-
-  gdb_assert (b->type == bp_catchpoint);
-  gdb_assert (b->ops != NULL && b->ops->insert != NULL);
-
-  b->ops->insert (b);
-}
-
 /* Return true if BPT is of any hardware watchpoint kind.  */
 
 static int
@@ -1790,10 +1778,10 @@ insert_bp_location (struct bp_location *bl,
 	      watchpoints.  It's not clear that it's necessary...  */
 	   && bl->owner->disposition != disp_del_at_next_stop)
     {
-      val = target_insert_watchpoint (bl->address,
-				      bl->length,
-				      bl->watchpoint_type,
-				      bl->owner->cond_exp);
+      gdb_assert (bl->owner->ops != NULL
+		  && bl->owner->ops->insert_location != NULL);
+
+      val = bl->owner->ops->insert_location (bl);
 
       /* If trying to set a read-watchpoint, and it turns out it's not
 	 supported, try emulating one with an access watchpoint.  */
@@ -1819,12 +1807,12 @@ insert_bp_location (struct bp_location *bl,
 
 	  if (val == 1)
 	    {
-	      val = target_insert_watchpoint (bl->address,
-					      bl->length,
-					      hw_access,
-					      bl->owner->cond_exp);
-	      if (val == 0)
-		bl->watchpoint_type = hw_access;
+	      bl->watchpoint_type = hw_access;
+	      val = bl->owner->ops->insert_location (bl);
+
+	      if (val)
+		/* Back to the original value.  */
+		bl->watchpoint_type = hw_read;
 	    }
 	}
 
@@ -1833,14 +1821,23 @@ insert_bp_location (struct bp_location *bl,
 
   else if (bl->owner->type == bp_catchpoint)
     {
-      struct gdb_exception e = catch_exception (uiout, insert_catchpoint,
-						bl->owner, RETURN_MASK_ERROR);
-      exception_fprintf (gdb_stderr, e, "warning: inserting catchpoint %d: ",
-			 bl->owner->number);
-      if (e.reason < 0)
-	bl->owner->enable_state = bp_disabled;
-      else
-	bl->inserted = 1;
+      gdb_assert (bl->owner->ops != NULL
+		  && bl->owner->ops->insert_location != NULL);
+
+      val = bl->owner->ops->insert_location (bl);
+      if (val)
+	{
+	  bl->owner->enable_state = bp_disabled;
+
+	  if (val == 1)
+	    warning (_("\
+Error inserting catchpoint %d: Your system does not support this type\n\
+of catchpoint."), bl->owner->number);
+	  else
+	    warning (_("Error inserting catchpoint %d."), bl->owner->number);
+	}
+
+      bl->inserted = (val == 0);
 
       /* We've already printed an error message if there was a problem
 	 inserting this catchpoint, and we've disabled the catchpoint,
@@ -2537,10 +2534,11 @@ remove_breakpoint_1 (struct bp_location *bl, insertion_state_t is)
     }
   else if (bl->loc_type == bp_loc_hardware_watchpoint)
     {
+      gdb_assert (bl->owner->ops != NULL
+		  && bl->owner->ops->remove_location != NULL);
+
       bl->inserted = (is == mark_inserted);
-      val = target_remove_watchpoint (bl->address, bl->length,
-				      bl->watchpoint_type, 
-				      bl->owner->cond_exp);
+      bl->owner->ops->remove_location (bl);
 
       /* Failure to remove any of the hardware watchpoints comes here.  */
       if ((is == mark_uninserted) && (bl->inserted))
@@ -2551,11 +2549,13 @@ remove_breakpoint_1 (struct bp_location *bl, insertion_state_t is)
            && breakpoint_enabled (bl->owner)
            && !bl->duplicate)
     {
-      gdb_assert (bl->owner->ops != NULL && bl->owner->ops->remove != NULL);
+      gdb_assert (bl->owner->ops != NULL
+		  && bl->owner->ops->remove_location != NULL);
 
-      val = bl->owner->ops->remove (bl->owner);
+      val = bl->owner->ops->remove_location (bl);
       if (val)
 	return val;
+
       bl->inserted = (is == mark_inserted);
     }
 
@@ -5997,17 +5997,17 @@ disable_breakpoints_in_unloaded_shlib (struct so_list *solib)
 /* Implement the "insert" breakpoint_ops method for fork
    catchpoints.  */
 
-static void
-insert_catch_fork (struct breakpoint *b)
+static int
+insert_catch_fork (struct bp_location *bl)
 {
-  target_insert_fork_catchpoint (PIDGET (inferior_ptid));
+  return target_insert_fork_catchpoint (PIDGET (inferior_ptid));
 }
 
 /* Implement the "remove" breakpoint_ops method for fork
    catchpoints.  */
 
 static int
-remove_catch_fork (struct breakpoint *b)
+remove_catch_fork (struct bp_location *bl)
 {
   return target_remove_fork_catchpoint (PIDGET (inferior_ptid));
 }
@@ -6093,17 +6093,17 @@ static struct breakpoint_ops catch_fork_breakpoint_ops =
 /* Implement the "insert" breakpoint_ops method for vfork
    catchpoints.  */
 
-static void
-insert_catch_vfork (struct breakpoint *b)
+static int
+insert_catch_vfork (struct bp_location *bl)
 {
-  target_insert_vfork_catchpoint (PIDGET (inferior_ptid));
+  return target_insert_vfork_catchpoint (PIDGET (inferior_ptid));
 }
 
 /* Implement the "remove" breakpoint_ops method for vfork
    catchpoints.  */
 
 static int
-remove_catch_vfork (struct breakpoint *b)
+remove_catch_vfork (struct bp_location *bl)
 {
   return target_remove_vfork_catchpoint (PIDGET (inferior_ptid));
 }
@@ -6188,20 +6188,20 @@ static struct breakpoint_ops catch_vfork_breakpoint_ops =
 /* Implement the "insert" breakpoint_ops method for syscall
    catchpoints.  */
 
-static void
-insert_catch_syscall (struct breakpoint *b)
+static int
+insert_catch_syscall (struct bp_location *bl)
 {
   struct inferior *inf = current_inferior ();
 
   ++inf->total_syscalls_count;
-  if (!b->syscalls_to_be_caught)
+  if (!bl->owner->syscalls_to_be_caught)
     ++inf->any_syscall_count;
   else
     {
       int i, iter;
 
       for (i = 0;
-           VEC_iterate (int, b->syscalls_to_be_caught, i, iter);
+           VEC_iterate (int, bl->owner->syscalls_to_be_caught, i, iter);
            i++)
 	{
           int elem;
@@ -6223,30 +6223,30 @@ insert_catch_syscall (struct breakpoint *b)
 	}
     }
 
-  target_set_syscall_catchpoint (PIDGET (inferior_ptid),
-				 inf->total_syscalls_count != 0,
-				 inf->any_syscall_count,
-				 VEC_length (int, inf->syscalls_counts),
-				 VEC_address (int, inf->syscalls_counts));
+  return target_set_syscall_catchpoint (PIDGET (inferior_ptid),
+					inf->total_syscalls_count != 0,
+					inf->any_syscall_count,
+					VEC_length (int, inf->syscalls_counts),
+					VEC_address (int, inf->syscalls_counts));
 }
 
 /* Implement the "remove" breakpoint_ops method for syscall
    catchpoints.  */
 
 static int
-remove_catch_syscall (struct breakpoint *b)
+remove_catch_syscall (struct bp_location *bl)
 {
   struct inferior *inf = current_inferior ();
 
   --inf->total_syscalls_count;
-  if (!b->syscalls_to_be_caught)
+  if (!bl->owner->syscalls_to_be_caught)
     --inf->any_syscall_count;
   else
     {
       int i, iter;
 
       for (i = 0;
-           VEC_iterate (int, b->syscalls_to_be_caught, i, iter);
+           VEC_iterate (int, bl->owner->syscalls_to_be_caught, i, iter);
            i++)
 	{
           int elem;
@@ -6546,14 +6546,14 @@ create_fork_vfork_event_catchpoint (struct gdbarch *gdbarch,
 
 /* Exec catchpoints.  */
 
-static void
-insert_catch_exec (struct breakpoint *b)
+static int
+insert_catch_exec (struct bp_location *bl)
 {
-  target_insert_exec_catchpoint (PIDGET (inferior_ptid));
+  return target_insert_exec_catchpoint (PIDGET (inferior_ptid));
 }
 
 static int
-remove_catch_exec (struct breakpoint *b)
+remove_catch_exec (struct bp_location *bl)
 {
   return target_remove_exec_catchpoint (PIDGET (inferior_ptid));
 }
@@ -8211,6 +8211,37 @@ watchpoint_exp_is_const (const struct expression *exp)
   return 1;
 }
 
+/* Implement the "insert" breakpoint_ops method for hardware watchpoints.  */
+
+static int
+insert_watchpoint (struct bp_location *bl)
+{
+  return target_insert_watchpoint (bl->address, bl->length,
+				   bl->watchpoint_type, bl->owner->cond_exp);
+}
+
+/* Implement the "remove" breakpoint_ops method for hardware watchpoints.  */
+
+static int
+remove_watchpoint (struct bp_location *bl)
+{
+  return target_remove_watchpoint (bl->address, bl->length,
+				   bl->watchpoint_type, bl->owner->cond_exp);
+}
+
+/* The breakpoint_ops structure to be used in hardware watchpoints.  */
+
+static struct breakpoint_ops watchpoint_breakpoint_ops =
+{
+  insert_watchpoint,
+  remove_watchpoint,
+  NULL, /* breakpoint_hit */
+  NULL, /* print_it */
+  NULL, /* print_one */
+  NULL, /* print_mention */
+  NULL  /* print_recreate */
+};
+
 /* accessflag:  hw_write:  watch write, 
                 hw_read:   watch read, 
 		hw_access: watch access (read or write) */
@@ -8454,6 +8485,8 @@ watch_command_1 (char *arg, int accessflag, int from_tty,
     b->exp_string = savestring (exp_start, exp_end - exp_start);
   b->val = val;
   b->val_valid = 1;
+  b->ops = &watchpoint_breakpoint_ops;
+
   if (cond_start)
     b->cond_string = savestring (cond_start, cond_end - cond_start);
   else
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 2d815c2..cd9b6b8 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -368,16 +368,18 @@ struct bp_location
    will be called instead of the performing the default action for this
    bptype.  */
 
-struct breakpoint_ops 
+struct breakpoint_ops
 {
-  /* Insert the breakpoint or activate the catchpoint.  Should raise
-     an exception if the operation failed.  */
-  void (*insert) (struct breakpoint *);
+  /* Insert the breakpoint or watchpoint or activate the catchpoint.
+     Return 0 for success, 1 if the breakpoint, watchpoint or catchpoint
+     type is not supported, -1 for failure.  */
+  int (*insert_location) (struct bp_location *);
 
   /* Remove the breakpoint/catchpoint that was previously inserted
-     with the "insert" method above.  Return non-zero if the operation
-     succeeded.  */
-  int (*remove) (struct breakpoint *);
+     with the "insert" method above.  Return 0 for success, 1 if the
+     breakpoint, watchpoint or catchpoint type is not supported,
+     -1 for failure.  */
+  int (*remove_location) (struct bp_location *);
 
   /* Return non-zero if the debugger should tell the user that this
      breakpoint was hit.  */
diff --git a/gdb/inf-child.c b/gdb/inf-child.c
index 6802343..1c45483 100644
--- a/gdb/inf-child.c
+++ b/gdb/inf-child.c
@@ -87,36 +87,6 @@ inf_child_post_startup_inferior (ptid_t ptid)
      inferior" operation by a debugger.  */
 }
 
-static void
-inf_child_insert_fork_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of fork
-     events.  */
-}
-
-static int
-inf_child_remove_fork_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of fork
-     events.  */
-  return 0;
-}
-
-static void
-inf_child_insert_vfork_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of vfork
-     events.  */
-}
-
-static int
-inf_child_remove_vfork_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of vfork
-     events.  */
-  return 0;
-}
-
 static int
 inf_child_follow_fork (struct target_ops *ops, int follow_child)
 {
@@ -125,30 +95,6 @@ inf_child_follow_fork (struct target_ops *ops, int follow_child)
   return 0;
 }
 
-static void
-inf_child_insert_exec_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of exec
-     events.  */
-}
-
-static int
-inf_child_remove_exec_catchpoint (int pid)
-{
-  /* This version of Unix doesn't support notification of exec
-     events.  */
-  return 0;
-}
-
-static int
-inf_child_set_syscall_catchpoint (int pid, int needed, int any_count,
-				  int table_size, int *table)
-{
-  /* This version of Unix doesn't support notification of syscall
-     events.  */
-  return 0;
-}
-
 static int
 inf_child_can_run (void)
 {
@@ -185,14 +131,7 @@ inf_child_target (void)
   t->to_terminal_ours = terminal_ours;
   t->to_terminal_info = child_terminal_info;
   t->to_post_startup_inferior = inf_child_post_startup_inferior;
-  t->to_insert_fork_catchpoint = inf_child_insert_fork_catchpoint;
-  t->to_remove_fork_catchpoint = inf_child_remove_fork_catchpoint;
-  t->to_insert_vfork_catchpoint = inf_child_insert_vfork_catchpoint;
-  t->to_remove_vfork_catchpoint = inf_child_remove_vfork_catchpoint;
   t->to_follow_fork = inf_child_follow_fork;
-  t->to_insert_exec_catchpoint = inf_child_insert_exec_catchpoint;
-  t->to_remove_exec_catchpoint = inf_child_remove_exec_catchpoint;
-  t->to_set_syscall_catchpoint = inf_child_set_syscall_catchpoint;
   t->to_can_run = inf_child_can_run;
   t->to_pid_to_exec_file = inf_child_pid_to_exec_file;
   t->to_stratum = process_stratum;
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index c769010..62a4538 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -974,36 +974,34 @@ holding the child stopped.  Try \"set detach-on-fork\" or \
 }
 
 \f
-static void
+static int
 linux_child_insert_fork_catchpoint (int pid)
 {
-  if (! linux_supports_tracefork (pid))
-    error (_("Your system does not support fork catchpoints."));
+  return !linux_supports_tracefork (pid);
 }
 
-static void
+static int
 linux_child_insert_vfork_catchpoint (int pid)
 {
-  if (!linux_supports_tracefork (pid))
-    error (_("Your system does not support vfork catchpoints."));
+  return !linux_supports_tracefork (pid);
 }
 
-static void
+static int
 linux_child_insert_exec_catchpoint (int pid)
 {
-  if (!linux_supports_tracefork (pid))
-    error (_("Your system does not support exec catchpoints."));
+  return !linux_supports_tracefork (pid);
 }
 
 static int
 linux_child_set_syscall_catchpoint (int pid, int needed, int any_count,
 				    int table_size, int *table)
 {
-  if (! linux_supports_tracesysgood (pid))
-    error (_("Your system does not support syscall catchpoints."));
+  if (!linux_supports_tracesysgood (pid))
+    return 1;
+
   /* On GNU/Linux, we ignore the arguments.  It means that we only
      enable the syscall catchpoints, but do not disable them.
-     
+
      Also, we do not use the `table' information because we do not
      filter system calls here.  We let GDB do the logic for us.  */
   return 0;
diff --git a/gdb/target.c b/gdb/target.c
index bfb2bbd..4854834 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -779,26 +779,26 @@ update_current_target (void)
 	    (void (*) (ptid_t))
 	    target_ignore);
   de_fault (to_insert_fork_catchpoint,
-	    (void (*) (int))
-	    tcomplain);
+	    (int (*) (int))
+	    return_one);
   de_fault (to_remove_fork_catchpoint,
 	    (int (*) (int))
-	    tcomplain);
+	    return_one);
   de_fault (to_insert_vfork_catchpoint,
-	    (void (*) (int))
-	    tcomplain);
+	    (int (*) (int))
+	    return_one);
   de_fault (to_remove_vfork_catchpoint,
 	    (int (*) (int))
-	    tcomplain);
+	    return_one);
   de_fault (to_insert_exec_catchpoint,
-	    (void (*) (int))
-	    tcomplain);
+	    (int (*) (int))
+	    return_one);
   de_fault (to_remove_exec_catchpoint,
 	    (int (*) (int))
-	    tcomplain);
+	    return_one);
   de_fault (to_set_syscall_catchpoint,
 	    (int (*) (int, int, int, int, int *))
-	    tcomplain);
+	    return_one);
   de_fault (to_has_exited,
 	    (int (*) (int, int, int *))
 	    return_zero);
@@ -3661,13 +3661,17 @@ debug_to_post_startup_inferior (ptid_t ptid)
 		      PIDGET (ptid));
 }
 
-static void
+static int
 debug_to_insert_fork_catchpoint (int pid)
 {
-  debug_target.to_insert_fork_catchpoint (pid);
+  int retval;
+
+  retval = debug_target.to_insert_fork_catchpoint (pid);
+
+  fprintf_unfiltered (gdb_stdlog, "target_insert_fork_catchpoint (%d) = %d\n",
+		      pid, retval);
 
-  fprintf_unfiltered (gdb_stdlog, "target_insert_fork_catchpoint (%d)\n",
-		      pid);
+  return retval;
 }
 
 static int
@@ -3683,13 +3687,17 @@ debug_to_remove_fork_catchpoint (int pid)
   return retval;
 }
 
-static void
+static int
 debug_to_insert_vfork_catchpoint (int pid)
 {
-  debug_target.to_insert_vfork_catchpoint (pid);
+  int retval;
+
+  retval = debug_target.to_insert_vfork_catchpoint (pid);
 
-  fprintf_unfiltered (gdb_stdlog, "target_insert_vfork_catchpoint (%d)\n",
-		      pid);
+  fprintf_unfiltered (gdb_stdlog, "target_insert_vfork_catchpoint (%d) = %d\n",
+		      pid, retval);
+
+  return retval;
 }
 
 static int
@@ -3705,13 +3713,17 @@ debug_to_remove_vfork_catchpoint (int pid)
   return retval;
 }
 
-static void
+static int
 debug_to_insert_exec_catchpoint (int pid)
 {
-  debug_target.to_insert_exec_catchpoint (pid);
+  int retval;
+
+  retval = debug_target.to_insert_exec_catchpoint (pid);
 
-  fprintf_unfiltered (gdb_stdlog, "target_insert_exec_catchpoint (%d)\n",
-		      pid);
+  fprintf_unfiltered (gdb_stdlog, "target_insert_exec_catchpoint (%d) = %d\n",
+		      pid, retval);
+
+  return retval;
 }
 
 static int
diff --git a/gdb/target.h b/gdb/target.h
index c34625c..72fd211 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -468,12 +468,12 @@ struct target_ops
     void (*to_create_inferior) (struct target_ops *, 
 				char *, char *, char **, int);
     void (*to_post_startup_inferior) (ptid_t);
-    void (*to_insert_fork_catchpoint) (int);
+    int (*to_insert_fork_catchpoint) (int);
     int (*to_remove_fork_catchpoint) (int);
-    void (*to_insert_vfork_catchpoint) (int);
+    int (*to_insert_vfork_catchpoint) (int);
     int (*to_remove_vfork_catchpoint) (int);
     int (*to_follow_fork) (struct target_ops *, int);
-    void (*to_insert_exec_catchpoint) (int);
+    int (*to_insert_exec_catchpoint) (int);
     int (*to_remove_exec_catchpoint) (int);
     int (*to_set_syscall_catchpoint) (int, int, int, int, int *);
     int (*to_has_exited) (int, int, int *);
@@ -1029,7 +1029,8 @@ void target_create_inferior (char *exec_file, char *args,
 
 /* On some targets, we can catch an inferior fork or vfork event when
    it occurs.  These functions insert/remove an already-created
-   catchpoint for such events.  */
+   catchpoint for such events.  They return  0 for success, 1 if the
+   catchpoint type is not supported and -1 for failure.  */
 
 #define target_insert_fork_catchpoint(pid) \
      (*current_target.to_insert_fork_catchpoint) (pid)
@@ -1055,7 +1056,8 @@ int target_follow_fork (int follow_child);
 
 /* On some targets, we can catch an inferior exec event when it
    occurs.  These functions insert/remove an already-created
-   catchpoint for such events.  */
+   catchpoint for such events.  They return  0 for success, 1 if the
+   catchpoint type is not supported and -1 for failure.  */
 
 #define target_insert_exec_catchpoint(pid) \
      (*current_target.to_insert_exec_catchpoint) (pid)
@@ -1078,7 +1080,10 @@ int target_follow_fork (int follow_child);
 
    TABLE is an array of ints, indexed by syscall number.  An element in
    this array is nonzero if that syscall should be caught.  This argument
-   only matters if ANY_COUNT is zero.  */
+   only matters if ANY_COUNT is zero.
+
+   Return 0 for success, 1 if syscall catchpoints are not supported or -1
+   for failure.  */
 
 #define target_set_syscall_catchpoint(pid, needed, any_count, table_size, table) \
      (*current_target.to_set_syscall_catchpoint) (pid, needed, any_count, \
diff --git a/gdb/testsuite/gdb.base/foll-exec.exp b/gdb/testsuite/gdb.base/foll-exec.exp
index 88194fb..64c1bfd 100644
--- a/gdb/testsuite/gdb.base/foll-exec.exp
+++ b/gdb/testsuite/gdb.base/foll-exec.exp
@@ -89,7 +89,7 @@ proc do_exec_tests {} {
    gdb_test "catch exec" "Catchpoint \[0-9\]* \\(exec\\)" "insert first exec catchpoint"
    set has_exec_catchpoints 0
    gdb_test_multiple "continue" "continue to first exec catchpoint" {
-     -re ".*Your system does not support exec catchpoints.*$gdb_prompt $" {
+     -re ".*Your system does not support this type\r\nof catchpoint.*$gdb_prompt $" {
        unsupported "continue to first exec catchpoint"
      }
      -re ".*Catchpoint.*$gdb_prompt $" {
diff --git a/gdb/testsuite/gdb.base/foll-fork.exp b/gdb/testsuite/gdb.base/foll-fork.exp
index 36a56b5..40d0702 100644
--- a/gdb/testsuite/gdb.base/foll-fork.exp
+++ b/gdb/testsuite/gdb.base/foll-fork.exp
@@ -45,7 +45,7 @@ proc check_fork_catchpoints {} {
   gdb_test "catch fork" "Catchpoint \[0-9\]* \\(fork\\)" "insert first fork catchpoint"
   set has_fork_catchpoints 0
   gdb_test_multiple "continue" "continue to first fork catchpoint" {
-    -re ".*Your system does not support fork catchpoints.*$gdb_prompt $" {
+    -re ".*Your system does not support this type\r\nof catchpoint.*$gdb_prompt $" {
       unsupported "continue to first fork catchpoint"
     }
     -re ".*Catchpoint.*$gdb_prompt $" {
diff --git a/gdb/testsuite/gdb.base/foll-vfork.exp b/gdb/testsuite/gdb.base/foll-vfork.exp
index 1bad957..f8a3eeb 100644
--- a/gdb/testsuite/gdb.base/foll-vfork.exp
+++ b/gdb/testsuite/gdb.base/foll-vfork.exp
@@ -74,7 +74,7 @@ proc check_vfork_catchpoints {} {
   gdb_test "catch vfork" "Catchpoint \[0-9\]* \\(vfork\\)" "insert first vfork catchpoint"
   set has_vfork_catchpoints 0
   gdb_test_multiple "continue" "continue to first vfork catchpoint" {
-    -re ".*Your system does not support vfork catchpoints.*$gdb_prompt $" {
+    -re ".*Your system does not support this type\r\nof catchpoint.*$gdb_prompt $" {
       unsupported "continue to first vfork catchpoint"
     }
     -re ".*Catchpoint.*$gdb_prompt $" {


^ permalink raw reply	[flat|nested] 24+ messages in thread

* "Cannot remove breakpoints because program is no longer writable" & catchpoints (was: Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops)
  2011-01-11 19:31               ` Thiago Jung Bauermann
@ 2011-04-29 15:51                 ` Pedro Alves
  0 siblings, 0 replies; 24+ messages in thread
From: Pedro Alves @ 2011-04-29 15:51 UTC (permalink / raw)
  To: gdb-patches

I just noticed:

 (gdb) catch fork
 Catchpoint 2 (fork)
 (gdb) n
 Cannot remove breakpoints because program is no longer writable.
 Further execution is probably impossible.
 96            args[j] = j;
 (gdb) 

Turns out to be a a simple overlook:

On Tuesday 11 January 2011 19:30:20, Thiago Jung Bauermann wrote:

> 2010-01-11  Thiago Jung Bauermann  <bauerman@br.ibm.com>
> 
>         Convert hardware watchpoints to use breakpoint_ops.
> 
> gdb/
>         * inf-child.c (inf_child_insert_fork_catchpoint)
>         (inf_child_remove_fork_catchpoint, inf_child_insert_vfork_catchpoint)
>         (inf_child_remove_vfork_catchpoint, inf_child_insert_exec_catchpoint)
>         (inf_child_remove_exec_catchpoint, inf_child_set_syscall_catchpoint):
>         Delete functions.

On Tuesday 17 August 2010 20:41:11, Thiago Jung Bauermann wrote:
> This patch actually fixes some inconsistencies in the catchpoints
> support: target.c sets the default implementation of
> to_remove_{fork,vfork,exec}_catchpoint to be tcomplain, which throws an
> exception, but remove_breakpoint_1 is not prepared to deal with it. The
> only target which implements catchpoints is linux-nat.c and it doesn't
> actually implement the to_remove_* methods, so in theory an exception
> would be thrown when GDB tried to remove the catchpoints. The only
> reason it works is that inf-child.c implements the remove methods to
> return 0 indicating that the target doesn't support the feature and
> linux-nat.c inherits those...

Since inf_child_remove_*_catchpoint functions were removed (which
were returning success), the linux-nat.c target needs to gain its
own implementation of these functions, otherwise, it gets the default
implementation, that now returns error.

Tested on x86_64-linux.  Applying on trunk and branch.

Pedro Alves

2011-04-29  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* linux-nat.c (linux_child_remove_fork_catchpoint)
	(linux_child_remove_vfork_catchpoint)
	(linux_child_remove_exec_catchpoint): New functions.
	(linux_target_install_ops): Install them.

---
 gdb/linux-nat.c |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Index: src/gdb/linux-nat.c
===================================================================
--- src.orig/gdb/linux-nat.c	2011-04-29 16:32:33.605644000 +0100
+++ src/gdb/linux-nat.c	2011-04-29 16:36:16.285644002 +0100
@@ -944,18 +944,36 @@ linux_child_insert_fork_catchpoint (int
 }
 
 static int
+linux_child_remove_fork_catchpoint (int pid)
+{
+  return 0;
+}
+
+static int
 linux_child_insert_vfork_catchpoint (int pid)
 {
   return !linux_supports_tracefork (pid);
 }
 
 static int
+linux_child_remove_vfork_catchpoint (int pid)
+{
+  return 0;
+}
+
+static int
 linux_child_insert_exec_catchpoint (int pid)
 {
   return !linux_supports_tracefork (pid);
 }
 
 static int
+linux_child_remove_exec_catchpoint (int pid)
+{
+  return 0;
+}
+
+static int
 linux_child_set_syscall_catchpoint (int pid, int needed, int any_count,
 				    int table_size, int *table)
 {
@@ -5214,8 +5232,11 @@ static void
 linux_target_install_ops (struct target_ops *t)
 {
   t->to_insert_fork_catchpoint = linux_child_insert_fork_catchpoint;
+  t->to_remove_fork_catchpoint = linux_child_remove_fork_catchpoint;
   t->to_insert_vfork_catchpoint = linux_child_insert_vfork_catchpoint;
+  t->to_remove_vfork_catchpoint = linux_child_remove_vfork_catchpoint;
   t->to_insert_exec_catchpoint = linux_child_insert_exec_catchpoint;
+  t->to_remove_exec_catchpoint = linux_child_remove_exec_catchpoint;
   t->to_set_syscall_catchpoint = linux_child_set_syscall_catchpoint;
   t->to_pid_to_exec_file = linux_child_pid_to_exec_file;
   t->to_post_startup_inferior = linux_child_post_startup_inferior;

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2011-04-29 15:51 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-08-17 19:41 [patch 1/2] Convert hardware watchpoints to use breakpoint_ops Thiago Jung Bauermann
2010-10-07 14:47 ` Thiago Jung Bauermann
2010-10-16 17:43 ` Pedro Alves
2010-10-20  0:31   ` Thiago Jung Bauermann
2010-11-04 21:17     ` Thiago Jung Bauermann
2010-11-08 18:43     ` Joel Brobecker
2010-11-08 21:39       ` Thiago Jung Bauermann
2010-11-15 22:23     ` Joel Brobecker
2010-11-16 19:03       ` Thiago Jung Bauermann
2010-11-18 17:18         ` Joel Brobecker
2010-11-19 20:10           ` Thiago Jung Bauermann
2010-12-23 19:06             ` Thiago Jung Bauermann
2011-01-11 19:31               ` Thiago Jung Bauermann
2011-04-29 15:51                 ` "Cannot remove breakpoints because program is no longer writable" & catchpoints (was: Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops) Pedro Alves
2010-11-16  4:06     ` [patch 1/2] Convert hardware watchpoints to use breakpoint_ops Jan Kratochvil
2010-11-16  8:07       ` Joel Brobecker
2010-11-16 18:51         ` Jan Kratochvil
2010-11-17  3:47           ` [patch] Renaming: {insert,remove} += _location [Re: [patch 1/2] Convert hardware watchpoints to use breakpoint_ops] Jan Kratochvil
2010-11-18 17:13             ` Joel Brobecker
2010-11-17  3:47           ` [patch] renaming: bp_location: b->bl &co. " Jan Kratochvil
2010-11-18 17:15             ` Joel Brobecker
2010-12-23 18:50             ` Thiago Jung Bauermann
2010-12-24  5:14               ` Joel Brobecker
2010-12-27 20:04                 ` Thiago Jung Bauermann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).