[RFA] target adjust pc after break with itself

[RFA] target adjust pc after break with itself

[teawater]

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

Hi Pedro,

According to your idea, I write a patch
"target_adjust_pc_with_itself.txt" for it. It's for the main trunk and
20080930 branch.

2008-11-02  Hui Zhu  <teawater@gmail.com>

	* target.h (target_ops): Add "to_adjust_pc_with_itself".
	Return true if target adjust pc after break with itself.
	(target_adjust_pc_with_itself): New macro.
	Call "to_adjust_pc_with_itself".
	* target.c (update_current_target): Set
	"to_adjust_pc_with_itself".
	* infrun.c (adjust_pc_after_break): If
	"target_adjust_pc_with_itself" return true, not adjust pc.

To make P record support it. I make another patch
"record_adjust_pc_with_itself.txt".

2008-11-02  Hui Zhu  <teawater@gmail.com>

	* record.c (record_adjust_pc_with_itself): New function.
	Return true.
	(init_record_ops): Set to_adjust_pc_with_itself point to
	record_adjust_pc_with_itself.

Thanks,
Hui

[-- Attachment #2: target_adjust_pc_with_itself.txt --]
[-- Type: text/plain, Size: 1692 bytes --]

--- a/target.h
+++ b/target.h
@@ -536,6 +536,9 @@ struct target_ops
     /* Can target execute in reverse?  */
     int (*to_can_execute_reverse) ();
 
+    /* Can target adjust pc after break with itself?  */
+    int (*to_adjust_pc_with_itself) (ptid_t ptid);
+
     /* Does this target support debugging multiple processes
        simultaneously?  */
     int (*to_supports_multi_process) (void);
@@ -1155,6 +1158,11 @@ extern int target_stopped_data_address_p
      (current_target.to_can_execute_reverse ? \
       current_target.to_can_execute_reverse () : 0)
 
+/* Can target adjust pc after break with itself?  */
+#define target_adjust_pc_with_itself(ptid) \
+     (current_target.to_adjust_pc_with_itself ? \
+      current_target.to_adjust_pc_with_itself (ptid) : 0)
+
 extern const struct target_desc *target_read_description (struct target_ops *);
 
 #define target_get_ada_task_ptid(lwp, tid) \
--- a/target.c
+++ b/target.c
@@ -468,6 +468,7 @@ update_current_target (void)
       INHERIT (to_make_corefile_notes, t);
       INHERIT (to_get_thread_local_address, t);
       INHERIT (to_can_execute_reverse, t);
+      INHERIT (to_adjust_pc_with_itself, t);
       /* Do not inherit to_read_description.  */
       INHERIT (to_get_ada_task_ptid, t);
       /* Do not inherit to_search_memory.  */
--- a/infrun.c
+++ b/infrun.c
@@ -1944,6 +1944,9 @@ adjust_pc_after_break (struct execution_
      target with both of these set in GDB history, and it seems unlikely to be
      correct, so gdbarch_have_nonsteppable_watchpoint is not checked here.  */
 
+  if (target_adjust_pc_with_itself (ecs->ptid))
+    return;
+
   if (ecs->ws.kind != TARGET_WAITKIND_STOPPED)
     return;
 

[-- Attachment #3: record_adjust_pc_with_itself.txt --]
[-- Type: text/plain, Size: 609 bytes --]

--- a/record.c
+++ b/record.c
@@ -954,6 +954,12 @@ record_can_execute_reverse (void)
   return 1;
 }
 
+static int
+record_adjust_pc_with_itself (ptid_t ptid)
+{
+  return 1;
+}
+
 static void
 init_record_ops (void)
 {
@@ -975,6 +981,7 @@ init_record_ops (void)
   record_ops.to_insert_breakpoint = record_insert_breakpoint;
   record_ops.to_remove_breakpoint = record_remove_breakpoint;
   record_ops.to_can_execute_reverse = record_can_execute_reverse;
+  record_ops.to_adjust_pc_with_itself = record_adjust_pc_with_itself;
   record_ops.to_stratum = record_stratum;
   record_ops.to_magic = OPS_MAGIC;
 }

Re: [RFA] target adjust pc after break with itself

[teawater]

Hi Pedro,

Could you please help me review it?

http://sourceware.org/ml/gdb-patches/2008-11/msg00004.html

Thanks,
Hui

On Sun, Nov 2, 2008 at 14:01, teawater <teawater@gmail.com> wrote:
> Hi Pedro,
>
> According to your idea, I write a patch
> "target_adjust_pc_with_itself.txt" for it. It's for the main trunk and
> 20080930 branch.
>
> 2008-11-02  Hui Zhu  <teawater@gmail.com>
>
>        * target.h (target_ops): Add "to_adjust_pc_with_itself".
>        Return true if target adjust pc after break with itself.
>        (target_adjust_pc_with_itself): New macro.
>        Call "to_adjust_pc_with_itself".
>        * target.c (update_current_target): Set
>        "to_adjust_pc_with_itself".
>        * infrun.c (adjust_pc_after_break): If
>        "target_adjust_pc_with_itself" return true, not adjust pc.
>
> To make P record support it. I make another patch
> "record_adjust_pc_with_itself.txt".
>
> 2008-11-02  Hui Zhu  <teawater@gmail.com>
>
>        * record.c (record_adjust_pc_with_itself): New function.
>        Return true.
>        (init_record_ops): Set to_adjust_pc_with_itself point to
>        record_adjust_pc_with_itself.
>
> Thanks,
> Hui
>

Re: [RFA] target adjust pc after break with itself

[Pedro Alves]

Hi,

On Sunday 02 November 2008 06:01:29, teawater wrote:
> Hi Pedro,
> 
> According to your idea, I write a patch
> "target_adjust_pc_with_itself.txt" for it. It's for the main trunk and
> 20080930 branch.
> 

Could we place this in the branch only for a while and let it cook
there?  There's nothing in mainline that would require this for
now, unless Michael wants to do something similar for his remote target.

Your patch will do for target record, though there are several other
places that check gdbarch_decr_pc_after_break () outside of infrun
that should be adjusted --- but no other target cares currently.
Maybe we should centralize this a bit more, so a single call would
be needed everywhere else instead
of (if !target_adjusts_pc_... && gdbarch_decr_pc...))

I'm always thinking about target remote when I add new target_ops
interfaces, because that's the case where the decoupling is as high
as it comes; with that in mind, I don't think that there'll be cases
were the gdbarch does decr_pc_after_break == 0, while the target will
want to override it to != 0; this interface is probably fine.

I'm not a native speaker as well, but "with itself" doesn't
sound right to me.  How about something like:

 target_adjusts_pc_after_break ()

 if (target_adjusts_pc_after_break ())
   return;  /* ... then we don't have to.  */

or

  target_adjusts_pc_after_break_itself

?

> 2008-11-02  Hui Zhu  <teawater@gmail.com>
> 
> 	* target.h (target_ops): Add "to_adjust_pc_with_itself".
> 	Return true if target adjust pc after break with itself.
> 	(target_adjust_pc_with_itself): New macro.
> 	Call "to_adjust_pc_with_itself".
> 	* target.c (update_current_target): Set
> 	"to_adjust_pc_with_itself".
> 	* infrun.c (adjust_pc_after_break): If
> 	"target_adjust_pc_with_itself" return true, not adjust pc.
> 
> To make P record support it. I make another patch
> "record_adjust_pc_with_itself.txt".
> 
> 2008-11-02  Hui Zhu  <teawater@gmail.com>
> 
> 	* record.c (record_adjust_pc_with_itself): New function.
> 	Return true.
> 	(init_record_ops): Set to_adjust_pc_with_itself point to
> 	record_adjust_pc_with_itself.

-- 
Pedro Alves

Re: [RFA] target adjust pc after break with itself

[teawater]

Hi,

On Wed, Nov 5, 2008 at 02:32, Pedro Alves <pedro@codesourcery.com> wrote:
>
> Hi,
>
> On Sunday 02 November 2008 06:01:29, teawater wrote:
> > Hi Pedro,
> >
> > According to your idea, I write a patch
> > "target_adjust_pc_with_itself.txt" for it. It's for the main trunk and
> > 20080930 branch.
> >
>
> Could we place this in the branch only for a while and let it cook
> there?  There's nothing in mainline that would require this for
> now, unless Michael wants to do something similar for his remote target.
>

OK.

> Your patch will do for target record, though there are several other
> places that check gdbarch_decr_pc_after_break () outside of infrun
> that should be adjusted --- but no other target cares currently.
> Maybe we should centralize this a bit more, so a single call would
> be needed everywhere else instead
> of (if !target_adjusts_pc_... && gdbarch_decr_pc...))
>

