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: 3176 bytes --]

> Date: Sat, 27 Mar 2010 08:33:01 -0700
> From: "H.J. Lu" <hjl.tools@gmail.com>
> >> 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.
> 
> AVX gdb support only needs PTRACE_GETREGSET/PTRACE_SETREGSET,
> which are fixed constants. I don't think we should require new kernel/glibc
> header files for AVX support. I can change it to
> 
> #ifdef PTRACE_GETREGSET
> #if PTRACE_GETREGSET != 0x4204
> # error PTRACE_GETREGSET != 0x4204
> #endif
> #else
> #define PTRACE_GETREGSET        0x4204
> #endif

Ugh, no.  That's even worse.  Let's leave it as it was in your diff.

> >> >> +    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:
> 
> I will remove I386_XSTATE_MAX_SIZE since it isn't needed by kernel.

Huh?  You're missing the point here.  GDB is supposed to be written in
C90, which doesn't support variable-length arrays.  So you need a
compile-time constant to size the xstateregs array.  And
I386_XSTATE_MAX_SIZE fits the bill there perfectly.