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

[Joel Brobecker <brobecker at adacore dot com>]

> 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