[RFA] Fix verification of changed values for big values.

[RFA] Fix verification of changed values for big values.

[Thiago Jung Bauermann]

[-- Attachment #1: Type: Text/Plain, Size: 1316 bytes --]

Hi,

This is a resubmission of a patch I posted long ago:

http://sourceware.org/ml/gdb-patches/2009-05/msg00670.html

It's good on its own, but is also important for ranged watchpoints in our
ppc476 support patches. Here's my original explanation:

	Right now, GDB calls value_equal when comparing the old and new values
	of a watchpoint. IMO this is not correct, since that function will call
	coerce_array and effectively just compare the addresses of arrays being
	watched.

	This patch introduces a new value comparison function which works in the
	mentioned case, and a testcase which fails without the patch and passes
	with it.

Tromey had approved it here:

http://sourceware.org/ml/gdb-patches/2009-06/msg00076.html

But I don't know if approvals expire or not, so...

This version addresses Tromey's comments.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center

gdb/
	* valarith.c (value_equal_contents): New function.
	* value.h (value_equal_contents): Declare.
	* breakpoint.c (watchpoint_check): Use value_equal_contents
	instead of value_equal.

gdb/testsuite/
	* gdb.base/watchpoint.exp (test_watchpoint_in_big_blob): New function.
	(top level): Call test_watchpoint_in_big_blob.
	* gdb.base/watchpoint.c (buf): Change size to value too big for hardware
	watchpoints.
	(func3): Write to buf.

[-- Attachment #2: equal-contents.diff --]
[-- Type: text/x-patch, Size: 4137 bytes --]

Index: gdb/gdb/breakpoint.c
===================================================================
--- gdb.orig/gdb/breakpoint.c	2009-12-23 17:39:55.000000000 -0200
+++ gdb/gdb/breakpoint.c	2009-12-23 17:56:27.000000000 -0200
@@ -3174,7 +3174,6 @@ watchpoints_triggered (struct target_wai
 #define BP_TEMPFLAG 1
 #define BP_HARDWAREFLAG 2
 
-/* Check watchpoint condition.  */
 
 static int
 watchpoint_check (void *p)
@@ -3245,8 +3244,12 @@ watchpoint_check (void *p)
       struct value *new_val;
 
       fetch_watchpoint_value (b->exp, &new_val, NULL, NULL);
+
+      /* We use value_equal_contents instead of value_equal because the latter
+	 coerces an array to a pointer, thus comparing just the address of the
+	 array instead of its contents.  This is not what we want.  */
       if ((b->val != NULL) != (new_val != NULL)
-	  || (b->val != NULL && !value_equal (b->val, new_val)))
+	  || (b->val != NULL && !value_equal_contents (b->val, new_val)))
 	{
 	  if (new_val != NULL)
 	    {
Index: gdb/gdb/testsuite/gdb.base/watchpoint.c
===================================================================
--- gdb.orig/gdb/testsuite/gdb.base/watchpoint.c	2009-12-23 17:39:55.000000000 -0200
+++ gdb/gdb/testsuite/gdb.base/watchpoint.c	2009-12-23 17:40:03.000000000 -0200
@@ -30,7 +30,7 @@ int ival2 = -1;
 int ival3 = -1;
 int ival4 = -1;
 int ival5 = -1;
-char buf[10];
+char buf[30];
 struct foo
 {
   int val;
@@ -95,6 +95,7 @@ func3 ()
   x = 1;				/* second x assignment */
   y = 1;
   y = 2;
+  buf[26] = 3;
 }
 
 int
Index: gdb/gdb/testsuite/gdb.base/watchpoint.exp
===================================================================
--- gdb.orig/gdb/testsuite/gdb.base/watchpoint.exp	2009-12-23 17:39:55.000000000 -0200
+++ gdb/gdb/testsuite/gdb.base/watchpoint.exp	2009-12-23 17:40:03.000000000 -0200
@@ -678,6 +678,24 @@ proc test_inaccessible_watchpoint {} {
     }
 }
     
+proc test_watchpoint_in_big_blob {} {
+    global gdb_prompt
+
+    gdb_test "watch buf" ".*atchpoint \[0-9\]+: buf"
+    send_gdb "cont\n"
+    gdb_expect {
+	-re "Continuing.*\[Ww\]atchpoint.*buf.*Old value = .*$gdb_prompt $" {
+	    pass "watchpoint on buf hit"
+	}
+	-re "Continuing.*$gdb_prompt $" {
+	    fail "watchpoint on buf hit"
+	}
+	-re ".*$gdb_prompt $" { fail "watchpoint on buf hit" ; return }
+	timeout { fail "watchpoint on buf hit (timeout)" ; return }
+	eof { fail "watchpoint on buf hit (eof)" ; return }
+    }
+}
+
 # Start with a fresh gdb.
 
 gdb_exit
@@ -842,6 +860,8 @@ if [initialize] then {
     }
 
     test_watchpoint_and_breakpoint
+
+    test_watchpoint_in_big_blob
 }
 
 # Restore old timeout
Index: gdb/gdb/valarith.c
===================================================================
--- gdb.orig/gdb/valarith.c	2009-12-23 17:44:23.000000000 -0200
+++ gdb/gdb/valarith.c	2009-12-23 17:53:15.000000000 -0200
@@ -1397,6 +1397,24 @@ value_equal (struct value *arg1, struct 
     }
 }
 
+/* Compare values based on their raw contents. Useful for arrays since
+   value_equal coerces them to pointers, thus comparing just the address
+   of the array instead of its contents.  */
+
+int
+value_equal_contents (struct value *arg1, struct value *arg2)
+{
+  struct type *type1, *type2;
+
+  type1 = check_typedef (value_type (arg1));
+  type2 = check_typedef (value_type (arg2));
+
+  return (TYPE_CODE (type1) == TYPE_CODE (type2)
+	  && TYPE_LENGTH (type1) == TYPE_LENGTH (type2)
+	  && memcmp (value_contents (arg1), value_contents (arg2),
+		     TYPE_LENGTH (type1)) == 0);
+}
+
 /* Simulate the C operator < by returning 1
    iff ARG1's contents are less than ARG2's.  */
 
Index: gdb/gdb/value.h
===================================================================
--- gdb.orig/gdb/value.h	2009-12-23 17:53:47.000000000 -0200
+++ gdb/gdb/value.h	2009-12-23 17:58:36.000000000 -0200
@@ -563,6 +563,8 @@ extern struct internalvar *lookup_intern
 
 extern int value_equal (struct value *arg1, struct value *arg2);
 
+extern int value_equal_contents (struct value *arg1, struct value *arg2);
+
 extern int value_less (struct value *arg1, struct value *arg2);
 
 extern int value_logical_not (struct value *arg1);

Re: [RFA] Fix verification of changed values for big values.

[Joel Brobecker]

> But I don't know if approvals expire or not, so...

I guess it depends whether the code that you're updating has changed
much or not. If not, I'd say that the approval does not expire.

> gdb/
> 	* valarith.c (value_equal_contents): New function.
> 	* value.h (value_equal_contents): Declare.
> 	* breakpoint.c (watchpoint_check): Use value_equal_contents
> 	instead of value_equal.

OK, with just one little request.

> 
> gdb/testsuite/
> 	* gdb.base/watchpoint.exp (test_watchpoint_in_big_blob): New function.
> 	(top level): Call test_watchpoint_in_big_blob.
> 	* gdb.base/watchpoint.c (buf): Change size to value too big for hardware
> 	watchpoints.
> 	(func3): Write to buf.

Just one comment, but pre-approved.

> -/* Check watchpoint condition.  */
>  
>  static int
>  watchpoint_check (void *p)

Can you add a short description of what the function does? We would
like all functions to be documented...  In particular, since P is
declared as a void *, it's probably going to be useful to explain
what the real type is supposed to be.

> +/* Compare values based on their raw contents. Useful for arrays since
                                                 ^ Missing space

> +    send_gdb "cont\n"
> +    gdb_expect {
> +	-re "Continuing.*\[Ww\]atchpoint.*buf.*Old value = .*$gdb_prompt $" {
> +	    pass "watchpoint on buf hit"
> +	}

I am wondering if this could be written more simply, by using gdb_test?

gdb_test "cont" "Continuing.*\[Ww\]atchpoint.*buf.*Old value = .*" [...]

? Otherwise, we try to avoid the use of send_gdb/gdb_expect, as it
forces you to handle by hand all possible failure conditions. You can
use gdb_test_multiple instead. For instance:

  | gdb_test_multiple "next" "next after watch x" {
  |     -re ".*atchpoint \[0-9\]+: x\r\n\r\nOld value = 0\r\nNew value = 1\r\n.*$gdb_prompt $" {
  |         pass "next after watch x"
  |     }
  |     -re "\[0-9\]+\[\t \]+y = 1;\r\n$gdb_prompt $" {
  |         kfail "gdb/38" "next after watch x"
  |     }

(notice how you know longer need to handle eof, timeout, but also
internal errors, etc)

-- 
Joel

Re: [RFA] Fix verification of changed values for big values.

[Thiago Jung Bauermann]

[-- Attachment #1: Type: Text/Plain, Size: 2193 bytes --]

On Thu 24 Dec 2009 02:41:25 Joel Brobecker wrote:
> > gdb/
> > 	* valarith.c (value_equal_contents): New function.
> > 	* value.h (value_equal_contents): Declare.
> > 	* breakpoint.c (watchpoint_check): Use value_equal_contents
> > 	instead of value_equal.
> 
> OK, with just one little request.

Thanks!

> > -/* Check watchpoint condition.  */
> >
> >  static int
> >  watchpoint_check (void *p)
> 
> Can you add a short description of what the function does? We would
> like all functions to be documented...  In particular, since P is
> declared as a void *, it's probably going to be useful to explain
> what the real type is supposed to be.

The purpose of the function is rather simple, so I didn't have much to say.
I added this description:

/* Evaluate watchpoint condition expression and check if its value changed.  */

> > +/* Compare values based on their raw contents. Useful for arrays since
> 
>                                                  ^ Missing space

Fixed.

> > +    send_gdb "cont\n"
> > +    gdb_expect {
> > +	-re "Continuing.*\[Ww\]atchpoint.*buf.*Old value = .*$gdb_prompt $" {
> > +	    pass "watchpoint on buf hit"
> > +	}
> 
> I am wondering if this could be written more simply, by using gdb_test?
> 
> gdb_test "cont" "Continuing.*\[Ww\]atchpoint.*buf.*Old value = .*" [...]
> 
> ? Otherwise, we try to avoid the use of send_gdb/gdb_expect, as it
> forces you to handle by hand all possible failure conditions. You can

You're right. IIRC I used gdb_expect because I wanted to explicitly check
for the gdb prompt in my regex, to avoid it being too greedy. But I changed
the testcase a little bit and now I don't need that anymore.

This is what I committed.
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center


gdb/
	* valarith.c (value_equal_contents): New function.
	* value.h (value_equal_contents): Declare.
	* breakpoint.c (watchpoint_check): Use value_equal_contents
	instead of value_equal.

gdb/testsuite/
	* gdb.base/watchpoint.exp (test_watchpoint_in_big_blob): New function.
	(top level): Call test_watchpoint_in_big_blob.
	* gdb.base/watchpoint.c (buf): Change size to value too big for hardware
	watchpoints.
	(func3): Write to buf.

[-- Attachment #2: equal-contents.diff --]
[-- Type: text/x-patch, Size: 3953 bytes --]

Index: gdb/gdb/breakpoint.c
===================================================================
--- gdb.orig/gdb/breakpoint.c	2009-12-28 10:10:40.000000000 -0200
+++ gdb/gdb/breakpoint.c	2009-12-28 11:46:36.000000000 -0200
@@ -3174,7 +3174,7 @@ watchpoints_triggered (struct target_wai
 #define BP_TEMPFLAG 1
 #define BP_HARDWAREFLAG 2
 
-/* Check watchpoint condition.  */
+/* Evaluate watchpoint condition expression and check if its value changed.  */
 
 static int
 watchpoint_check (void *p)
@@ -3245,8 +3245,12 @@ watchpoint_check (void *p)
       struct value *new_val;
 
       fetch_watchpoint_value (b->exp, &new_val, NULL, NULL);
+
+      /* We use value_equal_contents instead of value_equal because the latter
+	 coerces an array to a pointer, thus comparing just the address of the
+	 array instead of its contents.  This is not what we want.  */
       if ((b->val != NULL) != (new_val != NULL)
-	  || (b->val != NULL && !value_equal (b->val, new_val)))
+	  || (b->val != NULL && !value_equal_contents (b->val, new_val)))
 	{
 	  if (new_val != NULL)
 	    {
Index: gdb/gdb/testsuite/gdb.base/watchpoint.c
===================================================================
--- gdb.orig/gdb/testsuite/gdb.base/watchpoint.c	2009-12-28 10:10:40.000000000 -0200
+++ gdb/gdb/testsuite/gdb.base/watchpoint.c	2009-12-28 11:32:04.000000000 -0200
@@ -30,7 +30,7 @@ int ival2 = -1;
 int ival3 = -1;
 int ival4 = -1;
 int ival5 = -1;
-char buf[10];
+char buf[30] = "testtesttesttesttesttesttestte";
 struct foo
 {
   int val;
@@ -95,6 +95,7 @@ func3 ()
   x = 1;				/* second x assignment */
   y = 1;
   y = 2;
+  buf[26] = 3;
 }
 
 int
Index: gdb/gdb/testsuite/gdb.base/watchpoint.exp
===================================================================
--- gdb.orig/gdb/testsuite/gdb.base/watchpoint.exp	2009-12-28 10:10:40.000000000 -0200
+++ gdb/gdb/testsuite/gdb.base/watchpoint.exp	2009-12-28 11:46:01.000000000 -0200
@@ -678,6 +678,13 @@ proc test_inaccessible_watchpoint {} {
     }
 }
     
+proc test_watchpoint_in_big_blob {} {
+    global gdb_prompt
+
+    gdb_test "watch buf" ".*atchpoint \[0-9\]+: buf"
+    gdb_test "cont" "Continuing.*atchpoint \[0-9\]+: buf\r\n\r\nOld value = .*testte\".*" "watchpoint on buf hit"
+}
+
 # Start with a fresh gdb.
 
 gdb_exit
@@ -842,6 +849,8 @@ if [initialize] then {
     }
 
     test_watchpoint_and_breakpoint
+
+    test_watchpoint_in_big_blob
 }
 
 # Restore old timeout
