Re: Watchpoint resource accounting broken (Re: [5/6] breakpoints_ops for all kinds of breakpoints: new watchpoints instance type)

[Pedro Alves <pedro at codesourcery dot com>]

On Monday 12 September 2011 16:03:18, Ulrich Weigand wrote:

> However, update_watchpoint relies on having the watchpoint under
> investigation be on the breakpoint list; see the comment:
> 
>               /* We need to determine how many resources are already
>                  used for all other hardware watchpoints plus this one
>                  to see if we still have enough resources to also fit
>                  this watchpoint in as well.  To guarantee the
>                  hw_watchpoint_used_count call below counts this
>                  watchpoint, make sure that it is marked as a hardware
>                  watchpoint.  */
>               if (b->base.type == bp_watchpoint)
>                 b->base.type = bp_hardware_watchpoint;
> 
>               i = hw_watchpoint_used_count (b->base.type, &other_type_used);
>               target_resources_ok = target_can_use_hardware_watchpoint
>                     (b->base.type, i, other_type_used);
> 
> Note how just "i", the result of hw_watchpoint_used_count, is passed to
> target_can_use_hardware_watchpoint -- this works only if the current
> watchpoint is on the list that hw_watchpoint_used_count iterates over.
> 
> (You cannot just consider the current watchpoint in addition to the
> result of hw_watchpoint_used_count either, because for other callers
> to update_watchpoint, the current watchpoint *is* on the list.)
> 
> I'm sure sure how best to fix this; maybe go back to adding the
> watchpoint to the breakpoint list earlier?

Does the patch below work?  Never consider the current watchpoint when
going over the breakpoint list counting resources, and then 
add the resources of the current watchpoint on top.  This way we
don't have to care of the current watchpoint being on the list yet or
not.  As bonus, we no longer have to frog the watchpoint's type before
knowing if it'll fit.

I've tested it on x86_64-linux and saw no regressions.

On Monday 12 September 2011 16:16:53, Edjunior Barbosa Machado wrote:
> On 09/12/2011 12:03 PM, Ulrich Weigand wrote:
> > Note how just "i", the result of hw_watchpoint_used_count, is passed to
> > target_can_use_hardware_watchpoint -- this works only if the current
> > watchpoint is on the list that hw_watchpoint_used_count iterates over.
> 
> I noticed this problem too and was considering use "i + reg_cnt" instead
> of only "i" when calling hw_watchpoint_used_count() (actually, I saw gdb
> used to use this previously).
> However, with this change, due to the same problem with the watchpoint
> added to breakpoint list that Ulrich mentioned, watchpoints added before
> run the inferior will not work.

Can you expand on this?  I don't think I'm understanding that problem.

-- 
Pedro Alves

2011-09-13  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* breakpoint.c (update_watchpoint): Handle the case of the
	watchpoint to update not being the in the breakpoint list yet.
	(hw_watchpoint_use_count): New, factored out from
	hw_watchpoint_used_count.
	(hw_watchpoint_used_count): Rename to ...
	(hw_watchpoint_used_count_others): ... this.  Add `except'
	parameter.  Don't count resources of `except'.  Use
	hw_watchpoint_use_count.

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

Index: src/gdb/breakpoint.c
===================================================================
--- src.orig/gdb/breakpoint.c	2011-08-29 17:36:34.000000000 +0100
+++ src/gdb/breakpoint.c	2011-09-13 13:47:49.421051499 +0100
@@ -176,7 +176,11 @@ static void maintenance_info_breakpoints
 
 static int hw_breakpoint_used_count (void);
 
-static int hw_watchpoint_used_count (enum bptype, int *);
+static int hw_watchpoint_use_count (struct breakpoint *);
+
+static int hw_watchpoint_used_count_others (struct breakpoint *except,
+					    enum bptype type,
+					    int *other_type_used);
 
 static void hbreak_command (char *, int);
 
