Re: [PATCH 5/8] [PowerPC] Fix access to VSCR in linux targets

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Pedro Franco de Carvalho wrote:

> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Franco de Carvalho  <pedromfc@linux.vnet.ibm.com>
> 
> 	* ppc-linux-tdep.c (ppc_linux_collect_vrregset): New function.
> 	(ppc_linux_vrregset) : New function.
> 	(ppc32_le_linux_vrregmap, ppc32_be_linux_vrregmap)
> 	(ppc32_le_linux_vrregset, ppc32_be_linux_vrregset): New globals.
> 	(ppc32_linux_vrregset): Remove.
> 	(ppc_linux_iterate_over_regset_sections): Call ppc_linux_vrregset
> 	and use result instead of ppc32_linux_vrregset.
> 	* ppc-linux-tdep.h (ppc_linux_vrregset): New declaration.
> 	* ppc-linux-nat.c: Include regset.h.
> 	(gdb_vrregset_t): Adjust comment to account for little-endian
> 	mode.
> 	(supply_vrregset, fill_vrregset): Remove.
> 	(fetch_altivec_register, store_altivec_register): Remove.
> 	(fetch_altivec_registers): Add regno parameter. Get regset using
> 	ppc_linux_vrregset. Use regset to supply registers.
> 	(store_altivec_registers): Add regno parameter. Get regset using
> 	ppc_linux_vrregset. Use regset to collect registers.
> 	(fetch_register): Call fetch_altivec_registers instead of
> 	fetch_altivec_register.
> 	(store_register): Call store_altivec_registers instead of
> 	store_altivec_register.
> 	(fetch_ppc_registers): Call fetch_altivec_registers with -1 for
> 	the new regno parameter.
> 	(store_ppc_registers): Call store_altivec_registers with -1 for
> 	the new regno parameter.
> 
> gdb/gdbserver/ChangeLog:
> yyyy-mm-dd  Pedro Franco de Carvalho  <pedromfc@linux.vnet.ibm.com>
> 
> 	* linux-ppc-low.c (ppc_fill_vrregset): Add vscr_offset variable.
> 	Set vscr_offset to 0 in little-endian mode and 12 in big-endian
> 	mode. Call collect_register_by_name with vscr using
> 	vscr_offset. Zero-pad vscr and vrsave fields in collector buffer.
> 	(ppc_store_vrregset): Add and set vscr_offset variable as in
> 	ppc_fill_vrregset. Call supply_register_by_name with vscr using
> 	vscr_offset.

I agree with the general direction of this patch.  A few comments:

> +  vrregset->supply_regset(vrregset, regcache, regno, &regs,
> +			  PPC_LINUX_SIZEOF_VRREGSET);

Formatting (space before '(').

> +static void
> +ppc_linux_collect_vrregset (const struct regset *regset,
> +			    const struct regcache *regcache,
> +			    int regnum, void *buf, size_t len)
> +{
> +  gdb_byte *vrregs = (gdb_byte *) buf;
> +
> +  /* Zero-pad the unused bytes in the fields for vscr and vrsave
> +     in case they get displayed somewhere (e.g. in core files).  */
> +  if (regnum == PPC_VSCR_REGNUM || regnum == -1)
> +    memset (&vrregs[32 * 16], 0, 16);
> +
> +  if (regnum == PPC_VRSAVE_REGNUM || regnum == -1)
> +    memset (&vrregs[33 * 16], 0, 16);
> +
> +  regcache_collect_regset (regset, regcache, regnum, buf, len);
> +}

I'm wondering if we shouldn't have the common regcache_collect_regset
routine zero out areas covered by REGCACHE_MAP_SKIP?  Then we wouldn't
need this extra routine here.

> -static const struct regset ppc32_linux_vrregset = {
> -  &ppc32_linux_reg_offsets,
> -  ppc_supply_vrregset,
> -  ppc_collect_vrregset

This removes all users of ppc_supply_vrregset / ppc_collect_vrregset,
right?  Then those functions should go away as well, together with
all their associates data, in particular the v*_offset fields in
struct ppc_reg_offsets ...

> +static const struct regcache_map_entry ppc32_le_linux_vrregmap[] =
> +  {
> +      { 32, PPC_VR0_REGNUM, 16 },
> +      { 1, PPC_VSCR_REGNUM, 16 },
> +      { 1, PPC_VRSAVE_REGNUM, 16 },
> +      { 0 }
> +  };
> +
> +static const struct regcache_map_entry ppc32_be_linux_vrregmap[] =
> +  {
> +      { 32, PPC_VR0_REGNUM, 16 },
> +      { 1, REGCACHE_MAP_SKIP, 12},
> +      { 1, PPC_VSCR_REGNUM, 4 },
> +      { 1, PPC_VRSAVE_REGNUM, 16 },
> +      { 0 }
> +  };

This looks weirdly asymmetric ... shouldn't we then use
REGCACHE_MAP_SKIP for both LE and BE cases?  In fact, it would
probably make sense to make VRSAVE also a 4-byte field + skip.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com