This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA/RFC] mips tracepoint: fix Bug 12013
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