Re: [PATCH 5/6] gdb/x86: Implement ax_pseudo_register_collect hook.

[Marcin KoÅcielnicki <koriakin at 0x04 dot net>]

On 10/02/16 14:31, Pedro Alves wrote:
> Great stuff.  Minor comments follow...
>
> Patch is missing intro comments to new functions:
>
>    /* Implementation of foo. / See foo.h. / etc.  */

OK.
> On 02/06/2016 08:54 PM, Marcin KoÅcielnicki wrote:
>
> > +static int
> > +amd64_ax_pseudo_register_collect (struct gdbarch *gdbarch,
> > +				  struct agent_expr *ax, int regnum)
> > +{
> > +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>
> Empty line after decl here.

OK.
> > +int
> > +i386_ax_pseudo_register_collect (struct gdbarch *gdbarch,
> > +				 struct agent_expr *ax, int regnum)
> > +{
> > +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>
> Ditto.
>
> > +  if (i386_mmx_regnum_p (gdbarch, regnum))
> > +    {
> > +      /* MMX to FPU register mapping depends on current TOS.  Let's just
> > +	 not care and collect everything...  */
> > +      int i;
>
> Ditto.  (several instances more in the function)
>
> > +      ax_reg_mask (ax, I387_FSTAT_REGNUM (tdep));
> > +      for (i = 0; i < 8; i++)
> > +	ax_reg_mask (ax, I387_ST0_REGNUM (tdep) + i);
> > +      return 0;
> > +    }
>
>
> > +  else if (i386_byte_regnum_p (gdbarch, regnum))
> > +    {
> > +      /* Check byte pseudo registers last since this function will
> > +	 be called from amd64_ax_pseudo_register_collect, which handles
> > +	 byte pseudo registers differently.  */
>
> I don't understand this comment.  amd64_ax_pseudo_register_collect
> checks i386_byte_regnum_p before ever reaching here, afaics.

I've copied it from i386_pseudo_register_read_into_value - didn't make 
that much sense to me either.  Let's remove it from both places?
> > +      int gpnum = regnum - tdep->al_regnum;
> > +      ax_reg_mask (ax, gpnum % 4);
> > +      return 0;
> > +    }
> > +  else
> > +    internal_error (__FILE__, __LINE__, _("invalid regnum"));
> > +  return 1;
> > +}
>
> Thanks,
> Pedro Alves