From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25223 invoked by alias); 11 Mar 2010 22:37:21 -0000 Received: (qmail 25202 invoked by uid 22791); 11 Mar 2010 22:37:17 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from sibelius.xs4all.nl (HELO glazunov.sibelius.xs4all.nl) (83.163.83.176) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 11 Mar 2010 22:37:11 +0000 Received: from glazunov.sibelius.xs4all.nl (kettenis@localhost [127.0.0.1]) by glazunov.sibelius.xs4all.nl (8.14.3/8.14.3) with ESMTP id o2BMb64l027550; Thu, 11 Mar 2010 23:37:06 +0100 (CET) Received: (from kettenis@localhost) by glazunov.sibelius.xs4all.nl (8.14.3/8.14.3/Submit) id o2BMb4XR024283; Thu, 11 Mar 2010 23:37:04 +0100 (CET) Date: Thu, 11 Mar 2010 22:37:00 -0000 Message-Id: <201003112237.o2BMb4XR024283@glazunov.sibelius.xs4all.nl> From: Mark Kettenis To: hjl.tools@gmail.com CC: gdb-patches@sourceware.org In-reply-to: <20100307213153.GA7170@intel.com> (hongjiu.lu@intel.com) Subject: Re: PATCH: 3/6 [2nd try]: Add AVX support (i386 changes) References: <20100304180219.GA10826@intel.com> <20100304180408.GA10869@intel.com> <20100304180643.GB10869@intel.com> <20100306222037.GD21133@intel.com> <20100307213153.GA7170@intel.com> Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-03/txt/msg00425.txt.bz2 > Date: Sun, 7 Mar 2010 13:31:53 -0800 > From: "H.J. Lu" > > 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 > > * i386-linux-nat.c: Include "regset.h", "elf/common.h" and > . > (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 . */ > + > +#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 . */ > + > +#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 > #include > #include > #include > @@ -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.