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

[Hui Zhu <teawater at gmail dot com>]

Thanks for your help Pedro.

On Mon, Dec 27, 2010 at 21:09, Pedro Alves <pedro@codesourcery.com> wrote:
> 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.

When this patch ok,  I will post a separate patch for it.

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

Agree with you.  These names are more better than what I use.  I will use them.

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

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?

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

So sorry about that.
I have some net issue so I cannot use gmail with SMTP sometimes.  So I
use the attachment to send the patch.

Please help me review the new patch.

Thanks,
Hui

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.
	* 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.
	* 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.

Attachment: trace-pseudo.txt
Description: Text document