[RFA] patch for [Bug gdb/11253]

[RFA] patch for [Bug gdb/11253]

[Hui Zhu]

This patch is for bug 11253 http://sourceware.org/bugzilla/show_bug.cgi?id=11253
Tested by AG.
Please help me review it.

Thanks,
Hui


2010-02-26  Hui Zhu  <teawater@gmail.com>

	* target.c (init_dummy_target): Add to_stopped_by_watchpoint.
---
 target.c |    1 +
 1 file changed, 1 insertion(+)

--- a/target.c
+++ b/target.c
@@ -2836,6 +2836,7 @@ init_dummy_target (void)
   dummy_target.to_has_stack = (int (*) (struct target_ops *)) return_zero;
   dummy_target.to_has_registers = (int (*) (struct target_ops *)) return_zero;
   dummy_target.to_has_execution = (int (*) (struct target_ops *)) return_zero;
+  dummy_target.to_stopped_by_watchpoint = return_zero;
   dummy_target.to_magic = OPS_MAGIC;
 }

Re: [RFA] patch for [Bug gdb/11253]

[Jan Kratochvil]

On Fri, 26 Feb 2010 04:25:58 +0100, Hui Zhu wrote:
> This patch is for bug 11253 http://sourceware.org/bugzilla/show_bug.cgi?id=11253
[..]
> --- a/target.c
> +++ b/target.c
> @@ -2836,6 +2836,7 @@ init_dummy_target (void)
>    dummy_target.to_has_stack = (int (*) (struct target_ops *)) return_zero;
>    dummy_target.to_has_registers = (int (*) (struct target_ops *)) return_zero;
>    dummy_target.to_has_execution = (int (*) (struct target_ops *)) return_zero;
> +  dummy_target.to_stopped_by_watchpoint = return_zero;
>    dummy_target.to_magic = OPS_MAGIC;
>  }

while there is already:

update_current_target (void)
...
      INHERIT (to_stopped_by_watchpoint, t);
+
  de_fault (to_stopped_by_watchpoint,
            (int (*) (void))
            return_zero);

and I wanted to propose the attached patch I expect it is because INHERITs are
deprecated now and current_target.beneath traversal is preferred nowadays.

Just posted when I wrote it, IMo the patch of Hui's is the right one instead.

No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.


Thanks,
Jan


2010-02-26  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* record.c (record_open): Initialize tmp_to_insert_breakpoint,
	tmp_to_remove_breakpoint, tmp_to_stopped_by_watchpoint and
	tmp_to_stopped_data_address directly from current_target.  Remove their
	initialization during current_target.beneath chain traversal.

--- a/gdb/record.c
+++ b/gdb/record.c
@@ -895,8 +895,10 @@ record_open (char *name, int from_tty)
   tmp_to_store_registers = NULL;
   tmp_to_xfer_partial_ops = NULL;
   tmp_to_xfer_partial = NULL;
-  tmp_to_insert_breakpoint = NULL;
-  tmp_to_remove_breakpoint = NULL;
+  tmp_to_insert_breakpoint = current_target.to_insert_breakpoint;
+  tmp_to_remove_breakpoint = current_target.to_remove_breakpoint;
+  tmp_to_stopped_by_watchpoint = current_target.to_stopped_by_watchpoint;
+  tmp_to_stopped_data_address = current_target.to_stopped_data_address;
 
   /* Set the beneath function pointers.  */
   for (t = current_target.beneath; t != NULL; t = t->beneath)
@@ -921,14 +923,6 @@ record_open (char *name, int from_tty)
 	  tmp_to_xfer_partial = t->to_xfer_partial;
 	  tmp_to_xfer_partial_ops = t;
         }
-      if (!tmp_to_insert_breakpoint)
-	tmp_to_insert_breakpoint = t->to_insert_breakpoint;
-      if (!tmp_to_remove_breakpoint)
-	tmp_to_remove_breakpoint = t->to_remove_breakpoint;
-      if (!tmp_to_stopped_by_watchpoint)
-	tmp_to_stopped_by_watchpoint = t->to_stopped_by_watchpoint;
-      if (!tmp_to_stopped_data_address)
-	tmp_to_stopped_data_address = t->to_stopped_data_address;
     }
   if (!tmp_to_xfer_partial)
     error (_("Could not find 'to_xfer_partial' method on the target stack."));