Index: gdb/gdb/valarith.c
===================================================================
--- gdb.orig/gdb/valarith.c	2009-12-28 10:10:40.000000000 -0200
+++ gdb/gdb/valarith.c	2009-12-28 11:47:45.000000000 -0200
@@ -1397,6 +1397,24 @@ value_equal (struct value *arg1, struct 
     }
 }
 
+/* Compare values based on their raw contents.  Useful for arrays since
+   value_equal coerces them to pointers, thus comparing just the address
+   of the array instead of its contents.  */
+
+int
+value_equal_contents (struct value *arg1, struct value *arg2)
+{
+  struct type *type1, *type2;
+
+  type1 = check_typedef (value_type (arg1));
+  type2 = check_typedef (value_type (arg2));
+
+  return (TYPE_CODE (type1) == TYPE_CODE (type2)
+	  && TYPE_LENGTH (type1) == TYPE_LENGTH (type2)
+	  && memcmp (value_contents (arg1), value_contents (arg2),
+		     TYPE_LENGTH (type1)) == 0);
+}
+
 /* Simulate the C operator < by returning 1
    iff ARG1's contents are less than ARG2's.  */
 
Index: gdb/gdb/value.h
===================================================================
--- gdb.orig/gdb/value.h	2009-12-28 10:10:40.000000000 -0200
+++ gdb/gdb/value.h	2009-12-28 10:10:56.000000000 -0200
@@ -563,6 +563,8 @@ extern struct internalvar *lookup_intern
 
 extern int value_equal (struct value *arg1, struct value *arg2);
 