Sorry for my careless. I will fix it in the next patch.

> I'm always thinking about target remote when I add new target_ops
> interfaces, because that's the case where the decoupling is as high
> as it comes; with that in mind, I don't think that there'll be cases
> were the gdbarch does decr_pc_after_break == 0, while the target will
> want to override it to != 0; this interface is probably fine.
>

Cool. I will add a interface for it too. It will called
"target_decr_pc_after_break". If "target_adjusts_pc_after_break" is
true, GDB will use it instead "gdbarch_decr_pc_after_break".

> I'm not a native speaker as well, but "with itself" doesn't
> sound right to me.  How about something like:
>
>  target_adjusts_pc_after_break ()
>
>  if (target_adjusts_pc_after_break ())
>   return;  /* ... then we don't have to.  */
>
> or
>
>  target_adjusts_pc_after_break_itself
>
> ?

I will change it to "target_adjusts_pc_after_break".


Thanks for your advices. I will make a new patch for it.

Thanks,
Hui


>
> > 2008-11-02  Hui Zhu  <teawater@gmail.com>
> >
> >       * target.h (target_ops): Add "to_adjust_pc_with_itself".
> >       Return true if target adjust pc after break with itself.
> >       (target_adjust_pc_with_itself): New macro.
> >       Call "to_adjust_pc_with_itself".
> >       * target.c (update_current_target): Set
> >       "to_adjust_pc_with_itself".
> >       * infrun.c (adjust_pc_after_break): If
> >       "target_adjust_pc_with_itself" return true, not adjust pc.
> >
> > To make P record support it. I make another patch
> > "record_adjust_pc_with_itself.txt".
> >
> > 2008-11-02  Hui Zhu  <teawater@gmail.com>
> >
> >       * record.c (record_adjust_pc_with_itself): New function.
> >       Return true.
> >       (init_record_ops): Set to_adjust_pc_with_itself point to
> >       record_adjust_pc_with_itself.
>
> --
> Pedro Alves

