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

[Elena Zannoni <ezannoni at redhat dot com>]

Kevin Buettner writes:
 > 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.  ;-)

No, that was not intentional! :-)

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

Yes, you are right. Thanks for catching this. I was using a different
algorithm, and I changed things at the last minute. I shuld have gone
to sleep instead.

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

Yes, right again. I was processing those two separately in an earlier
version of this patch. That's why the loop is off. Will fix.

 > > +    {
 > > +      /* 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.
 > 

Yes, as above.

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

Thanks. I had a last minute change of hart before posting the
patch. That will teach me.

I'll repost with fixes.

Elena


 > Kevin