Re: [patch] Add support for PPC Altivec registers in core files

["Ulrich Weigand" <uweigand@de.ibm.com>]

Carlos Eduardo Seo wrote:

> Recently, a kernel patch by Mark Nelson added support for Altivec
> registers in core dumps under a new note section called NT_PPC_VMX. Two
> other patches (by myself and Roland McGrath) added support for the new
> note section in binutils. Now, this patch adds support for Altivec
> registers debugging in core files, using the contents under NT_PPC_VMX.

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

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com