Re: [RFA] patch for [Bug gdb/11253]

[Hui Zhu]

Hi Jan,

I think your patch is more better.
Thanks.  :)

Best regards,
Hui

On Fri, Feb 26, 2010 at 20:53, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
> On Fri, 26 Feb 2010 04:25:58 +0100, Hui Zhu wrote:
>> This patch is for bug 11253 http://sourceware.org/bugzilla/show_bug.cgi?id=11253
> [..]
>> --- a/target.c
>> +++ b/target.c
>> @@ -2836,6 +2836,7 @@ init_dummy_target (void)
>>    dummy_target.to_has_stack = (int (*) (struct target_ops *)) return_zero;
>>    dummy_target.to_has_registers = (int (*) (struct target_ops *)) return_zero;
>>    dummy_target.to_has_execution = (int (*) (struct target_ops *)) return_zero;
>> +  dummy_target.to_stopped_by_watchpoint = return_zero;
>>    dummy_target.to_magic = OPS_MAGIC;
>>  }
>
> while there is already:
>
> update_current_target (void)
> ...
>      INHERIT (to_stopped_by_watchpoint, t);
> +
>  de_fault (to_stopped_by_watchpoint,
>            (int (*) (void))
>            return_zero);
>
> and I wanted to propose the attached patch I expect it is because INHERITs are
> deprecated now and current_target.beneath traversal is preferred nowadays.
>
> Just posted when I wrote it, IMo the patch of Hui's is the right one instead.
>
> No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.
>
>
> Thanks,
> Jan
>
>
> 2010-02-26  Jan Kratochvil  <jan.kratochvil@redhat.com>
>
>        * record.c (record_open): Initialize tmp_to_insert_breakpoint,
>        tmp_to_remove_breakpoint, tmp_to_stopped_by_watchpoint and
>        tmp_to_stopped_data_address directly from current_target.  Remove their
>        initialization during current_target.beneath chain traversal.
>
> --- a/gdb/record.c
> +++ b/gdb/record.c
> @@ -895,8 +895,10 @@ record_open (char *name, int from_tty)
>   tmp_to_store_registers = NULL;
>   tmp_to_xfer_partial_ops = NULL;
>   tmp_to_xfer_partial = NULL;
> -  tmp_to_insert_breakpoint = NULL;
> -  tmp_to_remove_breakpoint = NULL;
> +  tmp_to_insert_breakpoint = current_target.to_insert_breakpoint;
> +  tmp_to_remove_breakpoint = current_target.to_remove_breakpoint;
> +  tmp_to_stopped_by_watchpoint = current_target.to_stopped_by_watchpoint;
> +  tmp_to_stopped_data_address = current_target.to_stopped_data_address;
>
>   /* Set the beneath function pointers.  */
>   for (t = current_target.beneath; t != NULL; t = t->beneath)
> @@ -921,14 +923,6 @@ record_open (char *name, int from_tty)
>          tmp_to_xfer_partial = t->to_xfer_partial;
>          tmp_to_xfer_partial_ops = t;
>         }
> -      if (!tmp_to_insert_breakpoint)
> -       tmp_to_insert_breakpoint = t->to_insert_breakpoint;
> -      if (!tmp_to_remove_breakpoint)
> -       tmp_to_remove_breakpoint = t->to_remove_breakpoint;
> -      if (!tmp_to_stopped_by_watchpoint)
> -       tmp_to_stopped_by_watchpoint = t->to_stopped_by_watchpoint;
> -      if (!tmp_to_stopped_data_address)
> -       tmp_to_stopped_data_address = t->to_stopped_data_address;
>     }
>   if (!tmp_to_xfer_partial)
>     error (_("Could not find 'to_xfer_partial' method on the target stack."));
>

Re: [RFA] patch for [Bug gdb/11253]

[Jan Kratochvil]

Hi Hui,

On Sun, 28 Feb 2010 16:37:56 +0100, Hui Zhu wrote:
> I think your patch is more better.
> Thanks.  :)

