Re: [rfa:breakpoint] Correctly count watchpoints

Re: [rfa:breakpoint] Correctly count watchpoints

[Eli Zaretskii]

On Mon, 30 Sep 2002, Andrew Cagney wrote:

> On the i386, one watch resource is two registers.

Why two?  Some expressions might need 3 registers.  If you use this 
worst-case scenario, GDB will think it cannot watch more than a single 
expression, and that some data types, such as double's, and complex 
aggregates, such as struct's, cannot be watched at all.  It's hardly a 
Good Thing to refuse to set watchpoints based on inaccurate decisions 
like this.

> > But this is very hard or even impossible to do in practice.  For
> > example, on a i386, if there are two watchpoint that watch the same
> > 4-byte aligned int variable, you need only one debug register to watch
> > them both, so counting each one as taking one resource is incorrect.
> 
> That is a bug.  A further change would be to accumulate all the regions 
> and eliminate any overlap from the count.

This requires a significant change in the high-level code of GDB: it 
needs to pass all the information about all the ``active'' watchpoints to 
the function that tells how many watchpoint resources are required for 
the next watchpoint.

> For an architecture to try and optimally allocate watchpoint resources, 
> I don't think (cf opencore code) a list of ADDR:LEN pairs is sufficient. 
>   Instead it should be provided with all the watchpoint expressions.

So that means an architecture should know about GDB's expression-parsing 
code.  In effect, we are going to have the arch-specific code be tightly 
coupled with arch-independent code in breakpoint.c and friends.

> For instance, the hw_resources_used_count() function in my other patch 
> could be made part of the architecture vector so that architectures, 
> such as the i386, could override the default model using some other type 
> of allocation scheme.

As I write above, overriding the default model is not enough, since the 
application-level code doesn't feed the architecture with enough info.

Re: [rfa:breakpoint] Correctly count watchpoints

[Andrew Cagney]

> On Mon, 30 Sep 2002, Andrew Cagney wrote:
> 
> 
>> On the i386, one watch resource is two registers.
> 
> 
> Why two?  Some expressions might need 3 registers.  If you use this 
> worst-case scenario, GDB will think it cannot watch more than a single 
> expression, and that some data types, such as double's, and complex 
> aggregates, such as struct's, cannot be watched at all.  It's hardly a 
> Good Thing to refuse to set watchpoints based on inaccurate decisions 
> like this.

You mentioned two :-)

Under the current arangement, an architecture has two choices:

- have target_can_use_hardware_watchpoints() always return true (most 
targets appear to do this) and then error while trying to insert the 
watchpoints.  This is what the i386 currently does.

- have target_can_use...() make use of the counts and return an 
indication based on that

>> > But this is very hard or even impossible to do in practice.  For
>> > example, on a i386, if there are two watchpoint that watch the same
>> > 4-byte aligned int variable, you need only one debug register to watch
>> > them both, so counting each one as taking one resource is incorrect.
> 
>> 
>> That is a bug.  A further change would be to accumulate all the regions 
>> and eliminate any overlap from the count.
> 
> 
> This requires a significant change in the high-level code of GDB: it 
> needs to pass all the information about all the ``active'' watchpoints to 
> the function that tells how many watchpoint resources are required for 
> the next watchpoint.

I'm not so sure.  I think this can be handled by:

