Re: [RFA] Fix hw watchpoints in process record.

[Pedro Alves <pedro at codesourcery dot com>]

On Sunday 22 November 2009 15:44:29, Pedro Alves wrote:
> We were missing a target_stopped_data_address method in precord
...

> ????????* record.c (record_beneath_to_stopped_by_watchpoint)
> ????????(record_beneath_to_stopped_data_address, record_hw_watchpoint):
> ????????New globals.
...
> ????????(record_stopped_by_watchpoint): New.
> ????????(record_stopped_data_address): New.
> ????????(init_record_ops): Install them.
> ????????(init_record_core_ops): Ditto.

"Them", yeah right...  I actually managed to forget to
install record_stopped_data_address in the version I committed.

> @@ -1594,6 +1657,7 @@ init_record_ops (void)
> ? ?record_ops.to_xfer_partial = record_xfer_partial;
> ? ?record_ops.to_insert_breakpoint = record_insert_breakpoint;
> ? ?record_ops.to_remove_breakpoint = record_remove_breakpoint;
> + ?record_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
> ? ?record_ops.to_can_execute_reverse = record_can_execute_reverse;
> ? ?record_ops.to_stratum = record_stratum;
> ? ?/* Add bookmark target methods. ?*/
> @@ -1801,6 +1865,7 @@ init_record_core_ops (void)
> ? ?record_core_ops.to_xfer_partial = record_core_xfer_partial;
> ? ?record_core_ops.to_insert_breakpoint = record_core_insert_breakpoint;
> ? ?record_core_ops.to_remove_breakpoint = record_core_remove_breakpoint;
> + ?record_core_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
> ? ?record_core_ops.to_can_execute_reverse = record_can_execute_reverse;
> ? ?record_core_ops.to_has_execution = record_core_has_execution;
> ? ?record_core_ops.to_stratum = record_stratum;

...

I've applied the patch below to fix it.


Without this, if the target beneath supports reporting the
stopped data address, there's a case where record can miss
a watchpoint.  That is the case of stopping recording when
stopped at a watchpoint, and then continue/step backwards
until a different watchpoint triggers.  The stopp_data_address
of the target beneath is called by mistake, and that may
reports the wrong stopped_data_address, from the last time
the target really ran.  E.g., on x86_64-linux :

 >./gdb -q ./gdb
 (top-gdb) start
 Temporary breakpoint 3 at 0x454727: file ../../src/gdb/gdb.c, line 28.
 Starting program: /home/pedro/gdb/baseline/build/gdb/gdb
 [Thread debugging using libthread_db enabled]
 
 Temporary breakpoint 3, main (argc=1, argv=0x7fffffffe3a8) at ../../src/gdb/gdb.c:28
 28        memset (&args, 0, sizeof args);
 (top-gdb) record
 (top-gdb) watch args.argv
 Hardware watchpoint 4: args.argv
 (top-gdb) c
 Continuing.
 Hardware watchpoint 4: args.argv
 
 Old value = (char **) 0x0
 New value = (char **) 0x7fffffffe3a8
 main (argc=1, argv=0x7fffffffe3a8) at ../../src/gdb/gdb.c:31
 31        args.use_windows = 0;
 (top-gdb) del
 Delete all breakpoints? (y or n) y
 (top-gdb) watch args.argc
 Hardware watchpoint 5: args.argc
 (top-gdb) reverse-continue
 Continuing.

 No more reverse-execution history.
 main (argc=1, argv=0x7fffffffe3a8) at ../../src/gdb/gdb.c:28
 28        memset (&args, 0, sizeof args);
 (top-gdb)  

It should have triggered a watchpoint.  With the
patch applied, the last reverse-continue does this instead:

 (top-gdb) reverse-continue
 Continuing.
 Hardware watchpoint 5: args.argc

 Old value = 1
 New value = 0
 0x000000000045473d in main (argc=1, argv=0x7fffffffe3a8) at ../../src/gdb/gdb.c:29
 29        args.argc = argc;
 (top-gdb) 

which is correct.


This deserves a testcase, but I haven't written it yet.  Will do
(unless someone else wants to, which I'd appreciate :-) ).

-- 
Pedro Alves

2009-11-24  Pedro Alves  <pedro@codesourcery.com>

	* record.c (init_record_ops, init_record_core_ops): Actually
	install record_stopped_data_address.

---
 gdb/record.c |    2 ++
 1 file changed, 2 insertions(+)

Index: src/gdb/record.c
===================================================================
--- src.orig/gdb/record.c	2009-11-24 01:36:42.000000000 +0000
+++ src/gdb/record.c	2009-11-24 01:37:01.000000000 +0000
@@ -1668,6 +1668,7 @@ init_record_ops (void)
   record_ops.to_insert_breakpoint = record_insert_breakpoint;
   record_ops.to_remove_breakpoint = record_remove_breakpoint;
   record_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
+  record_ops.to_stopped_data_address = record_stopped_data_address;
   record_ops.to_can_execute_reverse = record_can_execute_reverse;
   record_ops.to_stratum = record_stratum;
   /* Add bookmark target methods.  */
@@ -1876,6 +1877,7 @@ init_record_core_ops (void)
   record_core_ops.to_insert_breakpoint = record_core_insert_breakpoint;
   record_core_ops.to_remove_breakpoint = record_core_remove_breakpoint;
   record_core_ops.to_stopped_by_watchpoint = record_stopped_by_watchpoint;
+  record_core_ops.to_stopped_data_address = record_stopped_data_address;
   record_core_ops.to_can_execute_reverse = record_can_execute_reverse;
   record_core_ops.to_has_execution = record_core_has_execution;
   record_core_ops.to_stratum = record_stratum;