This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: [RFA] ppc-linux-nat.c AltiVec regs ptrace
Elena,
It looks pretty good. I noticed a typo in a comment. After your
earlier remarks, I'm beginning to think that you put these in
deliberately to see if I'd notice. ;-)
There are a few things that I'm confused about though...
On Feb 20, 9:09pm, Elena Zannoni wrote:
> + time throuhg, and we have comfirmed that there is kernel
^^^^^^^
Here's the typo.
Here's where I'm confused...
> +static void
> +supply_vrregset (gdb_vrregset_t *vrregsetp)
> +{
> + int i;
> + struct gdbarch_tdep *tdep = gdbarch_tdep (current_gdbarch);
> + int num_of_vrregs = tdep->ppc_vrsave_regnum - tdep->ppc_vr0_regnum;
Are you off by one here? I.e, shouldn't the above statement be
int num_of_vrregs = tdep->ppc_vrsave_regnum - tdep->ppc_vr0_regnum + 1;
?
> + int vrregsize = REGISTER_RAW_SIZE (tdep->ppc_vr0_regnum);
> + int offset = vrregsize - REGISTER_RAW_SIZE (tdep->ppc_vrsave_regnum);
> +
> + for (i = 0; i < num_of_vrregs - 1; i++)
Why ``num_of_vrregs - 1'' ? It seems to me that the combination of the
fencepost error (above) combined with this one means that the last two
Altivec registers won't get processed.
> + {
> + /* The last 2 registers of this set are only 32 bit long, not
> + 128. However an offset is necessary only for VSCR because it
> + occupies a whole vector, while VRSAVE occupies a full 4 bytes
> + slot. */
> + if (i == (tdep->ppc_vrsave_regnum - 1))
> + supply_register (tdep->ppc_vr0_regnum + i,
> + *vrregsetp + i * vrregsize + offset);
> + else
> + supply_register (tdep->ppc_vr0_regnum + i, *vrregsetp + i * vrregsize);
> + }
> +}
[...]
> static void
> +fill_vrregset (gdb_vrregset_t *vrregsetp)
> +{
> + int i;
> + struct gdbarch_tdep *tdep = gdbarch_tdep (current_gdbarch);
> + int num_of_vrregs = tdep->ppc_vrsave_regnum - tdep->ppc_vr0_regnum;
I think this is another fencepost error.
> + int vrregsize = REGISTER_RAW_SIZE (tdep->ppc_vr0_regnum);
> + int offset = vrregsize - REGISTER_RAW_SIZE (tdep->ppc_vrsave_regnum);
> +
> + for (i = 0; i < num_of_vrregs; i++)
This one looks right to me though, so long as you fix the computation of
num_of_vrregs.
Kevin