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

Pedro Alves
2010-12-27T13:10:12Z

On Saturday 25 December 2010 17:09:21, Hui Zhu wrote:
> On Wed, Dec 22, 2010 at 19:26, Pedro Alves <pedro@codesourcery.com> wrote:
...
> I make a patch according to your comments.

Thanks! I'm liking this. Would be super cool to have these
implemented on x86/x86_64 as well.

> It add to callback in gdbarch. They are gdbarch_ax_pseudo_reg and
> gdbarch_ax_pseudo_reg_mask.
>
> gdbarch_ax_pseudo_reg will be called by ax_reg. It assemble code to
> push the value of pseudo register number REG on the stack.
> gdbarch_ax_pseudo_reg_mask will be called by ax_reg_mask. It add
> pseudo register REG to the register mask for expression AX.

Well, not to the register mask. It really isn't garanteed
that a pseudo register maps to a raw register. It could
map to some value stored at some memory address, so the new
callback may do other things than flipping a bit
in the raw registers to collect mask (hence the ax_reg_mask function
name). I can see why you named the callbacks like that, and I'll approve
the patch with such names anyway, though I'd prefer naming them
something more descriptive like gdbarch_ax_pseudo_register_collect /
gdbarch_ax_pseudo_register_push_stack. E.g.,

# Assemble agent expression bytecode to collect pseudo-register REG.
# Return -1 if something goes wrong, 0 otherwise.
M:int:ax_pseudo_register_collect:struct agent_expr *ax, int reg:ax, reg

# Assemble agent expression bytecode to push the value of pseudo-register
# REG on the interpreter stack.
# Return -1 if something goes wrong, 0 otherwise.
M:int:ax_pseudo_register_push_stack:struct agent_expr *ax, int reg:ax, reg

> 2010-12-26 Hui Zhu <teawater@gmail.com>
>
> * ax-gdb.c (gen_expr): Remove pseudo-register check code.
> * ax-general.c (user-regs.h): New include.
> (ax_reg): Call gdbarch_ax_pseudo_reg.
> (ax_reg_mask): Call gdbarch_ax_pseudo_reg_mask.
> * gdbarch.sh (ax_pseudo_reg, ax_pseudo_reg_mask): new callback.

Capitalize, plural: "New callbacks."

> * mips-tdep.c (ax.h): New include.
> (mips_ax_pseudo_reg, mips_ax_pseudo_reg_mask): New function.

functions.

> (mips_gdbarch_init): Set mips_ax_pseudo_reg and
> mips_ax_pseudo_reg_mask.

You need to mention that gdbarch.c and gdbarch.h were regenerated.

> /* Given an agent expression AX, fill in requirements and other descriptive
> --- a/gdbarch.sh
> +++ b/gdbarch.sh
...
> @@ -919,6 +928,7 @@ struct target_desc;
> struct displaced_step_closure;
> struct core_regset_section;
> struct syscall;
> +struct agent_expr;

You forgot to mention this in the changelog.

> +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?

> + }

Please use the functions in ax-general.c that generate
the bytecode instead of writing to the bytecode buffer
directly (and forgetting to ensure there's enough room in
the buffer): ax_const_l/ax_simple, etc.

The patch you posted got line-wrapped/mangled, and I couldn't
apply it even after trying to manually fix it. Can you please
make the necessary tweaks to your email client to make that
not happen? (Me, to send patches with gmail, I find it simpler
to use an email client / SMTP than using gmail's web interface)

--
Pedro Alves