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

[Mark Kettenis <mark dot kettenis at xs4all dot nl>]

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

> 2010-03-07  H.J. Lu  <hongjiu.lu@intel.com>
> 
> 	* i386-linux-nat.c: Include "regset.h", "elf/common.h" and
> 	<sys/uio.h>.
> 	(xstate_size): New.
> 	(xstate_size_n_of_int64): Likewise.
> 	(fetch_xstateregs): Likewise.
> 	(store_xstateregs): Likewise.
> 	(GETXSTATEREGS_SUPPLIES): Likewise.
> 	(regmap): Include 8 upper YMM registers.
> 	(i386_linux_fetch_inferior_registers): Support XSAVE extended
> 	state.
> 	(i386_linux_store_inferior_registers): Likewise.
> 	(i386_linux_read_description): Check and enable AVX target
> 	descriptions.
> 
> 	* i386-linux-tdep.c: Include "regset.h", "i387-tdep.h",
> 	"i386-xstate.h" and "features/i386/i386-avx-linux.c".
> 	(i386_linux_regset_sections): Make it global.  Add
> 	".reg-xstate".
> 	(i386_linux_gregset_reg_offset): Include 8 upper YMM registers.
> 	(i386_linux_update_xstateregset): New.
> 	(i386_linux_core_read_xcr0): Likewise.
> 	(i386_linux_core_read_description): Check and enable AVX target
> 	description.
> 	(i386_linux_init_abi): Set xsave_xcr0_offset.
> 	(_initialize_i386_linux_tdep): Call
> 	initialize_tdesc_i386_avx_linux.
> 
> 	* i386-linux-tdep.h (I386_LINUX_ORIG_EAX_REGNUM): Replace
> 	I386_SSE_NUM_REGS with I386_AVX_NUM_REGS.
> 	(i386_linux_core_read_xcr0): New.
> 	(tdesc_i386_avx_linux): Likewise.
> 	(i386_linux_regset_sections): Likewise.
> 	(i386_linux_update_xstateregset): Likewise.
> 	(I386_LINUX_XSAVE_XCR0_OFFSET): Likewise.
> 
> 	* i386-tdep.c: Include "i386-xstate.h" and
> 	"features/i386/i386-avx.c".
> 	(i386_ymm_names): New.
> 	(i386_ymmh_names): Likewise.
> 	(i386_ymmh_regnum_p): Likewise.
> 	(i386_ymm_regnum_p): Likewise.
> 	(i386_xmm_regnum_p): Likewise.
> 	(i386_register_name): Likewise.
> 	(i386_ymm_type): Likewise.
> 	(i386_supply_xstateregset): Likewise.
> 	(i386_collect_xstateregset): Likewise.
> 	(i386_sse_regnum_p): Removed.
> 	(i386_pseudo_register_name): Support pseudo YMM registers.
> 	(i386_pseudo_register_type): Likewise.
> 	(i386_pseudo_register_read): Likewise.
> 	(i386_pseudo_register_write): Likewise.
> 	(i386_dbx_reg_to_regnum): Return %ymmN register number for
> 	%xmmN if AVX is available.
> 	(i386_regset_from_core_section): Support .reg-xstate section.
> 	(i386_register_reggroup_p): Supper upper YMM and YMM registers.
> 	(i386_validate_tdesc_p): Support org.gnu.gdb.i386.avx feature.
> 	Set ymmh_register_names, num_ymm_regs, ymm0h_regnum and xcr0.
> 	(i386_gdbarch_init): Set xstateregset.  Set xsave_xcr0_offset. 
> 	Call set_gdbarch_register_name.  Replace I386_SSE_NUM_REGS with
> 	I386_AVX_NUM_REGS.  Set ymmh_register_names, ymm0h_regnum and
> 	num_ymm_regs.  Add num_ymm_regs to set_gdbarch_num_pseudo_regs.
> 	Set ymm0_regnum.  Call set_gdbarch_qsupported.
> 	(_initialize_i386_tdep): Call initialize_tdesc_i386_avx.
> 
> 	* i386-tdep.h (gdbarch_tdep): Add xstateregset, ymm0_regnum,
> 	xcr0, xsave_xcr0_offset, ymm0h_regnum, ymmh_register_names and
> 	i386_ymm_type.
> 	(i386_regnum): Add I386_YMM0H_REGNUM, and I386_YMM7H_REGNUM.
> 	(I386_AVX_NUM_REGS): New.
> 	(i386_xmm_regnum_p): Likewise.
> 	(i386_ymm_regnum_p): Likewise.
> 	(i386_ymmh_regnum_p): Likewise.
> 
> 	* common/i386-xstate.h: New.
> 	* config/i386/nm-linux-xstate.h: Likewise.
> 	* config/i386/nm-linux64.h: Likewise.
> 
> 	* config/i386/linux64.mh (NAT_FILE): Set to nm-linux64.h.
> 
> 	* config/i386/nm-linux.h: Include "config/i386/nm-linux-xstate.h".
> 
> diff --git a/gdb/common/i386-xstate.h b/gdb/common/i386-xstate.h
> new file mode 100644
> index 0000000..3548103
> --- /dev/null
> +++ b/gdb/common/i386-xstate.h
> @@ -0,0 +1,45 @@
> +/* Common code for i386 XSAVE extended state.
> +
> +   Copyright (C) 2010 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef I386_XSTATE_H
> +#define I386_XSTATE_H 1
> +
> +/* 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

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

> diff --git a/gdb/config/i386/linux64.mh b/gdb/config/i386/linux64.mh
> index 19f3be0..99a5042 100644
> --- a/gdb/config/i386/linux64.mh
> +++ b/gdb/config/i386/linux64.mh
> @@ -2,7 +2,7 @@
>  NATDEPFILES= inf-ptrace.o fork-child.o \
>  	i386-nat.o amd64-nat.o amd64-linux-nat.o linux-nat.o \
>  	proc-service.o linux-thread-db.o linux-fork.o
> -NAT_FILE= config/nm-linux.h
> +NAT_FILE= nm-linux64.h
>  
>  # The dynamically loaded libthread_db needs access to symbols in the
>  # gdb executable.
> diff --git a/gdb/config/i386/nm-linux-xstate.h b/gdb/config/i386/nm-linux-xstate.h
> new file mode 100644
> index 0000000..0dbf9e5
> --- /dev/null
> +++ b/gdb/config/i386/nm-linux-xstate.h
> @@ -0,0 +1,33 @@
> +/* Native XSAVE extended state support for GNU/Linux x86.
> +
> +   Copyright 2010 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef	NM_LINUX_XSTATE_H
> +#define	NM_LINUX_XSTATE_H
> +
> +#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?

> diff --git a/gdb/i386-linux-nat.c b/gdb/i386-linux-nat.c
> index 31b9086..344c814 100644
> --- a/gdb/i386-linux-nat.c
> +++ b/gdb/i386-linux-nat.c
> @@ -23,11 +23,14 @@
>  #include "inferior.h"
>  #include "gdbcore.h"
>  #include "regcache.h"
> +#include "regset.h"
>  #include "target.h"
>  #include "linux-nat.h"
>  
>  #include "gdb_assert.h"
>  #include "gdb_string.h"
> +#include "elf/common.h"
> +#include <sys/uio.h>
>  #include <sys/ptrace.h>
>  #include <sys/user.h>
>  #include <sys/procfs.h>
> @@ -69,6 +72,16 @@
>  
>  /* Defines ps_err_e, struct ps_prochandle.  */
>  #include "gdb_proc_service.h"
> +
> +/* The extended state size in bytes.  */
> +static unsigned int xstate_size;
> +
> +/* 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.

> +/* Does the current host support PTRACE_GETREGSET?  */
> +static int have_ptrace_getregset = -1;
>  
>  
>  /* The register sets used in GNU/Linux ELF core-dumps are identical to
> @@ -98,6 +111,8 @@ static int regmap[] =
>    -1, -1, -1, -1,		/* xmm0, xmm1, xmm2, xmm3 */
>    -1, -1, -1, -1,		/* xmm4, xmm5, xmm6, xmm6 */
>    -1,				/* mxcsr */
> +  -1, -1, -1, -1,		/* ymm0h, ymm1h, ymm2h, ymm3h */
> +  -1, -1, -1, -1,		/* ymm4h, ymm5h, ymm6h, ymm6h */
>    ORIG_EAX
>  };
>  
> @@ -110,6 +125,9 @@ static int regmap[] =
>  #define GETFPXREGS_SUPPLIES(regno) \
>    (I386_ST0_REGNUM <= (regno) && (regno) < I386_SSE_NUM_REGS)
>  
> +#define GETXSTATEREGS_SUPPLIES(regno) \
> +  (I386_ST0_REGNUM <= (regno) && (regno) < I386_AVX_NUM_REGS)
> +
>  /* Does the current host support the GETREGS request?  */
>  int have_ptrace_getregs =
>  #ifdef HAVE_PTRACE_GETREGS
> @@ -355,6 +373,57 @@ static void store_fpregs (const struct regcache *regcache, int tid, int regno) {
>  
>  /* Transfering floating-point and SSE registers to and from GDB.  */
>  
> +/* Fetch all registers covered by the PTRACE_GETREGSET request from
> +   process/thread TID and store their values in GDB's register array.
> +   Return non-zero if successful, zero otherwise.  */
> +
> +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!

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

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

> +  i387_collect_xsave (regcache, regno, xstateregs, 0);
> +
> +  if (ptrace (PTRACE_SETREGSET, tid, (unsigned int) NT_X86_XSTATE,
> +	      (int) &iov) < 0)
> +    perror_with_name (_("Couldn't write extended state status"));
> +
> +  return 1;
> +}
> +
>  #ifdef HAVE_PTRACE_GETFPXREGS
>  
>  /* Fill GDB's register array with the floating-point and SSE register
> @@ -489,6 +558,8 @@ i386_linux_fetch_inferior_registers (struct target_ops *ops,
>  	  return;
>  	}
>  
> +      if (fetch_xstateregs (regcache, tid))
> +	return;
>        if (fetch_fpxregs (regcache, tid))
>  	return;
>        fetch_fpregs (regcache, tid);
> @@ -501,6 +572,12 @@ i386_linux_fetch_inferior_registers (struct target_ops *ops,
>        return;
>      }
>  
> +  if (GETXSTATEREGS_SUPPLIES (regno))
> +    {
> +      if (fetch_xstateregs (regcache, tid))
> +	return;
> +    }
> +
>    if (GETFPXREGS_SUPPLIES (regno))
>      {
>        if (fetch_fpxregs (regcache, tid))
> @@ -553,6 +630,8 @@ i386_linux_store_inferior_registers (struct target_ops *ops,
>    if (regno == -1)
>      {
>        store_regs (regcache, tid, regno);
> +      if (store_xstateregs (regcache, tid, regno))
> +	return;
>        if (store_fpxregs (regcache, tid, regno))
>  	return;
>        store_fpregs (regcache, tid, regno);
> @@ -565,6 +644,12 @@ i386_linux_store_inferior_registers (struct target_ops *ops,
>        return;
>      }
>  
> +  if (GETXSTATEREGS_SUPPLIES (regno))
> +    {
> +      if (store_xstateregs (regcache, tid, regno))
> +	return;
> +    }
> +
>    if (GETFPXREGS_SUPPLIES (regno))
>      {
>        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?

> +  if (have_ptrace_getregset == -1)
> +    {
> +      int tid;
> +      unsigned long long xstateregs[(I386_XSTATE_SSE_SIZE
> +				     / sizeof (long long))];
> +      struct iovec iov;
> +
> +      /* GNU/Linux LWP ID's are process ID's.  */
> +      tid = TIDGET (inferior_ptid);
> +      if (tid == 0)
> +	tid = PIDGET (inferior_ptid); /* Not a threaded program.  */
> +
> +      iov.iov_base = xstateregs;
> +      iov.iov_len = I386_XSTATE_SSE_SIZE;
> +
> +      /* Check if PTRACE_GETREGSET works.  */
> +      if (ptrace (PTRACE_GETREGSET, tid,
> +		  (unsigned int) NT_X86_XSTATE, (long) &iov) < 0)
> +	have_ptrace_getregset = 0;
> +      else
> +	{
> +	  have_ptrace_getregset = 1;
> +
> +	  /* Get XCR0 from XSAVE extended state.  */
> +	  xcr0 = xstateregs[(I386_LINUX_XSAVE_XCR0_OFFSET
> +			     / sizeof (long long))];
> +
> +	  xstate_size = I386_XSTATE_SIZE (xcr0);
> +	  xstate_size_n_of_int64 = xstate_size / sizeof (long long);
> +	}
> +
> +      i386_linux_update_xstateregset (i386_linux_regset_sections,
> +				      xstate_size);
> +    }
> +
> +  /* Check the native XCR0 only if PTRACE_GETREGSET is available.  */
> +  if (have_ptrace_getregset
> +      && (xcr0 & I386_XSTATE_AVX_MASK) == I386_XSTATE_AVX_MASK)
> +    return tdesc_i386_avx_linux;
> +  else
> +    return tdesc_i386_linux;
>  }
>  
>  void

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