Re: [RFA] target adjust pc after break with itself

[teawater]

Hi Pedro,

I think it again.

There are not other target affect by this issue. And P record have a
way to deal with it inside.
So maybe fix it inside P record is better than add interface to
target_ops. Maybe we can  add interface to target_ops later.

Thanks,
Hui

On Wed, Nov 5, 2008 at 09:38, teawater <teawater@gmail.com> wrote:
> Hi,
>
> On Wed, Nov 5, 2008 at 02:32, Pedro Alves <pedro@codesourcery.com> wrote:
>>
>> Hi,
>>
>> On Sunday 02 November 2008 06:01:29, teawater wrote:
>> > Hi Pedro,
>> >
>> > According to your idea, I write a patch
>> > "target_adjust_pc_with_itself.txt" for it. It's for the main trunk and
>> > 20080930 branch.
>> >
>>
>> Could we place this in the branch only for a while and let it cook
>> there?  There's nothing in mainline that would require this for
>> now, unless Michael wants to do something similar for his remote target.
>>
>
> OK.
>
>> Your patch will do for target record, though there are several other
>> places that check gdbarch_decr_pc_after_break () outside of infrun
>> that should be adjusted --- but no other target cares currently.
>> Maybe we should centralize this a bit more, so a single call would
>> be needed everywhere else instead
>> of (if !target_adjusts_pc_... && gdbarch_decr_pc...))
>>
>
> Sorry for my careless. I will fix it in the next patch.
>
>> I'm always thinking about target remote when I add new target_ops
>> interfaces, because that's the case where the decoupling is as high
>> as it comes; with that in mind, I don't think that there'll be cases
>> were the gdbarch does decr_pc_after_break == 0, while the target will
>> want to override it to != 0; this interface is probably fine.
>>
>
> Cool. I will add a interface for it too. It will called
> "target_decr_pc_after_break". If "target_adjusts_pc_after_break" is
> true, GDB will use it instead "gdbarch_decr_pc_after_break".
>
>> I'm not a native speaker as well, but "with itself" doesn't
>> sound right to me.  How about something like:
>>
>>  target_adjusts_pc_after_break ()
>>
>>  if (target_adjusts_pc_after_break ())
>>   return;  /* ... then we don't have to.  */
>>
>> or
>>
>>  target_adjusts_pc_after_break_itself
>>
>> ?
>
> I will change it to "target_adjusts_pc_after_break".
>
>
> Thanks for your advices. I will make a new patch for it.
>
> Thanks,
> Hui
>
>
>>
>> > 2008-11-02  Hui Zhu  <teawater@gmail.com>
>> >
>> >       * target.h (target_ops): Add "to_adjust_pc_with_itself".
>> >       Return true if target adjust pc after break with itself.
>> >       (target_adjust_pc_with_itself): New macro.
>> >       Call "to_adjust_pc_with_itself".
>> >       * target.c (update_current_target): Set
>> >       "to_adjust_pc_with_itself".
>> >       * infrun.c (adjust_pc_after_break): If
>> >       "target_adjust_pc_with_itself" return true, not adjust pc.
>> >
>> > To make P record support it. I make another patch
>> > "record_adjust_pc_with_itself.txt".
>> >
>> > 2008-11-02  Hui Zhu  <teawater@gmail.com>
>> >
>> >       * record.c (record_adjust_pc_with_itself): New function.
>> >       Return true.
>> >       (init_record_ops): Set to_adjust_pc_with_itself point to
>> >       record_adjust_pc_with_itself.
>>
>> --
>> Pedro Alves
>