Re: [RFA] W.I.P. AltiVec ppc registers support.

[Kevin Buettner <kevinb@redhat.com>]

On Nov 28,  9:15pm, Elena Zannoni wrote:

> The first Changelog is just for the most recent change set. I included
> the older one for easy reference.

Thanks for doing this.

> I tested this on AIX4.3, on a Linux-ppc with Altivec kernel support,
> and on a Linux-ppx w/o Altivec kernel support.

Good.

Now, for my comments...

First, regarding the AltiVec ptrace() support in the Linux/PPC
kernel...  Have the necessary patches been submitted to some public
mailing list?  If so, I'd be willing to let the ptrace related bits go
in even if they haven't been agreed upon by the kernel developers.  We
can always make adjustments to GDB later on after the kernel folks
have hashed out how they want to do things.  OTOH, I'm extremely
reluctant approve any changes to GDB based on kernel patches which
haven't at least been posted for commentary to some public list.

Regarding the implementation...

> +/* AltiVec regs are not always supported by the kernel. We need to fake them,
> +   and let the ptrace requests fail.  */
> +#ifndef PT_VR0
> +#define PT_VR0 (PT_FPSCR + 1)
> +#endif
> +#ifndef PT_VR31
> +#define PT_VR31 (PT_VR0 + 4*31)
> +#endif
> +#ifndef PT_VRCR
> +#define PT_VRCR (PT_VR0 + 4*32)
> +#endif
> +#ifndef PT_VRSAVE
> +#define PT_VRSAVE (PT_VR0 + 4*33)
> +#endif

I don't think it's a good idea to provide fake PT_ constants,
especially since the kernel folks haven't agreed upon using PEEKUSER /
POKEUSER for accessing the AltiVec registers.  Also, in the interim,
it could happen that someone else will come up with a different ptrace
extension (perhaps involving hardware watchpoints?) that uses
constants at some of the offsets used by your fake AltiVec registers. 
If this were to happen, gdb could potentially write these registers
with unpredictable consequences.

The PT_V0 constant is the only constant used in the above defines and
it's only used in one place (in ppc_register_u_addr).  Rather than
using the above #defines, might I suggest that instead of doing:

  /* Altivec registers: 4 slots each. */
  if (altivec_register_p (regno))
    u_addr = (ustart + (PT_VR0 + (regno - gdbarch_tdep (current_gdbarch)->first_altivec_regnum) * 4) * 4);

you instead do:

  /* Altivec registers: 4 slots each. */
  if (altivec_register_p (regno))
    {
#ifdef PT_VR0
      u_addr = (ustart + (PT_VR0 + (regno - gdbarch_tdep (current_gdbarch)->first_altivec_regnum) * 4) * 4);
#else
      u_addr = -1;
#endif
    }

The #else clause isn't really necessary because you already initialize
u_addr to -1 at the outset.

The other advantage to doing things this way is that you'll no longer
need a separate fetch_altivec_register() function which differs from
fetch_register() only in that it doesn't print a message when ptrace()
produces an error.  (And likewise for store_register() /
store_altivec_register().)

....

Regarding...

> -static int regmap[] =
[etc]

from your previous patch, I've decided that while the code ends up being
more verbose, it is easier to read and understand what's going on.  So
I'm approving your previous patch.  (I'll send out a separate approval
message if you wish.)

With regard to

> Index: config/powerpc/nm-linux.h
[...]
> +#define FETCH_INFERIOR_REGISTERS

I think this is a good thing.  It *might not* be strictly necessary
for adding AltiVec support via PEEKUSER / POKEUSER, but it does give
us more control.  Also doing this allows us to clean up the code in other
ways.  E.g, the following bit from config/powerpc/nm-linux.h can be
removed:

>  extern int ppc_register_u_addr (int, int);
>  #define REGISTER_U_ADDR(addr, blockend, regno) \
>          (addr) = ppc_register_u_addr ((blockend),(regno));

This in turn means that ppc_register_u_addr() can be made static
and that the ``ustart'' parameter can be removed.  All calls to
register_addr() (in your new code) in ppc-linux-nat.c should be
changed to invoke ppc_register_u_addr() directly.

....

Your changes to rs6000-tdep.c and ppc-tdep.h look fine to me.

Kevin