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

Thiago Jung Bauermann
2009-12-30T17:38:59Z

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