[rfc;rfa:breakpoint] Pass full breakpoint/watchpoint count to target

[rfc;rfa:breakpoint] Pass full breakpoint/watchpoint count to target

[Andrew Cagney]

[-- Attachment #1: Type: text/plain, Size: 500 bytes --]

Hello,

The attached patch changes the target vector so that a count of each 
type of hardware watchpoint/breakpoint is passed to the 
target_can_use_hardware_breakpoints() function. (Anyone got a better 
name for the function?).

This lets targets make a better guess at how many watchpoints are going 
to be needed.  The intent is for the Z? packet to pass these totals down 
to the target.

Thoughts?

Is the breakpoint side ok?

(I need to find more hw with breakpoints to test against.)
Andrew


[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 12971 bytes --]

2002-09-29  Andrew Cagney  <ac131313@redhat.com>

	* target.h (target_can_use_hardware_breakpoint): Declare.
	(TARGET_CAN_USE_HARDWARE_WATCHPOINT): Delete macro.
	(struct target_ops): Change type of to_can_use_hw_breakpoint.
	* target.c (cleanup_target): Update.
	(debug_to_can_use_hw_breakpoint): Update.
	(target_can_use_hardware_breakpoint): New function.

	* breakpoint.h (NUM_BPTYPES): Define.
	* breakpoint.c (hw_breakpoint_used_count): Delete function.
	(hw_resources_used_count): Replace hw_watchpoint_used_count.
	Compute memory count for all hardware watchpoint types.
	(watch_command_1, do_enable_breakpoint, create_breakpoints): Use
	target_can_use_hardware_watchpoint and hw_resources_used_count.

	* remote.c (remote_check_watch_resources): Update.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.90
diff -u -r1.90 breakpoint.c
--- breakpoint.c	22 Sep 2002 20:29:52 -0000	1.90
+++ breakpoint.c	30 Sep 2002 03:19:02 -0000
@@ -149,10 +149,6 @@
 
 static void create_overlay_event_breakpoint (char *);
 
-static int hw_breakpoint_used_count (void);
-
-static int hw_watchpoint_used_count (enum bptype, int *);
-
 static void hbreak_command (char *, int);
 
 static void thbreak_command (char *, int);
@@ -4262,42 +4258,28 @@
   mention (b);
 }
 
