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

Mark Kettenis
2010-03-11T22:37:21Z

> 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;
> \f
>
> /* 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.