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

Ulrich Weigand
2007-10-26T00:04:14Z

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