-static int
-hw_breakpoint_used_count (void)
-{
-  register struct breakpoint *b;
-  int i = 0;
+/* Accumulate the total number of hardware watchpoints that have so
+   far been used (including an additional COUNT of TYPE).  */
 
-  ALL_BREAKPOINTS (b)
-  {
-    if (b->type == bp_hardware_breakpoint && b->enable_state == bp_enabled)
-      i++;
-  }
-
-  return i;
-}
-
-static int
-hw_watchpoint_used_count (enum bptype type, int *other_type_used)
+static void
+hw_resources_used_count (enum bptype type, int count, int *used)
 {
-  register struct breakpoint *b;
-  int i = 0;
-
-  *other_type_used = 0;
+  struct breakpoint *b;
+  memset (used, 0, NUM_BPTYPES * sizeof (used[0]));
+  used[type] = count;
   ALL_BREAKPOINTS (b)
   {
     if (b->enable_state == bp_enabled)
       {
-	if (b->type == type)
-	  i++;
-	else if ((b->type == bp_hardware_watchpoint ||
-		  b->type == bp_read_watchpoint ||
-		  b->type == bp_access_watchpoint)
-		 && b->enable_state == bp_enabled)
-	  *other_type_used = 1;
+	if (b->type == bp_hardware_watchpoint
+	    || b->type == bp_read_watchpoint
+	    || b->type == bp_access_watchpoint
+	    || b->type == bp_hardware_breakpoint)
+	  {
+	    used[b->type] += 1;
+	  }
       }
   }
-  return i;
 }
 
 /* Call this after hitting the longjmp() breakpoint.  Use this to set
@@ -4544,10 +4526,11 @@
 {
   if (type == bp_hardware_breakpoint)
     {
-      int i = hw_breakpoint_used_count ();
-      int target_resources_ok = 
-	TARGET_CAN_USE_HARDWARE_WATCHPOINT (bp_hardware_breakpoint, 
-					    i + sals.nelts, 0);
+      int used[NUM_BPTYPES];
+      int target_resources_ok;
+      hw_resources_used_count (bp_hardware_breakpoint, sals.nelts, used);
+      target_resources_ok = 
+	target_can_use_hardware_breakpoint (bp_hardware_breakpoint, used);
       if (target_resources_ok == 0)
 	error ("No hardware breakpoint support in the target.");
       else if (target_resources_ok < 0)
@@ -5289,7 +5272,7 @@
   char *cond_start = NULL;
   char *cond_end = NULL;
   struct expression *cond = NULL;
-  int i, other_type_used, target_resources_ok = 0;
+  int i, target_resources_ok = 0;
   enum bptype bp_type;
   int mem_cnt = 0;
 
@@ -5337,10 +5320,9 @@
     error ("Expression cannot be implemented with read/access watchpoint.");
   if (mem_cnt != 0)
     {
-      i = hw_watchpoint_used_count (bp_type, &other_type_used);
-      target_resources_ok = 
-	TARGET_CAN_USE_HARDWARE_WATCHPOINT (bp_type, i + mem_cnt, 
-					    other_type_used);
+      int used[NUM_BPTYPES];
+      hw_resources_used_count (bp_type, mem_cnt, used);
+      target_resources_ok = target_can_use_hardware_breakpoint (bp_type, used);
       if (target_resources_ok == 0 && bp_type != bp_hardware_watchpoint)
 	error ("Target does not support this type of hardware watchpoint.");
 
@@ -7322,16 +7304,15 @@
 {
   struct frame_info *save_selected_frame = NULL;
   int save_selected_frame_level = -1;
-  int target_resources_ok, other_type_used;
+  int target_resources_ok;
   struct value *mark;
 
   if (bpt->type == bp_hardware_breakpoint)
     {
-      int i;
-      i = hw_breakpoint_used_count ();
+      int used[NUM_BPTYPES];
+      hw_resources_used_count (bp_hardware_breakpoint, 1, used);
       target_resources_ok = 
-	TARGET_CAN_USE_HARDWARE_WATCHPOINT (bp_hardware_breakpoint, 
-					    i + 1, 0);
+	target_can_use_hardware_breakpoint (bp_hardware_breakpoint, used);
       if (target_resources_ok == 0)
 	error ("No hardware breakpoint support in the target.");
       else if (target_resources_ok < 0)
@@ -7378,13 +7359,11 @@
 	  bpt->type == bp_read_watchpoint ||
 	  bpt->type == bp_access_watchpoint)
 	{
-	  int i = hw_watchpoint_used_count (bpt->type, &other_type_used);
+	  int used[NUM_BPTYPES];
 	  int mem_cnt = can_use_hardware_watchpoint (bpt->val);
-
-	  /* Hack around 'unused var' error for some targets here */
-	  (void) mem_cnt, i;
-	  target_resources_ok = TARGET_CAN_USE_HARDWARE_WATCHPOINT (
-				   bpt->type, i + mem_cnt, other_type_used);
+	  hw_resources_used_count (bpt->type, mem_cnt, used);
+	  target_resources_ok =
+	    target_can_use_hardware_breakpoint (bpt->type, used);
 	  /* we can consider of type is bp_hardware_watchpoint, convert to 
 	     bp_watchpoint in the following condition */
 	  if (target_resources_ok < 0)
Index: breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.13
diff -u -r1.13 breakpoint.h
--- breakpoint.h	16 Aug 2002 15:37:54 -0000	1.13
+++ breakpoint.h	30 Sep 2002 03:19:17 -0000
@@ -1,6 +1,7 @@
 /* Data structures associated with breakpoints in GDB.
-   Copyright 1992, 1993, 1994, 1995, 1996, 1997, 1998, 1999, 2000
-   Free Software Foundation, Inc.
+
+   Copyright 1992, 1993, 1994, 1995, 1996, 1997, 1998, 1999, 2000,
+   2002 Free Software Foundation, Inc.
 
    This file is part of GDB.
 
@@ -138,8 +139,10 @@
     bp_catch_catch,
     bp_catch_throw
 
-
+    /* NOTE: If you update this, remember to update NUM_BPTYPES below.  */
   };
+enum { NUM_BPTYPES = bp_catch_throw + 1 };
+
 
 /* States of enablement of breakpoint. */
 
Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.93
diff -u -r1.93 remote.c
--- remote.c	18 Aug 2002 23:17:57 -0000	1.93
+++ remote.c	30 Sep 2002 03:20:09 -0000
@@ -4842,22 +4842,26 @@
 int remote_hw_breakpoint_limit = 0;
 
 int
