Re: [PATCH-ppc 1/5] Add basic support for new VSX register set

[Thiago Jung Bauermann <bauerman at br dot ibm dot com>]

On Fri, 2008-07-25 at 17:15 -0300, Luis Machado wrote:
> This patch adds basic functionality for VSX registers as follows:
> 
> * Adds pseudo-registers for VSX and Extended FPR's.
> * Read/Write functions for pseudo-registers.
> * Read/Write functions for VSxH registers.
> * New ptrace definitions for GET_VSRREGS and SET_VSRREGS and calls to
> fetch/store registers, in synch with the kernel.
> * Defines numbering scheme for pseudo-registers. 

I'm no maintainer, but FWIW I reviewed and found only small issues. 

> +/* Supply register REGNUM in the VSX register set REGSET
> +   from the buffer specified by VSRREGS and LEN to register cache
> +   REGCACHE.  If REGNUM is -1, do this for all registers in REGSET.
*/
> +
> +void
> +ppc_supply_vsxregset (const struct regset *regset, struct regcache
*regcache,
> +                    int regnum, const void *vsxregs, size_t len)
> +{
> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
> +  struct gdbarch_tdep *tdep;
> +  size_t offset = 0;

This variable is always zero, so it's of no use.

> +/* Collect register REGNUM in the VSX register set
> +   REGSET from register cache REGCACHE into the buffer specified by
> +   VSRREGS and LEN.  If REGNUM is -1, do this for all registers in
> +   REGSET.  */
> +
> +void
> +ppc_collect_vsxregset (const struct regset *regset,
> +                     const struct regcache *regcache,
> +                     int regnum, void *vsxregs, size_t len)
> +{
> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
> +  struct gdbarch_tdep *tdep;
> +  size_t offset = 0;

Same here.

> +         static const char *const vsx_regs[] = {
> +           "vs0h", "vs1h", "vs2h", "vs3h", "vs4h", "vs5h",
> +           "vs6h", "vs7h", "vs8h", "vs9h", "vs10h", "vs11h",
> +           "vs12h", "vs13h", "vs14h", "vs15h", "vs16h", "vs17h",
> +           "vs18h", "vs19h", "vs20h", "vs21h", "vs22h", "vs23h",
> +           "vs24h", "vs25h", "vs26h", "vs27h", "vs28h", "vs29h",
> +           "vs30h", "vs31h", "vs32h"
> +         };

Maybe it's too late at night and I'm missing the obvious, but shouldn't
it go only up to vs31h and not vs32h?

> +         for (i = 0; i < ppc_num_gprs; i++)
> +           valid_p &= tdesc_numbered_register (feature, tdesc_data,
> +                                               PPC_VSR0_UPPER_REGNUM
+ i,
> +                                               vsx_regs[i]);

I don't think this loop should use ppc_num_gprs. Perhaps a new constant
needs to be created?

>  static void
> +supply_vsxregset (struct regcache *regcache, gdb_vsxregset_t
*vsxregsetp)
> +{
> +  int i;
> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +  int num_of_vsxregs = 32;

This should be a constant. One more reason to create a ppc_num_vsx or
somesuch (mmm... coming up with a good name for the constant is not
that easy).

> +/* Store one VSX register. */
> +static void
> +store_vsx_register (const struct regcache *regcache, int tid, int
regno)
> +{
> +  int ret;
> +  int offset = 0;

This variable is not used.

>  static void
> +fill_vsxregset (const struct regcache *regcache, gdb_vsxregset_t
*vsxregsetp)
> +{
> +  int i;
> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +  int num_of_vsxregs = 32;

This should be a constant too.

>  /* Constants for register set sizes.  */
>  enum
>    {
> -    ppc_num_gprs = 32,          /* 32 general-purpose registers */
> -    ppc_num_fprs = 32,          /* 32 floating-point registers */
> -    ppc_num_srs = 16,           /* 16 segment registers */
> -    ppc_num_vrs = 32            /* 32 Altivec vector registers */
> +    ppc_num_gprs = 32,         /* 32 general-purpose registers.  */
> +    ppc_num_fprs = 32,         /* 32 floating-point registers.  */
> +    ppc_num_srs = 16,          /* 16 segment registers.  */
> +    ppc_num_vrs = 32,          /* 32 Altivec vector registers.  */
> +    ppc_num_vsrs = 64,         /* 64 VSX vector registers.  */
> +    ppc_num_efprs = 32         /* 32 Extended FP registers.  */
>    };

Why change the column of all the comments here?

>  /* Linux target descriptions.  */
>  extern struct target_desc *tdesc_powerpc_32l;
>  extern struct target_desc *tdesc_powerpc_altivec32l;
> +extern struct target_desc *tdesc_powerpc_vsx32l;
>  extern struct target_desc *tdesc_powerpc_e500l;
>  extern struct target_desc *tdesc_powerpc_64l;
>  extern struct target_desc *tdesc_powerpc_altivec64l;
> -
> +extern struct target_desc *tdesc_powerpc_vsx64l;
>  #endif /* PPC_LINUX_TDEP_H */

I know this is very picky, but I'd preserve the blank line before
#endif.

-- 
[]'s
Thiago Jung Bauermann
IBM Linux Technology Center