Thanks for your help Pedro. On Mon, Dec 27, 2010 at 21:09, Pedro Alves wrote: > On Saturday 25 December 2010 17:09:21, Hui Zhu wrote: >> On Wed, Dec 22, 2010 at 19:26, Pedro Alves 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   >> >>       * 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 * 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.