[PATCH] Compare contents when evaluating an array watchpoint

[PATCH] Compare contents when evaluating an array watchpoint

[Klee Dienes]

The following patch allows one to set watchpoints on arrays, and have 
the watchpoint triggered if any element in the array changes.  Without 
the patch, the C value_equal semantics causes the address of the array 
to be checked for change, not the contents --- resulting in a 
watchpoint that can never be hit.

This is particularly useful if one wants to do commands like watch 
{char[80]} 0xfff0000, or similar, in order to watch an arbitrary region 
of memory.

2002-08-06  Klee Dienes  <kdienes@bluegill.localnet>

         * breakpoint.c (watchpoint_equal): New function.  Like
         value_equal, but arrays only count as "equal" if they have the
         same contents.
         (watchpoint_check): Update to use watchpoint_equal.

diff -u -r1.1.1.21 -r1.47
--- breakpoint.c	2002/09/26 20:56:41	1.1.1.21
+++ breakpoint.c	2002/10/06 09:07:23	1.47
@@ -2359,6 +2448,34 @@
    return bs;
  }

+/* Like value_equal, but two arrays are only considered equal if their
+   contents are equal. */
+
+static int
+watchpoint_equal (struct value *arg1, struct value *arg2)
+{
+  register int len;
+  register char *p1, *p2;
+
+  if ((TYPE_CODE (VALUE_TYPE (arg1)) == TYPE_CODE_ARRAY)
+      && (TYPE_CODE (VALUE_TYPE (arg2)) == TYPE_CODE_ARRAY))
+    {
+      int len = TYPE_LENGTH (VALUE_TYPE (arg1));
+      if (TYPE_LENGTH (VALUE_TYPE (arg1)) != TYPE_LENGTH (VALUE_TYPE 
(arg2)))
+	return 0;
+      p1 = VALUE_CONTENTS (arg1);
+      p2 = VALUE_CONTENTS (arg2);
+      while (--len >= 0)
+	{
+	  if (*p1++ != *p2++)
+	    break;
+	}
+      return len < 0;
+    }
+
+  return value_equal (arg1, arg2);
+}
+
  /* Possible return values for watchpoint_check (this can't be an enum
     because of check_errors).  */
  /* The watchpoint has been deleted.  */
@@ -2417,7 +2535,7 @@

        struct value *mark = value_mark ();
        struct value *new_val = evaluate_expression 
