Re: [RFA] ppc-linux-nat.c AltiVec regs ptrace

[Elena Zannoni <ezannoni at redhat dot com>]

Daniel Jacobowitz writes:
 > On Wed, Feb 20, 2002 at 03:20:44PM -0500, Elena Zannoni wrote:
 > > This patch adds support for fetching and storing the AltiVec registers
 > > on PPC.  I previously submitted a different patch based on a different
 > > ptrace interface that was rejected by the ppc linux kernel maintainer.
 > > A new ptrace interface has since been added to the kernel:
 > > http://ppc.bkbits.net:8080/linuxppc_2_4/cset@1.543.1.14
 > > 
 > > This was the old patch, which is now obsolete:
 > > http://sources.redhat.com/ml/gdb-patches/2001-11/msg00548.html
 > 
 > Thanks for keeping at this after we made you run around in circles on
 > the ptrace interface.
 > 
 > > The Altivec registers are defined like this:
 > > #define ELF_NVRREG      33      /* includes vscr */
 > > /* Altivec registers */
 > > typedef __vector128 elf_vrreg_t;
 > > typedef elf_vrreg_t elf_vrregset_t[ELF_NVRREG];
 > > 
 > > There are 32 vector registers 16 bytes longs, plus a VSCR register
 > > which is only 4 bytes long, but is fetched as a 16 bytes quantity. Up
 > > to here we have the elf_vrregset_t structure.
 > > Appended to this there is space for the VRSAVE register: 4 bytes.
 > > Even though this vrsave register is not included in the regset
 > > typedef, it is handled by the ptrace requests.
 > > 
 > > The layout is like this:
 > > 
 > >  |.|.|.|.|.....|.|.|.|.||.|.|.|x||.|
 > >  <------->     <-------><-------><->
 > >    VR0           VR31     VSCR    VRSAVE
 > > (where x is the actual value of the vscr reg)
 > > 
 > > 
 > > I have added a typedef to the ppc-linux-nat.c file to deal with
 > > such a layout. I am not sure whether this is the best place to put
 > > that. Maybe in gregset.h?
 > 
 > Ew.  But I think it should remain local to ppc-linux-nat.c rather than
 > being visible anywhere outside the nat file.  Nothing else should need
 > to know.  Well, eventually the -tdep, since we will need this for
 > core files, but cross corefiles are not your problem at this point.

Yes, I don't like it much myself, that's why it's buried in here.

 > 
 > > Index: ppc-linux-nat.c
 > > ===================================================================
 > > RCS file: /cvs/uberbaum/gdb/ppc-linux-nat.c,v
 > > retrieving revision 1.15
 > > diff -u -p -r1.15 ppc-linux-nat.c
 > > --- ppc-linux-nat.c	2002/02/18 15:08:40	1.15
 > > +++ ppc-linux-nat.c	2002/02/20 19:27:29
 > > @@ -17,7 +17,7 @@
 > >     You should have received a copy of the GNU General Public License
 > >     along with this program; if not, write to the Free Software
 > >     Foundation, Inc., 59 Temple Place - Suite 330,
 > > -   Boston, MA 02111-1307, USA.  */
 > > +   Boston, MA 02111-1307, USA.*/
 > 
 > Nit.  Two spaces after periods.  You did this in a number of other
 > places throughout the patch, too.
 > 

Right will fix. [I had an ongoing bet :-)]

 > > +int have_ptrace_getvrregs
 > > +#ifdef HAVE_PTRACE_GETFPXREGS
 > > +     = 1;
 > > +#else
 > > +     = 0;
 > > +#endif
 > > +
 > 
 > Huh?  You defined GETVRREGS unconditionally above.  GETFPXREGS has no
 > place in this file, does it?  Or do the headers define GETFPXREGS?
 > You also continue this confusion all the way down the patch.
 > 

The glibc headers define GETFPXREGS, and that's what we test for in
the configury.  But we are not dealing with floating point registers
here, so I used the 'correct' name where I could.  It would be more
confusing to talk about FPX regs while instead there are none.
I explained this in the comments.

I guess I can do the following if it helps.
#ifdef HAVE_PTRACE_GETFPXREGS
#define HAVE_PTRACE_GETVRREGS

Whatever I end up using it's partially going to be a lie.  I would
prefer using the VRREGS nomenclature where relevant, though.



Elena

 > -- 
 > Daniel Jacobowitz                           Carnegie Mellon University
 > MontaVista Software                         Debian GNU/Linux Developer