From mboxrd@z Thu Jan 1 00:00:00 1970 From: Elena Zannoni To: Kevin Buettner Cc: Elena Zannoni , gdb-patches@sources.redhat.com Subject: Re: [RFA] W.I.P. AltiVec ppc registers support. Date: Thu, 29 Nov 2001 13:53:00 -0000 Message-ID: <15366.44991.616576.411278@krustylu.cygnus.com> References: <15365.39495.801289.497931@krustylu.cygnus.com> <1011129183830.ZM18856@ocotillo.lan> X-SW-Source: 2001-11/msg00569.html Message-ID: <20011129135300.hxOZWRfQkFcdbtKx94FJUC5jOIRmxdcxyRbuV0J9Kno@z> 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