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

[Hui Zhu <teawater at gmail dot 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
>