I wanted to submit now a comment to target.c INHERIT is deprecated but I have
found now the notice is already there:

	   NOTE: cagney/2003-10-17: The problem with this inheritance, as it
	   is currently implemented, is that it discards any knowledge of
	   which target an inherited method originally belonged to.
	   Consequently, new new target methods should instead explicitly and
	   locally search the target stack for the target that can handle the
	   request.  */

	static void
	update_current_target (void)

which proves my patch should be dropped, I just was not sure before.


Sorry,
Jan

Re: [RFA] patch for [Bug gdb/11253]

[Jan Kratochvil]

On Fri, 26 Feb 2010 04:25:58 +0100, Hui Zhu wrote:
> This patch is for bug 11253 http://sourceware.org/bugzilla/show_bug.cgi?id=11253

just extended it a bit but it remains the same.

No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.

since
        Re: [RFA] Fix hw watchpoints in process record.
        From: Pedro Alves <pedro at codesourcery dot com>
        http://sourceware.org/ml/gdb-patches/2009-11/msg00476.html
        http://sourceware.org/ml/gdb-cvs/2009-11/msg00187.html
        commit 1ab0b876af2e2410fa462e901623669f29d75c89

there are IMO missing the resets in record_open.


Thanks,
Jan


2010-02-28  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Hui Zhu  <teawater@gmail.com>

	* record.c (record_open_1): Check tmp_to_stopped_by_watchpoint and
	tmp_to_stopped_data_address.
	(record_open): Reset tmp_to_stopped_by_watchpoint and
	tmp_to_stopped_data_address.
	* target.c (init_dummy_target): Add to_stopped_by_watchpoint and
	to_stopped_data_address.

--- a/gdb/record.c
+++ b/gdb/record.c
@@ -867,6 +867,10 @@ record_open_1 (char *name, int from_tty)
     error (_("Could not find 'to_insert_breakpoint' method on the target stack."));
   if (!tmp_to_remove_breakpoint)
     error (_("Could not find 'to_remove_breakpoint' method on the target stack."));
+  if (!tmp_to_stopped_by_watchpoint)
+    error (_("Could not find 'to_stopped_by_watchpoint' method on the target stack."));
+  if (!tmp_to_stopped_data_address)
+    error (_("Could not find 'to_stopped_data_address' method on the target stack."));
 
   push_target (&record_ops);
 }
@@ -897,6 +901,8 @@ record_open (char *name, int from_tty)
   tmp_to_xfer_partial = NULL;
   tmp_to_insert_breakpoint = NULL;
   tmp_to_remove_breakpoint = NULL;
+  tmp_to_stopped_by_watchpoint = NULL;
+  tmp_to_stopped_data_address = NULL;
 
   /* Set the beneath function pointers.  */
   for (t = current_target.beneath; t != NULL; t = t->beneath)
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2838,6 +2838,9 @@ init_dummy_target (void)
   dummy_target.to_has_stack = (int (*) (struct target_ops *)) return_zero;
   dummy_target.to_has_registers = (int (*) (struct target_ops *)) return_zero;
   dummy_target.to_has_execution = (int (*) (struct target_ops *)) return_zero;
+  dummy_target.to_stopped_by_watchpoint = return_zero;
+  dummy_target.to_stopped_data_address =
+    (int (*) (struct target_ops *, CORE_ADDR *)) return_zero;
   dummy_target.to_magic = OPS_MAGIC;
 }
 \f

Re: [RFA] patch for [Bug gdb/11253]

[Hui Zhu]

Could some people help us review this patch?

Thanks,
Hui

