Re: [RFA/RFC] mips tracepoint: fix Bug 12013

[Hui Zhu <teawater@gmail.com>]

Thanks Pedro.  Fixed and checked in.

Joel,  do you think this patch can check in to 7.2.1.

Thanks,
Hui

On Tue, Dec 28, 2010 at 17:52, Pedro Alves <pedro@codesourcery.com> wrote:
> On Tuesday 28 December 2010 08:01:47, Hui Zhu wrote:
>
>> >> +static int
>> >> +mips_ax_pseudo_reg (struct gdbarch *gdbarch, struct agent_expr *ax, int reg)
>> >> +{
>> >> +  int rawnum = reg % gdbarch_num_regs (gdbarch);
>> >> +  gdb_assert (reg >= gdbarch_num_regs (gdbarch)
>> >> +           && reg < 2 * gdbarch_num_regs (gdbarch));
>> >> +  if (register_size (gdbarch, rawnum) >= register_size (gdbarch, reg))
>> >> +    {
>> >> +      ax_reg (ax, rawnum);
>> >> +
>> >> +      if (register_size (gdbarch, rawnum) > register_size (gdbarch, reg))
>> >> +        {
>> >> +       if (gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p
>> >> +           && gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
>> >> +         {
>> >> +           ax->buf[ax->len] = aop_const8;
>> >> +           ax->buf[ax->len + 1] = 32;
>> >> +           ax->buf[ax->len + 2] = aop_rsh_unsigned;
>> >> +           ax->len += 3;
>> >> +         }
>> >> +       else
>> >> +         {
>> >> +           ax->buf[ax->len] = aop_const32;
>> >> +           ax->buf[ax->len + 1] = 0xff;
>> >> +           ax->buf[ax->len + 2] = 0xff;
>> >> +           ax->buf[ax->len + 3] = 0xff;
>> >> +           ax->buf[ax->len + 4] = 0xff;
>> >> +           ax->buf[ax->len + 5] = aop_bit_and;
>> >> +           ax->len += 6;
>> >
>> > Hmm, I'm not sure, but don't you need to sign extend?
>> > mips-tdep.c:mips_pseudo_register_read treats this as signed.
>> >
>> > Why do you apply the "and 0xffffffff" logic when
>> > mips64_transfers_32bit_regs_p is false?
>>
>> I change this part to:
>>       if (register_size (gdbarch, rawnum) > register_size (gdbarch, reg))
>>         {
>>         if (!gdbarch_tdep (gdbarch)->mips64_transfers_32bit_regs_p
>>             || gdbarch_byte_order (gdbarch) != BFD_ENDIAN_BIG)
>>           {
>>             ax_const_l (ax, 32);
>>             ax_simple (ax, aop_lsh);
>>           }
>>         ax_const_l (ax, 32);
>>         ax_simple (ax, aop_rsh_signed);
>>       }
>>
>> If mips64_transfers_32bit_regs_p is false or arch is little endian,
>> use the low 32bit.
>> If not, use the the high 32bit.
>>
>> Do you think it is OK?
>
> I'm not sure I understand the double shift logic, but I also don't
> care enough to spend the time to understand it either.  :-)  You're now
> using the right interfaces, and if you've tested this (e.g., try
> collecting an expression that involves adding and subtracting values
> of registers, and see if the result is sane, and/or if your agent supports
> it, cooking up a small test that involves a conditional tracepoint
> involving arithmetic with register values, all this with a 32-bit app
> running on a 64-bit board), then it's fine with me.
> I also have to say that I'm also not sure what does
> mips64_transfers_32bit_regs_p being true mean WRT the agent's register
> size (as opposed to the remote protocol register size), but, I understand
> that's a legacy mode, so, we can adjust if turns out we need to do something
> else.
>
>
>> 2010-12-28  Hui Zhu  <teawater@gmail.com>
>>
>>       * gdbarch.sh (ax_pseudo_register_collect,
>>       ax_pseudo_register_push_stack): new callbacks.
>>       (agent_expr): New struct.
>
> Not really new.  Use something like "Forward declare." instead.
>
>>       * gdbarch.c (gdbarch): Add ax_pseudo_register_collect and
>>       ax_pseudo_register_push_stack.
>>       (startup_gdbarch): Add 0 for ax_pseudo_register_collect and
>>       ax_pseudo_register_push_stack.
>>       (verify_gdbarch): Add comments for ax_pseudo_register_collect and
>>       ax_pseudo_register_push_stack.
>>       (gdbarch_dump): Add fprintf_unfiltered for ax_pseudo_register_collect
>>       and ax_pseudo_register_push_stack.
>>       (gdbarch_ax_pseudo_register_collect_p,
>>       gdbarch_ax_pseudo_register_collect,
>>       set_gdbarch_ax_pseudo_register_collect,
>>       gdbarch_ax_pseudo_register_push_stack_p,
>>       gdbarch_ax_pseudo_register_push_stack,
>>       set_gdbarch_ax_pseudo_register_push_stack): New functions.
>>       * gdbarch.h (agent_expr): New struct.
>>       (gdbarch_ax_pseudo_register_collect_p,
>>       gdbarch_ax_pseudo_register_collect,
>>       set_gdbarch_ax_pseudo_register_collect,
>>       gdbarch_ax_pseudo_register_push_stack_p,
>>       gdbarch_ax_pseudo_register_push_stack,
>>       set_gdbarch_ax_pseudo_register_push_stack): New externs.
>>       (gdbarch_ax_pseudo_register_collect_ftype,
>>       gdbarch_ax_pseudo_register_push_stack_ftype): New typedeies.
>
> That's not what I meant with
>
>> > You need to mention that gdbarch.c and gdbarch.h were regenerated.
>
> Write this, literaly:
>
>        * gdbarch.h, gdbarch.c: Regenerate.
>
> That's all.
>
>>       * ax-gdb.c (gen_expr): Remove pseudo-register check code.
>>       * ax-general.c (user-regs.h): New include.
>>       (ax_reg): Call gdbarch_ax_pseudo_register_push_stack.
>>       (ax_reg_mask): Call gdbarch_ax_pseudo_register_collect.
>>       * mips-tdep.c (ax.h): New include.
>>       (mips_ax_pseudo_register_collect,
>>       mips_ax_pseudo_register_push_stack): New functions.
>>       (mips_gdbarch_init): Set mips_ax_pseudo_register_collect and
>>       mips_ax_pseudo_register_push_stack.
>
> Otherwise okay.  Thanks!
>
> --
> Pedro Alves
>