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

[Elena Zannoni <ezannoni@cygnus.com>]

Kevin Buettner writes:
 > 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.

No :-( I am going hopefully to sort this out very soon.

 > 
 > 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().)
 > 

Oh, right, I don't use anything but PT_VR0 now (I did in ohter
versions of the patch, and forgot to erase them).  It indeed seems
cleaner doing like you say. I also came up with a configure patch but
that seemed less flexible.

 > ....
 > 
 > 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.)
 > 

Thanks, please, since that is a separate thread.

 > 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.
 > 

Unfortunately not. I thought the same, until I remembered about core
file debugging. That function is called by fetch_core_registers() in
core-aout.c.


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

OK.

 > Kevin

Elena