On Mon, Mar 1, 2010 at 02:38, Jan Kratochvil <jan.kratochvil@redhat.com> wrote:
> On Fri, 26 Feb 2010 04:25:58 +0100, Hui Zhu wrote:
>> This patch is for bug 11253 http://sourceware.org/bugzilla/show_bug.cgi?id=11253
>
> just extended it a bit but it remains the same.
>
> No regressions on {x86_64,x86_64-m32,i686}-fedora12-linux-gnu.
>
> since
>        Re: [RFA] Fix hw watchpoints in process record.
>        From: Pedro Alves <pedro at codesourcery dot com>
>        http://sourceware.org/ml/gdb-patches/2009-11/msg00476.html
>        http://sourceware.org/ml/gdb-cvs/2009-11/msg00187.html
>        commit 1ab0b876af2e2410fa462e901623669f29d75c89
>
> there are IMO missing the resets in record_open.
>
>
> Thanks,
> Jan
>
>
> 2010-02-28  Jan Kratochvil  <jan.kratochvil@redhat.com>
>            Hui Zhu  <teawater@gmail.com>
>
>        * record.c (record_open_1): Check tmp_to_stopped_by_watchpoint and
>        tmp_to_stopped_data_address.
>        (record_open): Reset tmp_to_stopped_by_watchpoint and
>        tmp_to_stopped_data_address.
>        * target.c (init_dummy_target): Add to_stopped_by_watchpoint and
>        to_stopped_data_address.
>
> --- a/gdb/record.c
> +++ b/gdb/record.c
> @@ -867,6 +867,10 @@ record_open_1 (char *name, int from_tty)
>     error (_("Could not find 'to_insert_breakpoint' method on the target stack."));
>   if (!tmp_to_remove_breakpoint)
>     error (_("Could not find 'to_remove_breakpoint' method on the target stack."));
> +  if (!tmp_to_stopped_by_watchpoint)
> +    error (_("Could not find 'to_stopped_by_watchpoint' method on the target stack."));
> +  if (!tmp_to_stopped_data_address)
> +    error (_("Could not find 'to_stopped_data_address' method on the target stack."));
>
>   push_target (&record_ops);
>  }
> @@ -897,6 +901,8 @@ record_open (char *name, int from_tty)
>   tmp_to_xfer_partial = NULL;
>   tmp_to_insert_breakpoint = NULL;
>   tmp_to_remove_breakpoint = NULL;
> +  tmp_to_stopped_by_watchpoint = NULL;
> +  tmp_to_stopped_data_address = NULL;
>
>   /* Set the beneath function pointers.  */
>   for (t = current_target.beneath; t != NULL; t = t->beneath)
> --- a/gdb/target.c
> +++ b/gdb/target.c
> @@ -2838,6 +2838,9 @@ init_dummy_target (void)
>   dummy_target.to_has_stack = (int (*) (struct target_ops *)) return_zero;
>   dummy_target.to_has_registers = (int (*) (struct target_ops *)) return_zero;
>   dummy_target.to_has_execution = (int (*) (struct target_ops *)) return_zero;
> +  dummy_target.to_stopped_by_watchpoint = return_zero;
> +  dummy_target.to_stopped_data_address =
> +    (int (*) (struct target_ops *, CORE_ADDR *)) return_zero;
>   dummy_target.to_magic = OPS_MAGIC;
>  }
>
>

Re: [RFA] patch for [Bug gdb/11253]

[Joel Brobecker]

Sorry for the delay.

> 2010-02-28  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 	    Hui Zhu  <teawater@gmail.com>
> 
> 	* record.c (record_open_1): Check tmp_to_stopped_by_watchpoint and
> 	tmp_to_stopped_data_address.
> 	(record_open): Reset tmp_to_stopped_by_watchpoint and
> 	tmp_to_stopped_data_address.
> 	* target.c (init_dummy_target): Add to_stopped_by_watchpoint and
> 	to_stopped_data_address.

This looks good to me.

Cheers,
-- 
Joel

Re: [RFA] patch for [Bug gdb/11253]

[Hui Zhu]

Checked in.
Thanks Joel and Jan.

Hui

On Mon, Mar 8, 2010 at 12:39, Joel Brobecker <brobecker@adacore.com> wrote:
> Sorry for the delay.
>
>> 2010-02-28  Jan Kratochvil  <jan.kratochvil@redhat.com>
>>           Hui Zhu  <teawater@gmail.com>
>>
>>       * record.c (record_open_1): Check tmp_to_stopped_by_watchpoint and
>>       tmp_to_stopped_data_address.
>>       (record_open): Reset tmp_to_stopped_by_watchpoint and
>>       tmp_to_stopped_data_address.
>>       * target.c (init_dummy_target): Add to_stopped_by_watchpoint and
>>       to_stopped_data_address.
>
> This looks good to me.
>
> Cheers,
> --
> Joel
>