This is the mail archive of the
gdb-patches@sources.redhat.com
mailing list for the GDB project.
Re: [RFA] W.I.P. AltiVec ppc registers support.
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