-remote_check_watch_resources (int type, int cnt, int ot)
+remote_check_watch_resources (int type, int *used)
 {
   if (type == bp_hardware_breakpoint)
     {
       if (remote_hw_breakpoint_limit == 0)
 	return 0;
-      else if (cnt <= remote_hw_breakpoint_limit)
+      else if (used[type] <= remote_hw_breakpoint_limit)
 	return 1;
     }
   else
     {
+      int count = (used[bp_hardware_watchpoint]
+		   + used[bp_read_watchpoint]
+		   + used[bp_access_watchpoint]);
+      int other = (count > used[type]);
       if (remote_hw_watchpoint_limit == 0)
 	return 0;
-      else if (ot)
+      else if (other)
 	return -1;
-      else if (cnt <= remote_hw_watchpoint_limit)
+      else if (count <= remote_hw_watchpoint_limit)
 	return 1;
     }
   return -1;
Index: target.c
===================================================================
RCS file: /cvs/src/src/gdb/target.c,v
retrieving revision 1.40
diff -u -r1.40 target.c
--- target.c	26 Aug 2002 19:18:33 -0000	1.40
+++ target.c	30 Sep 2002 03:20:41 -0000
@@ -116,8 +116,6 @@
 
 static int debug_to_remove_breakpoint (CORE_ADDR, char *);
 
-static int debug_to_can_use_hw_breakpoint (int, int, int);
-
 static int debug_to_insert_hw_breakpoint (CORE_ADDR, char *);
 
 static int debug_to_remove_hw_breakpoint (CORE_ADDR, char *);
@@ -413,7 +411,7 @@
   de_fault (to_remove_breakpoint, 
 	    memory_remove_breakpoint);
   de_fault (to_can_use_hw_breakpoint,
-	    (int (*) (int, int, int))
+	    (int (*) (int, int *))
 	    return_zero);
   de_fault (to_insert_hw_breakpoint,
 	    (int (*) (CORE_ADDR, char *))
@@ -1112,6 +1110,33 @@
   return target_xfer_memory_partial (memaddr, buf, len, 1, err);
 }
 
+/* Can the target handle breakpoints or watchpoints.  */
+int
+target_can_use_hardware_breakpoint (int bptype, int *used)
+{
+#ifdef TARGET_CAN_USE_HARDWARE_WATCHPOINT
+  /* ULGH!  Old target that hasn't yet integrated things into the
+     target vector.  Fake up old style call.  */
+  if (bptype == bp_hardware_breakpoint)
+    /* For hardware breakpoints, just pass down the total number of
+       hardware breakpoints needed.  */
+    return TARGET_CAN_USE_HARDWARE_WATCHPOINT (bptype, used[bptype], 0);
+  else
+    {
+      /* For watchpoints, pass down both the total number of hardware
+         breakpoints of this type, and a flag indicating that any
+         other watchpoint is in use.  */
+      int count = (used[bp_hardware_watchpoint]
+		   + used[bp_read_watchpoint]
+		   + used[bp_access_watchpoint]);
+      int other = (count > used[bptype]);
+      return TARGET_CAN_USE_HARDWARE_WATCHPOINT (bptype, used[bptype], other);
+    }
+#else
+  return current_target.to_can_use_hw_breakpoint (bptype, used);
+#endif
+}
+
 /* ARGSUSED */
 static void
 target_info (char *args, int from_tty)
@@ -1844,18 +1869,47 @@
 }
 
 static int
-debug_to_can_use_hw_breakpoint (int type, int cnt, int from_tty)
+debug_to_can_use_hw_breakpoint (int type, int *used)
 {
   int retval;
 
-  retval = debug_target.to_can_use_hw_breakpoint (type, cnt, from_tty);
+  retval = debug_target.to_can_use_hw_breakpoint (type, used);
 
   fprintf_unfiltered (gdb_stdlog,
-		      "target_can_use_hw_breakpoint (%ld, %ld, %ld) = %ld\n",
-		      (unsigned long) type,
-		      (unsigned long) cnt,
-		      (unsigned long) from_tty,
-		      (unsigned long) retval);
+		      "target_can_use_hw_breakpoint (%ld, used[",
+		      (unsigned long) type);
+  {
+    int i;
+    const char *sep = "";
+    for (i = 0; i < NUM_BPTYPES; i++)
+      {
+	if (used[i] != 0)
+	  {
+	    fprintf_unfiltered (gdb_stdlog, "%s", sep);
+	    sep = ",";
+	    switch (i)
+	      {
+	      case bp_hardware_watchpoint:
+		fprintf_unfiltered (gdb_stdlog, "bp_hardware_watchpoint");
+		break;
+	      case bp_read_watchpoint:
+		fprintf_unfiltered (gdb_stdlog, "bp_read_watchpoint");
+		break;
+	      case bp_access_watchpoint:
+		fprintf_unfiltered (gdb_stdlog, "bp_access_watchpoint");
+		break;  
+	      case bp_hardware_breakpoint:
+		fprintf_unfiltered (gdb_stdlog, "bp_hardware_breakpoint");
+		break;  
+	      default:
+		fprintf_unfiltered (gdb_stdlog, "%d", i);
+		break;
+	      }
+	    fprintf_unfiltered (gdb_stdlog, "=%d", used[i]);
+	  }
+      }
+  }
+  fprintf_unfiltered (gdb_stdlog, "]) = %ld\n", (unsigned long) retval);
   return retval;
 }
 