+extern int value_equal_contents (struct value *arg1, struct value *arg2);
+
 extern int value_less (struct value *arg1, struct value *arg2);
 
 extern int value_logical_not (struct value *arg1);

Re: [RFA] Fix verification of changed values for big values.

[Joel Brobecker]

> The purpose of the function is rather simple, so I didn't have much to say.
> I added this description:
> 
> /* Evaluate watchpoint condition expression and check if its value changed.  */

Thanks. I thought it was important to explain what the real type of
parameter P was so I added an extra sentence to the documentation.
Patch attached.

> You're right. IIRC I used gdb_expect because I wanted to explicitly check
> for the gdb prompt in my regex, to avoid it being too greedy.

Just for the fun of discussion (I am French, after all :-), I am
pretty sure that you can also do that with gdb_test_multiple...

-- 
Joel

Re: [RFA] Fix verification of changed values for big values.

[Joel Brobecker]

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

> Patch attached.

[trying to fight sleepiness.......]

2009-12-30  Joel Brobecker  <brobecker@adacore.com>

        * breakpoint.c (watchpoint_check): Expand the function description.

-- 
Joel

[-- Attachment #2: bp-comment.diff --]
[-- Type: text/x-diff, Size: 738 bytes --]

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.443
diff -u -p -r1.443 breakpoint.c
--- breakpoint.c	30 Dec 2009 17:33:33 -0000	1.443
+++ breakpoint.c	30 Dec 2009 19:13:44 -0000
@@ -3174,7 +3174,10 @@ watchpoints_triggered (struct target_wai
 #define BP_TEMPFLAG 1
 #define BP_HARDWAREFLAG 2
 
-/* Evaluate watchpoint condition expression and check if its value changed.  */
+/* Evaluate watchpoint condition expression and check if its value changed.
+
+   P should be a pointer to struct bpstat, but is defined as a void *
+   in order for this function to be usable with catch_errors.  */
 
 static int
 watchpoint_check (void *p)

Re: [RFA] Fix verification of changed values for big values.

[Thiago Jung Bauermann]

On Wed 30 Dec 2009 17:21:23 Joel Brobecker wrote:
> > The purpose of the function is rather simple, so I didn't have much to
> > say. I added this description:
> >
> > /* Evaluate watchpoint condition expression and check if its value
> > changed.  */
> 
> Thanks. I thought it was important to explain what the real type of
> parameter P was so I added an extra sentence to the documentation.
> Patch attached.

Sorry about that. You mentioned that concern but it slipped my mind somehow.

> > You're right. IIRC I used gdb_expect because I wanted to explicitly check
> > for the gdb prompt in my regex, to avoid it being too greedy.
> 
> Just for the fun of discussion (I am French, after all :-), I am
> pretty sure that you can also do that with gdb_test_multiple...

Nice. Thanks for the tip! It's even in the DeveloperTips page of the wiki. I 
should have known. :-)
-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center