- keeping a list of the addr:len pairs needed by each watchpoint (and 
their ``cost'' (hmm, that's the correct word)).

- iterating over all watchpoints, all addr:len pairs eliminating overlap

and this is local to breakpoint.c.

>> For an architecture to try and optimally allocate watchpoint resources, 
>> I don't think (cf opencore code) a list of ADDR:LEN pairs is sufficient. 
>>   Instead it should be provided with all the watchpoint expressions.
> 
> 
> So that means an architecture should know about GDB's expression-parsing 
> code.  In effect, we are going to have the arch-specific code be tightly 
> coupled with arch-independent code in breakpoint.c and friends.

The expression tree, yes.

More scary again, it's on par with a compiler where the code needs to 
assign instructions and registers to expression elements.  As I 
mentioned elsewhere I've seen it come as a feature request twice now.

>> For instance, the hw_resources_used_count() function in my other patch 
>> could be made part of the architecture vector so that architectures, 
>> such as the i386, could override the default model using some other type 
>> of allocation scheme.
> 
> 
> As I write above, overriding the default model is not enough, since the 
> application-level code doesn't feed the architecture with enough info.

Yes.

The function hw_resources_used_count() (nee hw_breakpoint_used_count()) 
is the real core to determining the number of watchpoints that are 
needed.  It has complete information.  I was thinking of allowing 
architectures to plug in a per-architecture equivalent.

Andrew

Re: [rfa:breakpoint] Correctly count watchpoints

[Eli Zaretskii]

> Date: Tue, 01 Oct 2002 01:59:43 -0400
> From: Andrew Cagney <ac131313@redhat.com>
> > 
> > Why two?  Some expressions might need 3 registers.  If you use this 
> > worst-case scenario, GDB will think it cannot watch more than a single 
> > expression, and that some data types, such as double's, and complex 
> > aggregates, such as struct's, cannot be watched at all.  It's hardly a 
> > Good Thing to refuse to set watchpoints based on inaccurate decisions 
> > like this.
> 
> You mentioned two :-)

That was an example.

> Under the current arangement, an architecture has two choices:
> 
> - have target_can_use_hardware_watchpoints() always return true (most 
> targets appear to do this) and then error while trying to insert the 
> watchpoints.  This is what the i386 currently does.
> 
> - have target_can_use...() make use of the counts and return an 
> indication based on that

The reason the first strategy is widely used is that the high-level
code of GDB makes it very hard to do anything intelligent otherwise,
especially since refusing the target_can_use_hardware_watchpoints call
means GDB won't even try to insert that watchpoint.  IIRC, GDB doesn't
even promise to call that function (macro) only once for each
watchpoint.

Re: [rfa:breakpoint] Correctly count watchpoints

[Andrew Cagney]

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

[I had an unscheduled e-mail outage]  My reply is attached.

[-- Attachment #2: Re: [rfa:breakpoint] Correctly count watchpoints.eml --]
[-- Type: message/rfc822, Size: 4036 bytes --]

From: Andrew Cagney <ac131313@redhat.com>
To: Eli Zaretskii <eliz@is.elta.co.il>
Cc: gdb-patches@sources.redhat.com
Subject: Re: [rfa:breakpoint] Correctly count watchpoints
Date: Mon, 30 Sep 2002 15:18:43 -0400
Message-ID: <3D98A393.6010801@redhat.com>

<div class="moz-text-flowed" style="font-family: -moz-fixed">>> Date: Mon, 30 Sep 2002 12:34:53 -0400
>> From: Andrew Cagney <ac131313@redhat.com>
>> 
>> Each watch 
>> element / location / value in the watchpoint expression is assumed to 
>> consume one watch resource.
> 
> 
> Given that this assumption doesn't hold on at least one very popular
> architecture, is it a useful assumption?

I think the model holds.  It just leads to an inefficient allocation of 
watchpoint resources.  On the i386, one watch resource is two registers.

>> Anyway, the problem you refer to is why I was thinking of re-defining 
>> TARGET_REGION_OK_FOR_HW_WATCHPOINT() so that it returns the number of 
>> watchpoint resources required to watch addr/len.  If {&a, sizeof a} 
>> required two registers it could return two.
> 
> 
> But this is very hard or even impossible to do in practice.  For
> example, on a i386, if there are two watchpoint that watch the same
> 4-byte aligned int variable, you need only one debug register to watch
> them both, so counting each one as taking one resource is incorrect.

That is a bug.  A further change would be to accumulate all the regions 
and eliminate any overlap from the count.  I don't know how often this 
happens in real life.

> But you cannot return the correct result unless you are presented with
> the entire list of watchpoints GDB would like to set.  Alas, GDB's
> application code examines the watchpoints one by one and queries the
> target vector about each one of them in order.  Thus, the target
> vector doesn't see the whole picture and therefore cannot give the
> right answer.

For an architecture to try and optimally allocate watchpoint resources, 
I don't think (cf opencore code) a list of ADDR:LEN pairs is sufficient. 
  Instead it should be provided with all the watchpoint expressions.

> What is the value of the result you get if we _know_ in advance that
> it will be incorrect, sometimes grossly incorrect, in some not very
> rare cases?

I think the model is sufficient for the common case - a few independant 
variables and no complex expressions.   To follow through the opencore's 
code, an extension would be to let an architecture define its own more 
complex model, overriding this default.

For instance, the hw_resources_used_count() function in my other patch 
could be made part of the architecture vector so that architectures, 
such as the i386, could override the default model using some other type 
of allocation scheme.

>> I think it would be helpful if, at least in maintainer mode, the user 
>> could see how many resources have been allocated to a watchpoint.
> 
> 
> If this is for maintainers, the count should be accurate.  The i386
> native debugging implements a maintainer-mode command to do that, but
> it manipulates target-side data, and only works after all watchpoints
> have been inserted.

True, there are several pieces of information:
- how many resources GDB thinks it is consuming
- how efficiently GDB is assigning those resources to hardware.
Sounds like the information is watchpoint model dependant.

>> (I've a sinking feeling that hardware breakpoints have the same problem 
>> ...).
> 
> 
> Indeed they do.

I'll revise the counts.

Andrew


</div>

Re: [rfa:breakpoint] Correctly count watchpoints

[Eli Zaretskii]

> Date: Mon, 30 Sep 2002 12:34:53 -0400
> From: Andrew Cagney <ac131313@redhat.com>
> 
> Each watch 
> element / location / value in the watchpoint expression is assumed to 
> consume one watch resource.

Given that this assumption doesn't hold on at least one very popular
architecture, is it a useful assumption?

> Anyway, the problem you refer to is why I was thinking of re-defining 
> TARGET_REGION_OK_FOR_HW_WATCHPOINT() so that it returns the number of 
> watchpoint resources required to watch addr/len.  If {&a, sizeof a} 
> required two registers it could return two.

But this is very hard or even impossible to do in practice.  For
example, on a i386, if there are two watchpoint that watch the same
4-byte aligned int variable, you need only one debug register to watch
them both, so counting each one as taking one resource is incorrect.
But you cannot return the correct result unless you are presented with
the entire list of watchpoints GDB would like to set.  Alas, GDB's
application code examines the watchpoints one by one and queries the
target vector about each one of them in order.  Thus, the target
vector doesn't see the whole picture and therefore cannot give the
right answer.

What is the value of the result you get if we _know_ in advance that
it will be incorrect, sometimes grossly incorrect, in some not very
rare cases?

> I think it would be helpful if, at least in maintainer mode, the user 
> could see how many resources have been allocated to a watchpoint.

If this is for maintainers, the count should be accurate.  The i386
native debugging implements a maintainer-mode command to do that, but
it manipulates target-side data, and only works after all watchpoints
have been inserted.

> (I've a sinking feeling that hardware breakpoints have the same problem 
> ...).

Indeed they do.

Re: [rfa:breakpoint] Correctly count watchpoints

[Andrew Cagney]

> On Sun, 29 Sep 2002, Andrew Cagney wrote:
> 
> 
>> (per earlier post) An expression like:
>> 
>> 	a + b
>> 
>> requires two watchpoint resources (&a and &b).
> 
> 
> What do you mean by ``watchpoint resources''?  On a i386, watching &a 
> might require much more than a single debug register, depending on a's 
> size and alignment.

At this point I'm trying desperatly to not define it :-) Each watch 
element / location / value in the watchpoint expression is assumed to 
consume one watch resource.

Anyway, the problem you refer to is why I was thinking of re-defining 
TARGET_REGION_OK_FOR_HW_WATCHPOINT() so that it returns the number of 
watchpoint resources required to watch addr/len.  If {&a, sizeof a} 
required two registers it could return two.

Such a model is obviously simple and will be sub-optimal in many 
situtations.  I think its sufficient though.

A more complete model would involve extending/generalizing what Marko 
Mlinar has implemented for the opencore:
http://sources.redhat.com/ml/gdb/2002-09/msg00308.html

>> When first creating the 
>> watchpoint, gdb correctly counts this as two.  However,  when GDB goes 
>> back to compute the number of watchpoints already used, it does a 
>> re-count and treats the above (and any watchpoint expression) as only one.
>> 
>> The attached, I belive, fixes this by saving the mem_cnt that was computed.
> 
> 
> I agree that the count should at least be consistent.
> 
> 
>> One thing I wonder about though, should ``info breakpoints'' or ``maint 
>> info breakpoints'' display this info?
> 
> 
> Only if it's useful.  Could you please make a concrete suggestion as to 
> what should be printed about this information?

I think it would be helpful if, at least in maintainer mode, the user 
could see how many resources have been allocated to a watchpoint.  That 
way (as well as letting me check its working :-) the user would be in a 
better position to figure out where, exactly, all their watchpoints have 
gone.

>> Ok?
> 
> 
> Fine with me.

(I've a sinking feeling that hardware breakpoints have the same problem 
...).

Andrew

Re: [rfa:breakpoint] Correctly count watchpoints

[Eli Zaretskii]

On Sun, 29 Sep 2002, Andrew Cagney wrote:

> (per earlier post) An expression like:
> 
> 	a + b
> 
> requires two watchpoint resources (&a and &b).

What do you mean by ``watchpoint resources''?  On a i386, watching &a 
might require much more than a single debug register, depending on a's 
size and alignment.

> When first creating the 
> watchpoint, gdb correctly counts this as two.  However,  when GDB goes 
> back to compute the number of watchpoints already used, it does a 
> re-count and treats the above (and any watchpoint expression) as only one.
> 
> The attached, I belive, fixes this by saving the mem_cnt that was computed.

I agree that the count should at least be consistent.

> One thing I wonder about though, should ``info breakpoints'' or ``maint 
> info breakpoints'' display this info?

Only if it's useful.  Could you please make a concrete suggestion as to 
what should be printed about this information?

> Ok?

Fine with me.

[rfa:breakpoint] Correctly count watchpoints

[Andrew Cagney]

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

Hello,

The attached modifies breakpoint.c so that (I think) it correctly counts 
the number of watchpoint resources in use.

(per earlier post) An expression like:

	a + b

requires two watchpoint resources (&a and &b).  When first creating the 
watchpoint, gdb correctly counts this as two.  However,  when GDB goes 
back to compute the number of watchpoints already used, it does a 
re-count and treats the above (and any watchpoint expression) as only one.

The attached, I belive, fixes this by saving the mem_cnt that was computed.

One thing I wonder about though, should ``info breakpoints'' or ``maint 
info breakpoints'' display this info?

Ok?
Andrew

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

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

	* breakpoint.c (watch_command_1): Save mem_cnt in watchpoint.
	(hw_watchpoint_used_count): Accumulate the mem_cnt's
	* breakpoint.h: Update copyright.
	(struct breakpoint): Add field mem_cnt;

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:09:42 -0000
@@ -4288,13 +4288,15 @@
   {
     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)
+	  {
+	    if (b->type == type)
+	      i += b->mem_cnt;
+	    else
+	      *other_type_used = 1;
+	  }
       }
   }
   return i;
@@ -5384,6 +5386,7 @@
   b->exp_valid_block = exp_valid_block;
   b->exp_string = savestring (exp_start, exp_end - exp_start);
   b->val = val;
+  b->mem_cnt = mem_cnt;
   b->cond = cond;
   if (cond_start)
     b->cond_string = savestring (cond_start, cond_end - cond_start);
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:11:51 -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.
 
@@ -260,6 +261,11 @@
     struct block *exp_valid_block;
     /* Value of the watchpoint the last time we checked it.  */
     struct value *val;
+
+    /* Count of memory watchpoint resources needed.  An expression can
+       refer to more than one location and hence may require more than
+       one memory watchpoint.  */
+    int mem_cnt;
 
     /* Holds the value chain for a hardware watchpoint expression.  */
     struct value *val_chain;