@@ -1458,6 +1462,7 @@ update_watchpoint (struct watchpoint *b,
 	  if (reg_cnt)
 	    {
 	      int i, target_resources_ok, other_type_used;
+	      enum bptype type;
 
 	      /* Use an exact watchpoint when there's only one memory region to be
 		 watched, and only one debug register is needed to watch it.  */
@@ -1466,16 +1471,29 @@ update_watchpoint (struct watchpoint *b,
 	      /* We need to determine how many resources are already
 		 used for all other hardware watchpoints plus this one
 		 to see if we still have enough resources to also fit
-		 this watchpoint in as well.  To guarantee the
-		 hw_watchpoint_used_count call below counts this
-		 watchpoint, make sure that it is marked as a hardware
-		 watchpoint.  */
-	      if (b->base.type == bp_watchpoint)
-		b->base.type = bp_hardware_watchpoint;
-
-	      i = hw_watchpoint_used_count (b->base.type, &other_type_used);
-	      target_resources_ok = target_can_use_hardware_watchpoint
-		    (b->base.type, i, other_type_used);
+		 this watchpoint in as well.  */
+
+	      /* If this is a software watchpoint, we try to turn it
+		 to a hardware one -- count resources as if B was of
+		 hardware watchpoint type.  */
+	      type = b->base.type;
+	      if (type == bp_watchpoint)
+		type = bp_hardware_watchpoint;
+
+	      /* This watchpoint may or may not have been placed on
+		 the list yet at this point (it won't be in the list
+		 if we're trying to create it for the first time,
+		 through watch_command), so always account for it
+		 manually.  */
+
+	      /* Count resources used by all watchpoints except B.  */
+	      i = hw_watchpoint_used_count_others (&b->base, type, &other_type_used);
+
+	      /* Add in the resources needed for B.  */
+	      i += hw_watchpoint_use_count (&b->base);
+
+	      target_resources_ok
+		= target_can_use_hardware_watchpoint (type, i, other_type_used);
 	      if (target_resources_ok <= 0)
 		{
 		  int sw_mode = b->base.ops->works_in_software_mode (&b->base);
@@ -1486,8 +1504,17 @@ update_watchpoint (struct watchpoint *b,
 		  else if (target_resources_ok < 0 && !sw_mode)
 		    error (_("There are not enough available hardware "
 			     "resources for this watchpoint."));
-		  else
-		    b->base.type = bp_watchpoint;
+
+		  /* Downgrade to software watchpoint.  */
+		  b->base.type = bp_watchpoint;
+		}
+	      else
+		{
+		  /* If this was a software watchpoint, we've just
+		     found we have enough resources to turn it to a
+		     hardware watchpoint.  Otherwise, this is a
+		     nop.  */
+		  b->base.type = type;
 		}
 	    }
 	  else if (!b->base.ops->works_in_software_mode (&b->base))
@@ -6846,28 +6873,52 @@ hw_breakpoint_used_count (void)
   return i;
 }
 
+/* Returns the resources B would use if it were a hardware
+   watchpoint.  */
+
 static int
-hw_watchpoint_used_count (enum bptype type, int *other_type_used)
+hw_watchpoint_use_count (struct breakpoint *b)
 {
   int i = 0;
-  struct breakpoint *b;
   struct bp_location *bl;
 
+  if (!breakpoint_enabled (b))
+    return 0;
+
+  for (bl = b->loc; bl; bl = bl->next)
+    {
+      /* Special types of hardware watchpoints may use more than
+	 one register.  */
+      i += b->ops->resources_needed (bl);
+    }
+
+  return i;
+}
+
+/* Returns the sum the used resources of all hardware watchpoints of
+   type TYPE in the breakpoints list.  Also returns in OTHER_TYPE_USED
+   the sum of the used resources of all hardware watchpoints of other
+   types _not_ TYPE.  */
+
+static int
+hw_watchpoint_used_count_others (struct breakpoint *except,
+				 enum bptype type, int *other_type_used)
+{
+  int i = 0;
+  struct breakpoint *b;
+
   *other_type_used = 0;
   ALL_BREAKPOINTS (b)
     {
+      if (b == except)
+	continue;
       if (!breakpoint_enabled (b))
 	continue;
 
-	if (b->type == type)
-	  for (bl = b->loc; bl; bl = bl->next)
-	    {
-	      /* Special types of hardware watchpoints may use more than
-		 one register.  */
-	      i += b->ops->resources_needed (bl);
-	    }
-	else if (is_hardware_watchpoint (b))
-	  *other_type_used = 1;
+      if (b->type == type)
+	i += hw_watchpoint_use_count (b);
+      else if (is_hardware_watchpoint (b))
+	*other_type_used = 1;
     }
 
   return i;