From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10480 invoked by alias); 29 Nov 2001 18:40:24 -0000 Mailing-List: contact gdb-patches-help@sourceware.cygnus.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 9634 invoked from network); 29 Nov 2001 18:39:03 -0000 Received: from unknown (HELO cygnus.com) (205.180.230.5) by hostedprojects.ges.redhat.com with SMTP; 29 Nov 2001 18:39:03 -0000 Received: from cse.cygnus.com (cse.cygnus.com [205.180.230.236]) by runyon.cygnus.com (8.8.7-cygnus/8.8.7) with ESMTP id KAA16959; Thu, 29 Nov 2001 10:39:01 -0800 (PST) Received: (from kev@localhost) by cse.cygnus.com (8.9.3/8.9.3) id LAA18857; Thu, 29 Nov 2001 11:38:30 -0700 Date: Mon, 19 Nov 2001 15:15:00 -0000 From: Kevin Buettner Message-ID: <1011129183830.ZM18856@ocotillo.lan> In-Reply-To: Elena Zannoni "[RFA] W.I.P. AltiVec ppc registers support." (Nov 28, 9:15pm) References: <15365.39495.801289.497931@krustylu.cygnus.com> X-Mailer: Z-Mail (4.0.1 13Jan97 Caldera) To: Elena Zannoni , gdb-patches@sources.redhat.com Subject: Re: [RFA] W.I.P. AltiVec ppc registers support. MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-SW-Source: 2001-11/txt/msg00349.txt.bz2 Message-ID: <20011119151500.Cct0o2lNoILxSYvFqcyKEdg_Je32q16uG7gxHzu-piE@z> 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