Index: target.h
===================================================================
RCS file: /cvs/src/src/gdb/target.h,v
retrieving revision 1.26
diff -u -r1.26 target.h
--- target.h	26 Aug 2002 19:18:33 -0000	1.26
+++ target.h	30 Sep 2002 03:22:52 -0000
@@ -252,7 +252,7 @@
     void (*to_files_info) (struct target_ops *);
     int (*to_insert_breakpoint) (CORE_ADDR, char *);
     int (*to_remove_breakpoint) (CORE_ADDR, char *);
-    int (*to_can_use_hw_breakpoint) (int, int, int);
+    int (*to_can_use_hw_breakpoint) (int, int *);
     int (*to_insert_hw_breakpoint) (CORE_ADDR, char *);
     int (*to_remove_hw_breakpoint) (CORE_ADDR, char *);
     int (*to_remove_watchpoint) (CORE_ADDR, int, int);
@@ -1079,15 +1079,13 @@
 /* If the *_hw_beakpoint functions have not been defined 
    elsewhere use the definitions in the target vector.  */
 
-/* Returns non-zero if we can set a hardware watchpoint of type TYPE.  TYPE is
-   one of bp_hardware_watchpoint, bp_read_watchpoint, bp_write_watchpoint, or
-   bp_hardware_breakpoint.  CNT is the number of such watchpoints used so far
-   (including this one?).  OTHERTYPE is who knows what...  */
-
-#ifndef TARGET_CAN_USE_HARDWARE_WATCHPOINT
-#define TARGET_CAN_USE_HARDWARE_WATCHPOINT(TYPE,CNT,OTHERTYPE) \
- (*current_target.to_can_use_hw_breakpoint) (TYPE, CNT, OTHERTYPE);
-#endif
+/* Returns non-zero if we can set a hardware breakpoint or watchpoint
+   of type TYPE.  TYPE is one of bp_hardware_watchpoint,
+   bp_read_watchpoint, bp_access_watchpoint, or
+   bp_hardware_breakpoint.  USED is an array containing a count of all
+   the hardware breakpoints and watchpoints needed.  */
+
+extern int target_can_use_hardware_breakpoint (int bptype, int *used);
 
 #if !defined(TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT)
 #define TARGET_REGION_SIZE_OK_FOR_HW_WATCHPOINT(byte_count) \

Re: [rfc;rfa:breakpoint] Pass full breakpoint/watchpoint count to target

[Eli Zaretskii]

On Sun, 29 Sep 2002, Andrew Cagney wrote:

> The attached patch changes the target vector so that a count of each 
> type of hardware watchpoint/breakpoint is passed to the 
> target_can_use_hardware_breakpoints() function. (Anyone got a better 
> name for the function?).
> 
> This lets targets make a better guess at how many watchpoints are going 
> to be needed.

Sorry, I don't see the large-scale picture: how will this facility be 
used in routine GDB operations by the application-level GDB code?

Also, on a i386, for example, if you don't pass the size and alignment of 
each address to be watched, the guess of the amount of required resources 
can be very wrong.  This is even more exacerbated due to debug register 
sharing implemented for i386.

> The intent is for the Z? packet to pass these totals down 
> to the target.

Do you mean to say this is only for remote targets?

Re: [rfc;rfa:breakpoint] Pass full breakpoint/watchpoint count to target

[Andrew Cagney]

> On Sun, 29 Sep 2002, Andrew Cagney wrote:
> 
> 
>> The attached patch changes the target vector so that a count of each 
>> type of hardware watchpoint/breakpoint is passed to the 
>> target_can_use_hardware_breakpoints() function. (Anyone got a better 
>> name for the function?).
>> 
>> This lets targets make a better guess at how many watchpoints are going 
>> to be needed.
> 
> 
> Sorry, I don't see the large-scale picture: how will this facility be 
> used in routine GDB operations by the application-level GDB code?
> 
> Also, on a i386, for example, if you don't pass the size and alignment of 
> each address to be watched, the guess of the amount of required resources 
> can be very wrong.  This is even more exacerbated due to debug register 
> sharing implemented for i386.

Yes, see my reply to the other e-mail.  I think we can do slightly 
better with the current model.  However, to make things optimal we'll 
need to totally overhaul things.

>> The intent is for the Z? packet to pass these totals down 
>> to the target.
> 
> 
> Do you mean to say this is only for remote targets?

Other architectures can use the info.  Its just that remote will be first.

Andrew