Re: [RFA] ppc-linux-nat.c AltiVec regs ptrace

[Kevin Buettner <kevinb@redhat.com>]

Elena,

It looks pretty good.  I noticed a typo in a comment.  After your
earlier remarks, I'm beginning to think that you put these in
deliberately to see if I'd notice.  ;-)

There are a few things that I'm confused about though...

On Feb 20,  9:09pm, Elena Zannoni wrote:

> +         time throuhg, and we have comfirmed that there is kernel
                 ^^^^^^^
Here's the typo.

Here's where I'm confused...

> +static void
> +supply_vrregset (gdb_vrregset_t *vrregsetp)
> +{
> +  int i;
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (current_gdbarch);
> +  int num_of_vrregs = tdep->ppc_vrsave_regnum - tdep->ppc_vr0_regnum;

Are you off by one here?  I.e, shouldn't the above statement be

     int num_of_vrregs = tdep->ppc_vrsave_regnum - tdep->ppc_vr0_regnum + 1;

?

> +  int vrregsize = REGISTER_RAW_SIZE (tdep->ppc_vr0_regnum);
> +  int offset = vrregsize - REGISTER_RAW_SIZE (tdep->ppc_vrsave_regnum);
> +
> +  for (i = 0; i < num_of_vrregs - 1; i++)

Why ``num_of_vrregs - 1'' ?  It seems to me that the combination of the
fencepost error (above) combined with this one means that the last two
Altivec registers won't get processed.

> +    {
> +      /* The last 2 registers of this set are only 32 bit long, not
> +         128.  However an offset is necessary only for VSCR because it
> +         occupies a whole vector, while VRSAVE occupies a full 4 bytes
> +         slot.  */
> +      if (i == (tdep->ppc_vrsave_regnum - 1))
> +        supply_register (tdep->ppc_vr0_regnum + i,
> +                         *vrregsetp + i * vrregsize + offset);
> +      else
> +        supply_register (tdep->ppc_vr0_regnum + i, *vrregsetp + i * vrregsize);
> +    }
> +}

[...]

>  static void
> +fill_vrregset (gdb_vrregset_t *vrregsetp)
> +{
> +  int i;
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (current_gdbarch);
> +  int num_of_vrregs = tdep->ppc_vrsave_regnum - tdep->ppc_vr0_regnum;

I think this is another fencepost error.

> +  int vrregsize = REGISTER_RAW_SIZE (tdep->ppc_vr0_regnum);
> +  int offset = vrregsize - REGISTER_RAW_SIZE (tdep->ppc_vrsave_regnum);
> +
> +  for (i = 0; i < num_of_vrregs; i++)

This one looks right to me though, so long as you fix the computation of
num_of_vrregs.

Kevin