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

[Klee Dienes <klee@apple.com>]

[-- 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)
     {