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

["H.J. Lu" <hjl.tools@gmail.com>]

On Thu, Mar 11, 2010 at 2:37 PM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>> Date: Sun, 7 Mar 2010 13:31:53 -0800
>> From: "H.J. Lu" <hongjiu.lu@intel.com>
>>
>> On Sat, Mar 06, 2010 at 02:20:37PM -0800, H.J. Lu wrote:
>> > Hi,
>> >
>> > Here are i386 changes to support AVX. OK to install?
>> >
>>
>> Here is the updated patch to change i386_dbx_reg_to_regnum to return
>> %ymmN register number for %xmmN if AVX is available.  Any comments?
>
> Can't find the time to review the complete diff.  So here's a partial
> review.
>
> The generic -tdep.c bits look reasonable, although I have a few nits.  I'm
> less happy with the Linux -tdep.c and -nat.c bits though.
>
...

>> +
>> +/* The extended state feature bits.  */
>> +#define bit_I386_XSTATE_X87          (1ULL << 0)
>> +#define bit_I386_XSTATE_SSE          (1ULL << 1)
>> +#define bit_I386_XSTATE_AVX          (1ULL << 2)
>
> #define's should be uppercase; please drop the bit_-prefix

I will make the change.

>> +
>> +/* Supported mask and size of the extended state.  */
>> +#define I386_XSTATE_SSE_MASK \
>> +  (bit_I386_XSTATE_X87 | bit_I386_XSTATE_SSE)
>> +#define I386_XSTATE_AVX_MASK \
>> +  (I386_XSTATE_SSE_MASK | bit_I386_XSTATE_AVX)
>> +#define I386_XSTATE_MAX_MASK \
>> +  I386_XSTATE_AVX_MASK
>> +
>> +#define I386_XSTATE_SSE_SIZE         576
>> +#define I386_XSTATE_AVX_SIZE         832
>> +#define I386_XSTATE_MAX_SIZE         832
>> +
>> +/* Get I386 XSAVE extended state size.  */
>> +#define I386_XSTATE_SIZE(XCR0)       \
>> +  (((XCR0) & bit_I386_XSTATE_AVX) != 0 \
>> +   ? I386_XSTATE_AVX_SIZE : I386_XSTATE_SSE_SIZE)
>> +
>> +#endif /* I386_XSTATE_H */
>
>
> Please don't introduce new nm-xxx.h files.  We've been trying to get
> rid of them for years and they shouldn't be necessary.

I will remove it.


>> +
>> +#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.


>> +
>> +/* 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.

>> +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.

>> +    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.

>> +  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)
>> +    perror_with_name (_("Couldn't read extended state status"));
>
> This can't be right either!
>

>>        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.

>
> Time for me to zzz now; hopefully I'll be able to review the remainder
> on Saturday.
>

Thanks.

-- 
H.J.