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

[teawater <teawater@gmail.com>]

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
>