Re: PATCH: 3/6 [2nd try]: Add AVX support (i386 changes)

[Mark Kettenis <mark.kettenis@xs4all.nl>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 4271 bytes --]

> Date: Thu, 11 Mar 2010 16:00:05 -0800
> From: "H.J. Lu" <hjl.tools@gmail.com>
>
> >> +
> >> +#include "i386-xstate.h"
> >> +
> >> +#ifndef PTRACE_GETREGSET
> >> +#define PTRACE_GETREGSET     0x4204
> >> +#endif
> >> +
> >> +#ifndef PTRACE_SETREGSET
> >> +#define PTRACE_SETREGSET     0x4205
> >> +#endif
> >> +
> >> +#endif       /* NM_LINUX_XSTATE_H */
> >
> > Do we really have to hardcode constants like this in GDB?  They should
> > be available in through kernel/libc headers.  Are Drepper and Torvalds
> > still fighting over that issue?
> 
> They are in Linux kernel 2.6.34-rc1. Do we enable gdb support only
> with the new kernel/glibc headers? I compiled gdb on RHEL4 and it
> works fine.  There are:
> 
> #ifndef PTRACE_GET_THREAD_AREA
> #define PTRACE_GET_THREAD_AREA 25
>  ...
> #ifndef PTRACE_ARCH_PRCTL
> #define PTRACE_ARCH_PRCTL      30
> 
> in amd64-linux-nat.c.

Yes, we have done that in the past, but I think we should stop adding
#defines like that.  

> >> +
> >> +/* The extended state size in unit of int64.  We use array of int64 for
> >> +   better alignment.  */
> >> +static unsigned int xstate_size_n_of_int64;
> >
> > Does alignment really matter?  I'd rather do without this additional
> > complication.
> 
> "xcr0" is a 64bit value.  It is nice to use array of uint64 to access it.

But there are also 32-bit, 128-bit and 256-bit fields in the xstate.
Therefore I think that typing it as an array of 64-bit values is
misleading.

> >> +static int
> >> +fetch_xstateregs (struct regcache *regcache, int tid)
> >> +{
> >> +  unsigned long long xstateregs[xstate_size_n_of_int64];
> >> +  struct iovec iov;
> >> +
> >> +  if (!have_ptrace_getregset)
> >> +    return 0;
> >> +
> >> +  iov.iov_base = xstateregs;
> >> +  iov.iov_len = xstate_size;
> >> +  if (ptrace (PTRACE_GETREGSET, tid, (unsigned int) NT_X86_XSTATE,
> >> +           (int) &iov) < 0)
> >
> > This can't be right!
> 
> Why? That is the kernel interface in 2.6.34-rc1.

Well, at least your usage of casts here and further on in the code is
inconsistent.  But casting a pointer to an int acts as a red flag to
me.  Given that the userland prototype for ptrace(2) is:

extern long int ptrace (enum __ptrace_request __request, ...) __THROW;

I believe those casts shouldn't be necessary.

> >> +    perror_with_name (_("Couldn't read extended state status"));
> >> +
> >> +  i387_supply_xsave (regcache, -1, xstateregs);
> >> +  return 1;
> >> +}
> >> +
> >> +/* Store all valid registers in GDB's register array covered by the
> >> +   PTRACE_SETREGSET request into the process/thread specified by TID.
> >> +   Return non-zero if successful, zero otherwise.  */
> >> +
> >> +static int
> >> +store_xstateregs (const struct regcache *regcache, int tid, int regno)
> >> +{
> >> +  unsigned long long xstateregs[xstate_size_n_of_int64];
> >
> > I think it is better to use I386_XSTATE_MAX_SIZE here.
> 
> That is how the kernel interface works.  Whatever value
> I386_XSTATE_MAX_SIZE is today won't be the same tomorrow. We will
> increase it in the coming years. But the same gdb binary will work
> fine since kernel will only copy number of bytes specified in
> iov.iov_len, which is all gdb cares/needs.

Yes, you'll need to raise I386_XSTATE_MAX_SIZE whenever the kernel
gains support for different/larger xstates.  But I don't see a problem
with that, since you'll have to make changes to GDB to support those
variants anyway.  That reminds me:

> >> +  struct iovec iov;
> >> +
> >> +  if (!have_ptrace_getregset)
> >> +    return 0;
> >> +
> >> +  iov.iov_base = xstateregs;
> >> +  iov.iov_len = xstate_size;

You probably should set iov.iov_len to sizeof(xstateregs) here.

> >>        if (store_fpxregs (regcache, tid, regno))
> >> @@ -858,7 +943,49 @@ i386_linux_child_post_startup_inferior (ptid_t ptid)
> >>  static const struct target_desc *
> >>  i386_linux_read_description (struct target_ops *ops)
> >>  {
> >> -  return tdesc_i386_linux;
> >> +  static unsigned long long xcr0;
> >
> > Is it really ok, to cache this?  Will the Linux kernel always return
> > the same value for every process?
> 
> xcr0 is a processor value and will be the same for all processes.

ok; but could you change this to uint64_t?