(bs->breakpoint_at->exp);
-      if (!value_equal (b->val, new_val))
+      if (!watchpoint_equal (b->val, new_val))
  	{
  	  release_value (new_val);

Re: [PATCH] Compare contents when evaluating an array watchpoint

[Eli Zaretskii]

On Sun, 6 Oct 2002, Klee Dienes wrote:

> The following patch allows one to set watchpoints on arrays, and have 
> the watchpoint triggered if any element in the array changes.  Without 
> the patch, the C value_equal semantics causes the address of the array 
> to be checked for change, not the contents --- resulting in a 
> watchpoint that can never be hit.
> 
> This is particularly useful if one wants to do commands like watch 
> {char[80]} 0xfff0000, or similar, in order to watch an arbitrary region 
> of memory.

What will this do to hardware watchpoints on arrays/array elements?  On 
many platforms, hardware watchpoints have size limitations, so large 
arrays cannot be watched in their entirety.

Re: [PATCH] Compare contents when evaluating an array watchpoint

[Klee Dienes]

There's two phases to the process by which GDB handles watchpoints,  a 
"trigger" phase, where GDB causes itself to be stopped in any 
circumstance where the watchpoint might have been changed (either by 
hardware registers, page-protections, or single-stepping), and a 
"compare" phase, where it checks the value of the data being watched to 
see if it has actually changed.  My patch doesn't change the behavior 
of the trigger phase at all --- this phase has always set the trigger 
to watch the entire contents of the array.

My patch changes only the "compare" phase, by causing it to actually 
compare the contents of the array.  The one performance implication I 
can think of is that watching large arrays could be unpleasant on 
systems that can only do breakpoints via single-stepping (whereas 
before it would have been impossible, so I'm not overly concerned).

On Monday, October 7, 2002, at 01:53 AM, Eli Zaretskii wrote:
>
> On Sun, 6 Oct 2002, Klee Dienes wrote:
>
>> The following patch allows one to set watchpoints on arrays, and have
>> the watchpoint triggered if any element in the array changes.  Without
>> the patch, the C value_equal semantics causes the address of the array
>> to be checked for change, not the contents --- resulting in a
>> watchpoint that can never be hit.
>>
>> This is particularly useful if one wants to do commands like watch
>> {char[80]} 0xfff0000, or similar, in order to watch an arbitrary 
>> region
>> of memory.
>
> What will this do to hardware watchpoints on arrays/array elements?  On
> many platforms, hardware watchpoints have size limitations, so large
> arrays cannot be watched in their entirety.

Re: [PATCH] Compare contents when evaluating an array watchpoint

[Eli Zaretskii]

On Mon, 7 Oct 2002, Klee Dienes wrote:

> My patch doesn't change the behavior 
> of the trigger phase at all --- this phase has always set the trigger 
> to watch the entire contents of the array.

IIRC, that's not true: what you call ``the trigger phase'' was watching 
the address of the array, not its contents.  To watch the contents, you 
needed to watch specific array elements.

In other words, "watch my_array" would break when the pointer to 
the first element of my_array[] changed to point to a different memory 
location.  If you want to watch array contents, you need to say
"watch my_array[0]", "watch my_array[1]", etc.

Of course, it's been a while since I looked at that code, so I might be 
mistaken.  If so, could you please tell where's the code in the current 
CVS sources which is watching the entire contents of the array?

Re: [PATCH] Compare contents when evaluating an array watchpoint

[Klee Dienes]

Sure:  breakpoint.c:984 (in insert_breakpoints), and breakpoint.c:5480 
(in can_use_hardware_watchpoint).

Here's the relevant code from breakpoint.c:984:

             /* Look at each value on the value chain.  */
             for (; v; v = v->next)
               {
                 /* If it's a memory location, and GDB actually needed
                    its contents to evaluate the expression, then we
                    must watch it.  */
                 if (VALUE_LVAL (v) == lval_memory
                     && ! VALUE_LAZY (v))
                   {
                     struct type *vtype = check_typedef (VALUE_TYPE (v));

                     /* We only watch structs and arrays if user asked
                        for it explicitly, never if they just happen to
                        appear in the middle of some value chain.  */
                     if (v == b->val_chain
                         || (TYPE_CODE (vtype) != TYPE_CODE_STRUCT
                             && TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
                       {
                         CORE_ADDR addr;
                         int len, type;

                         addr = VALUE_ADDRESS (v) + VALUE_OFFSET (v);
                         len = TYPE_LENGTH (VALUE_TYPE (v));
                         type = hw_write;
                         if (b->type == bp_read_watchpoint)
                           type = hw_read;
                         else if (b->type == bp_access_watchpoint)
                           type = hw_access;

                         val = target_insert_watchpoint (addr, len, 
type);

On Monday, October 7, 2002, at 10:26 AM, Eli Zaretskii wrote:

>
> On Mon, 7 Oct 2002, Klee Dienes wrote:
>
>> My patch doesn't change the behavior
>> of the trigger phase at all --- this phase has always set the trigger
>> to watch the entire contents of the array.
>
> IIRC, that's not true: what you call ``the trigger phase'' was watching
> the address of the array, not its contents.  To watch the contents, you
> needed to watch specific array elements.
>
> In other words, "watch my_array" would break when the pointer to
> the first element of my_array[] changed to point to a different memory
> location.  If you want to watch array contents, you need to say
> "watch my_array[0]", "watch my_array[1]", etc.
>
> Of course, it's been a while since I looked at that code, so I might be
> mistaken.  If so, could you please tell where's the code in the current
> CVS sources which is watching the entire contents of the array?

Re: [PATCH] Compare contents when evaluating an array watchpoint

[Eli Zaretskii]

> Date: Mon, 7 Oct 2002 14:50:20 -0400
> From: Klee Dienes <klee@apple.com>
> 
> Sure:  breakpoint.c:984 (in insert_breakpoints), and breakpoint.c:5480 
> (in can_use_hardware_watchpoint).

Yes, thanks.  I was indeed mistaken, sorry.

Re: [PATCH] Compare contents when evaluating an array watchpoint

[Andrew Cagney]

[suggest RFA rather than PATCH]

>> There's two phases to the process by which GDB handles watchpoints,  a "trigger" phase, where GDB causes itself to be stopped in any circumstance where the watchpoint might have been changed (either by hardware registers, page-protections, or single-stepping), and a "compare" phase, where it checks the value of the data being watched to see if it has actually changed.  My patch doesn't change the behavior of the trigger phase at all --- this phase has always set the trigger to watch the entire contents of the array.
>> 
>> My patch changes only the "compare" phase, by causing it to actually compare the contents of the array.  The one performance implication I can think of is that watching large arrays could be unpleasant on systems that can only do breakpoints via single-stepping (whereas before it would have been impossible, so I'm not overly concerned).

(Like Eli, I'm puzzled by the existing behavior :-)

Just to get this straight.  given:
	int a[10];
then:
	(gdb) watch a
sets up the hardware to look for a change in ``*a@10'' but then 
evaluates ``a'' and hence, while stopping when ever `a' changes, never 
trigger the watchpoint?

Would it be better to make it possible for the user to clearly 
differentiate between these two cases and specify any of:

	int a[10];
	int *b;
	(gdb) watch a
	(gdb) watch b
	(gdb) watch *b@10
	(gdb) watch *a@sizeof(a)

While the existing ``watch a'' might have annoying semantics, it would 
make its behavior consistent with C.  An array is converted to a pointer 
in an expression.  I'm not sure how well this would work with the 
expression evaluator though.

What ever the outcome, this desperatly needs a testcase.  Otherwize 
we're all going to keep spinning our weels wondering what the behavior 
was ment to be.

Andrew




> 
> On Monday, October 7, 2002, at 01:53 AM, Eli Zaretskii wrote:
> 
> On Sun, 6 Oct 2002, Klee Dienes wrote:
> 
>> The following patch allows one to set watchpoints on arrays, and have
>> the watchpoint triggered if any element in the array changes.  Without
>> the patch, the C value_equal semantics causes the address of the array
>> to be checked for change, not the contents --- resulting in a
>> watchpoint that can never be hit.
>> 
>> This is particularly useful if one wants to do commands like watch
>> {char[80]} 0xfff0000, or similar, in order to watch an arbitrary region
>> of memory.
>> 
>> What will this do to hardware watchpoints on arrays/array elements?  On
>> many platforms, hardware watchpoints have size limitations, so large
>> arrays cannot be watched in their entirety.
>> 


> 

Re: [RFA] Compare contents when evaluating an array watchpoint

[Klee Dienes]

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

On Monday, October 21, 2002, at 08:44 PM, Andrew Cagney wrote:

> (Like Eli, I'm puzzled by the existing behavior :-)
>
> Just to get this straight.  given:
> 	int a[10];
> then:
> 	(gdb) watch a
> sets up the hardware to look for a change in ``*a@10'' but then 
> evaluates ``a'' and hence, while stopping when ever `a' changes, never 
> trigger the watchpoint?

That's exactly correct.  The watchpoint code goes through all of the 
values referenced in evaluating an expression, and sets an OS-level 
watchpoint on each.  Since 'a' evaluates to a value with type ARRAY, 
the watchpoint code sets a hardware-assisted watchpoint on the full 
contents of that array.

> Would it be better to make it possible for the user to clearly 
> differentiate between these two cases and specify any of:
>
> 	int a[10];
> 	int *b;
> 	(gdb) watch a
> 	(gdb) watch b
> 	(gdb) watch *b@10
> 	(gdb) watch *a@sizeof(a)
>
> While the existing ``watch a'' might have annoying semantics, it would 
> make its behavior consistent with C.  An array is converted to a 
> pointer in an expression.  I'm not sure how well this would work with 
> the expression evaluator though.

The expression evaluator is perfectly happy to handle *b@10 in a 
watchpoint.  I think that the real issue is that the current mechanism 
to determine if a watchpoint has changed is:

	if (old_expr != new_expr) { print watchpoint; }

, which is fine, but IMO not overly intuitive, and would lead to the 
same problem of *b@10 not evaluating as having changed unless we were 
to special-case it somehow separately from 'watch a' (since both *b@10 
and 'a' evaluate to having the exact same type, namely ARRAY of INT).

The semantics I'm proposing are:

	if the value of 'old_expr' is different from the value of 'new_expr'.

, which I think is in general a lot less surprising to the user (if the 
output of 'print a' is different, how can we say that 'a' has not 
changed?), as well as simpler in the long term. If we're using the == 
semantics, do we call operator == for objects?  Easier I think just to 
say "we mark the watchpoint as changed if anything about the evaluated 
value has changed.".

Actually, I think there's a good argument to make watchpoint_equal use 
the code that I currently have special-cased for arrays for all the 
cases, and not use value_equal at all.  We don't have to worry about 
type conversions, since we're evaluating the same expression at two 
different points in time.

> What ever the outcome, this desperatly needs a testcase.  Otherwize 
> we're all going to keep spinning our weels wondering what the behavior 
> was ment to be.

Agreed.  I wrote this test with the assumption of "arrays compare as 
their contents in watchpoints", but I'm happy to modify the test if we 
decide on different semantics.


[-- Attachment #2: watchpoint-tests.txt --]
[-- Type: text/plain, Size: 5201 bytes --]

Index: watchpoint.exp
===================================================================
RCS file: /cvs/Darwin/src/live/cygnus/src/gdb/testsuite/gdb.base/watchpoint.exp,v
retrieving revision 1.4
diff -u -r1.4 watchpoint.exp
--- watchpoint.exp	2002/08/13 20:06:16	1.4
+++ watchpoint.exp	2002/11/05 02:05:59
@@ -56,6 +56,7 @@
 #	1		Breakpoint	marker1()
 #	2		Breakpoint	marker2()
 #	3		Watchpoint	ival3
+#	4		Watchpoint	buf
 
 proc initialize {} {
     global gdb_prompt
@@ -105,9 +106,25 @@
         }
     }
 
+    send_gdb "watch buf\n"
+    gdb_expect {
+        -re ".*\[Ww\]atchpoint 4: buf.*$gdb_prompt $" {
+            pass "set watchpoint on buf"
+        }
+        -re "warning: can't do that without a running program; try \"break main\", \"run\" first.*$gdb_prompt $" {
+            pass "set watchpoint on buf"
+	    set wp_set 0
+            return 1
+        }
+        timeout {
+            fail "(timeout) set watchpoint on buf"
+            return 0
+        }
+    }
+
     # "info watch" is the same as "info break"
 
-    if [gdb_test "info watch" "1\[ \]*breakpoint.*marker1.*\r\n2\[ \]*breakpoint.*marker2.*\r\n3\[ \]*.*watchpoint.*ival3" "watchpoint found in watchpoint/breakpoint table" ] { 
+    if [gdb_test "info watch" "1\[ \]*breakpoint.*marker1.*\r\n2\[ \]*breakpoint.*marker2.*\r\n3\[ \]*.*watchpoint.*ival3.*\r\n4\[ \]*.*watchpoint.*buf" "watchpoint found in watchpoint/breakpoint table" ] { 
       return 0; 
     }
 
@@ -119,6 +136,9 @@
     if [gdb_test "disable 3" "disable 3\[\r\n\]+" "disable watchpoint" ] { 
       return 0; 
     }
+    if [gdb_test "disable 4" "disable 4\[\r\n\]+" "disable watchpoint" ] { 
+      return 0; 
+    }
 
 
     return 1
@@ -140,6 +160,9 @@
         if [gdb_test "disable 3" "^disable 3\[\r\n\]+" "disable watchpoint in test_simple_watchpoint" ] { 
 	    return 0; 
 	}
+        if [gdb_test "disable 4" "^disable 4\[\r\n\]+" "disable watchpoint in test_simple_watchpoint" ] { 
+	    return 0; 
+	}
     }
 
 
@@ -176,14 +199,21 @@
 	    -re ".*$gdb_prompt $" { fail "set watchpoint on ival3"  }
 	    timeout { fail "set watchpoint on ival3 (timeout)"  }
 	}
-
+	send_gdb "watch buf\n"
+	gdb_expect {
+	    -re ".*\[Ww\]atchpoint 4: buf\r\n$gdb_prompt $" { 
+	        pass "set watchpoint on buf"
+	    }
+	    -re ".*$gdb_prompt $" { fail "set watchpoint on buf"  }
+	    timeout { fail "set watchpoint on buf (timeout)"  }
+	}
         set wp_set 1
 
 	# "info watch" is the same as "info break"
 
 	send_gdb "info watch\n"
 	gdb_expect {
-	    -re "1\[ \]*breakpoint.*marker1.*\r\n2\[ \]*breakpoint.*marker2.*\r\n3\[ \]*.*watchpoint.*ival3\r\n$gdb_prompt $" {
+	    -re "1\[ \]*breakpoint.*marker1.*\r\n2\[ \]*breakpoint.*marker2.*\r\n3\[ \]*.*watchpoint.*ival3\r\n4\[ \]*.*watchpoint.*buf\r\n$gdb_prompt $" {
 	        pass "watchpoint found in watchpoint/breakpoint table"
 	    }
 	    -re ".*$gdb_prompt $" {
@@ -204,13 +234,22 @@
 	    -re ".*$gdb_prompt $" { fail "disable watchpoint"  }
 	    timeout { fail "disable watchpoint (timeout)"  }
 	}
+	send_gdb "disable 4\n"
+	gdb_expect {
+	    -re "disable 4\[\r\n\]+$gdb_prompt $" { pass "disable watchpoint" }
+	    -re ".*$gdb_prompt $" { fail "disable watchpoint"  }
+	    timeout { fail "disable watchpoint (timeout)"  }
+	}
     }
 
     # After reaching the marker function, enable the watchpoint.
 
     if [gdb_test "enable 3" "^enable 3\[\r\n\]+" "enable watchpoint" ] { 
-      return ; 
+      return; 
     }
+    if [gdb_test "enable 4" "^enable 4\[\r\n\]+" "enable watchpoint" ] { 
+      return;
+    }
 
 
     gdb_test "break func1" "Breakpoint.*at.*"
@@ -270,6 +309,10 @@
     # Check that the hit count is reported correctly
     gdb_test "info break" ".*watchpoint\[ \t\]+keep\[ \t\]+y\[ \t\]+ival3\r\n\[ \t]+breakpoint already hit 5 times.*" "Watchpoint hit count is 5"
 
+    # Check watch of an array variable
+    # Don't match the entire array, because of the bizarre omit-final-null behavior.
+    gdb_test "cont" "Continuing.*\[Ww\]atchpoint.*buf.*Old value = \"\\\\000\\\\000\\\\000\\\\000\\\\000\\\\000\\\\000\\\\000\\\\000.*New value = \"\\\\000\\\\000\\\\000\\\\000\\\\000\\\\000\\\\000\\\\004.*" "watchpoint hit, buf"
+
     # Continue until we hit the finishing marker function.
     # Make sure we hit no more watchpoints.
 
@@ -279,7 +322,10 @@
     # Disable the watchpoint so we run at full speed until we exit.
 
     if [gdb_test "disable 3" "^disable 3\[\r\n\]+" "watchpoint disabled" ] { 
-      return ; 
+      return;
+    }
+    if [gdb_test "disable 4" "^disable 4\[\r\n\]+" "watchpoint disabled" ] { 
+      return;
     }
 
 
Index: watchpoint.c
===================================================================
RCS file: /cvs/Darwin/src/live/cygnus/src/gdb/testsuite/gdb.base/watchpoint.c,v
retrieving revision 1.2
diff -u -r1.2 watchpoint.c
--- watchpoint.c	2002/04/08 09:48:03	1.2
+++ watchpoint.c	2002/11/05 02:06:00
@@ -30,7 +30,7 @@
 int ival3 = -1;
 int ival4 = -1;
 int ival5 = -1;
-char buf[10];
+char buf[10] = { 0 };
 struct foo
 {
   int val;
@@ -117,6 +117,7 @@
   ival1 = count; /* Outside loop */
   ival2 = count;
   ival3 = count; ival4 = count;
+  buf[7] = 4;
   marker2 ();
   if (doread)
     {

Re: [PATCH] Compare contents when evaluating an array watchpoint

[Jim Blandy]

Andrew Cagney <ac131313@redhat.com> writes:
> 	int a[10];
> 	int *b;
> 	(gdb) watch a
> 	(gdb) watch b
> 	(gdb) watch *b@10
> 	(gdb) watch *a@sizeof(a)
> 
> While the existing ``watch a'' might have annoying semantics, it would
> make its behavior consistent with C.  An array is converted to a
> pointer in an expression.

Well, whether arrays are coerced to pointers depends on what you're
doing with them: see ISO C 6.3.2.  In most cases they are, but if
they're an operand to & or sizeof, then they aren't.  It's a
case-by-case thing: C chooses the most useful interpretation given the
context.  It seems consistent with C for GDB to do the same.

And in fact, GDB doesn't do "the usual unary conversions" on the final
value of an expression.  That's why when you have:

    int a[10];

then GDB does this:

    (gdb) print a
    $1 = {1, 0, 1074017445, 1075174868, 1075159808, 1073833280, 1075175900, 
      1074103810, 1073933620, 1075174868}
    (gdb) 

and not:

    (gdb) print a
    $1 = (int *) 0xbfffc100
    (gdb)

So I think Klee's patch makes GDB's behavior more consistent with
itself, more useful, and is still completely consistent with C
semantics.

Re: [PATCH] Compare contents when evaluating an array watchpoint

[Andrew Cagney]

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

Klee,

Looking through this thread, the technical issues appear to have been 
resolved.  You should probably ping the maintainers.

Andrew

[-- Attachment #2: mailbox-message://ac131313@movemail/fsf/gdb/patches#2273668 --]
[-- Type: message/rfc822, Size: 5063 bytes --]

From: Klee Dienes <klee@apple.com>
To: gdb-patches@sources.redhat.com
Subject: [PATCH] Compare contents when evaluating an array watchpoint
Date: Sun, 6 Oct 2002 05:16:39 -0400
Message-ID: <51EACEDC-D90C-11D6-9330-00039396EEB8@apple.com>

The following patch allows one to set watchpoints on arrays, and have 
the watchpoint triggered if any element in the array changes.  Without 
the patch, the C value_equal semantics causes the address of the array 
to be checked for change, not the contents --- resulting in a 
watchpoint that can never be hit.

This is particularly useful if one wants to do commands like watch 
{char[80]} 0xfff0000, or similar, in order to watch an arbitrary region 
of memory.

2002-08-06  Klee Dienes  <kdienes@bluegill.localnet>

         * breakpoint.c (watchpoint_equal): New function.  Like
         value_equal, but arrays only count as "equal" if they have the
         same contents.
         (watchpoint_check): Update to use watchpoint_equal.

diff -u -r1.1.1.21 -r1.47
--- breakpoint.c	2002/09/26 20:56:41	1.1.1.21
+++ breakpoint.c	2002/10/06 09:07:23	1.47
@@ -2359,6 +2448,34 @@
    return bs;
  }

+/* Like value_equal, but two arrays are only considered equal if their
+   contents are equal. */
+
+static int
+watchpoint_equal (struct value *arg1, struct value *arg2)
+{
+  register int len;
+  register char *p1, *p2;
+
+  if ((TYPE_CODE (VALUE_TYPE (arg1)) == TYPE_CODE_ARRAY)
+      && (TYPE_CODE (VALUE_TYPE (arg2)) == TYPE_CODE_ARRAY))
+    {
+      int len = TYPE_LENGTH (VALUE_TYPE (arg1));
+      if (TYPE_LENGTH (VALUE_TYPE (arg1)) != TYPE_LENGTH (VALUE_TYPE 
(arg2)))
+	return 0;
+      p1 = VALUE_CONTENTS (arg1);
+      p2 = VALUE_CONTENTS (arg2);
+      while (--len >= 0)
+	{
+	  if (*p1++ != *p2++)
+	    break;
+	}
+      return len < 0;
+    }
+
+  return value_equal (arg1, arg2);
+}
+
  /* Possible return values for watchpoint_check (this can't be an enum
     because of check_errors).  */
  /* The watchpoint has been deleted.  */
@@ -2417,7 +2535,7 @@

        struct value *mark = value_mark ();
        struct value *new_val = evaluate_expression 
(bs->breakpoint_at->exp);
-      if (!value_equal (b->val, new_val))
+      if (!watchpoint_equal (b->val, new_val))
  	{
  	  release_value (new_val);