-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



Ulrich Weigand wrote:
>
> I've noticed a couple of issues:
>
>> +extern void ppc_supply_vrregset (const struct regset *regset,
>> +                                 struct regcache *regcache,
>> +                         int regnum, const void *vrregs, size_t len);
>
> Whitespace?   Please use tabs consistently ...
>
>> +extern void ppc_collect_vrregset (const struct regset *regset,
>> +                          const struct regcache *regcache,
>> +                          int regnum, void *vrregs, size_t len);
>
> Likewise.
>
>> +ppc_altivec_support_p (struct gdbarch *gdbarch)
>> +{
>> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>> +
>> +  return (tdep->ppc_vr0_regnum >= 0
>> +          && tdep->ppc_vrsave_regnum >= 0
>> +          && (tdep->ppc_vrsave_regnum - 1) >= 0);
>
> The last check is superfluous.
>
>> +static int
>> +ppc_vrreg_offset (struct gdbarch_tdep *tdep,
>> +          const struct ppc_reg_offsets *offsets,
>> +          int regnum)
>> +{
>> +  if (regnum >= tdep->ppc_vr0_regnum
>> +      && regnum < tdep->ppc_vr0_regnum + ppc_num_vrs)
>> +    return offsets->vr0_offset + (regnum - tdep->ppc_vr0_regnum) * 8;
>
> Shouldn't that be * 16 instead of * 8?
>
>> +  if (regnum == (tdep->ppc_vrsave_regnum - 1))
> Parentheses are superfluous.
>
>> +    return offsets->vscr_offset;
>> +
>> +  if (regnum == tdep->ppc_vrsave_regnum)
>> +    return offsets->vrsave_offset;
>> +
>> +  return -1;
>> +}
>
>
>> +  offset = ppc_vrreg_offset (tdep, offsets, regnum);
>> +  if ((regnum != tdep->ppc_vrsave_regnum) && (regnum !=
(tdep->ppc_vrsave_regnum - 1)))
> Parentheses are superfluous as well.  Also, the line is probably too long.
>
>> +    ppc_supply_reg (regcache, regnum, vrregs, offset, 16);
>> +  else if (regnum == (tdep->ppc_vrsave_regnum - 1))
>> +    ppc_supply_reg (regcache, regnum,
>> +            vrregs, offsets->vscr_offset, 4);
>> +  else
>> +    ppc_supply_reg (regcache, regnum,
>> +            vrregs, offsets->vrsave_offset, 4);
>
> In the latter two case, why don't you trust the offset
> ppc_vrreg_offset has returned?
>
>
>> +  offset = ppc_vrreg_offset (tdep, offsets, regnum);
>> +  if ((regnum != tdep->ppc_vrsave_regnum) && (regnum !=
(tdep->ppc_vrsave_regnum - 1)))
>> +    ppc_collect_reg (regcache, regnum, vrregs, offset, 16);
>> +  else if (regnum == (tdep->ppc_vrsave_regnum - 1))
>> +    ppc_collect_reg (regcache, regnum,
>> +            vrregs, offsets->vscr_offset, 4);
>> +  else
>> +    ppc_collect_reg (regcache, regnum,
>> +            vrregs, offsets->vrsave_offset, 4);
> Likewise.
>
>
>> --- gdb/corelow.c.orig
>> +++ gdb/corelow.c
>> @@ -499,6 +499,8 @@ get_core_registers (struct regcache *reg
>>                   ".reg2", 2, "floating-point", 0);
>>    get_core_register_section (regcache,
>>                   ".reg-xfp", 3, "extended floating-point", 0);
>> +  get_core_register_section (regcache,
>> +                   ".reg-ppc-vmx", 3, "ppc Altivec", 0);
>
> Hmmm.  It would be nicer if this Altivec-specific statement didn't
> have to be in common code.  But I don't see a simple alternative,
> so I won't object to it ...
>
>
> Are you planning on adding gcore support for Altivec register as well?
>
> Bye,
> Ulrich
>
Thanks for reviewing this. I've attached the updated patch.

As for gcore, I'm not familiar with it. If you point out some
directions, I could do it as well. No problem.

Regards,


- --
Carlos Eduardo Seo
Software Engineer
IBM Linux Technology Center
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.7 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFHIfXAqvq7Aov/qQARArnjAJ9ecaH0bZb8PbjRfoK8jA96aGedbQCeMdK/
clVXzCKVQHC6I1yRdENlkH4=
=hNeR
-----END PGP SIGNATURE-----