From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 637 invoked by alias); 12 Mar 2010 00:00:18 -0000 Received: (qmail 615 invoked by uid 22791); 12 Mar 2010 00:00:16 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00,SARE_MSGID_LONG40 X-Spam-Check-By: sourceware.org Received: from mail-ww0-f41.google.com (HELO mail-ww0-f41.google.com) (74.125.82.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 12 Mar 2010 00:00:08 +0000 Received: by wwe15 with SMTP id 15so512482wwe.0 for ; Thu, 11 Mar 2010 16:00:05 -0800 (PST) MIME-Version: 1.0 Received: by 10.216.86.145 with SMTP id w17mr199310wee.216.1268352005339; Thu, 11 Mar 2010 16:00:05 -0800 (PST) In-Reply-To: <201003112237.o2BMb4XR024283@glazunov.sibelius.xs4all.nl> References: <20100304180219.GA10826@intel.com> <20100304180408.GA10869@intel.com> <20100304180643.GB10869@intel.com> <20100306222037.GD21133@intel.com> <20100307213153.GA7170@intel.com> <201003112237.o2BMb4XR024283@glazunov.sibelius.xs4all.nl> Date: Fri, 12 Mar 2010 00:00:00 -0000 Message-ID: <6dc9ffc81003111600k66be499cqf6639a07d20f5cce@mail.gmail.com> Subject: Re: PATCH: 3/6 [2nd try]: Add AVX support (i386 changes) From: "H.J. Lu" To: Mark Kettenis Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes 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/msg00427.txt.bz2 On Thu, Mar 11, 2010 at 2:37 PM, Mark Kettenis wr= ote: >> 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. =A0Any comments? > > Can't find the time to review the complete diff. =A0So here's a partial > review. > > The generic -tdep.c bits look reasonable, although I have a few nits. =A0= I'm > less happy with the Linux -tdep.c and -nat.c bits though. > ... >> + >> +/* The extended state feature bits. =A0*/ >> +#define bit_I386_XSTATE_X87 =A0 =A0 =A0 =A0 =A0(1ULL << 0) >> +#define bit_I386_XSTATE_SSE =A0 =A0 =A0 =A0 =A0(1ULL << 1) >> +#define bit_I386_XSTATE_AVX =A0 =A0 =A0 =A0 =A0(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. =A0*/ >> +#define I386_XSTATE_SSE_MASK \ >> + =A0(bit_I386_XSTATE_X87 | bit_I386_XSTATE_SSE) >> +#define I386_XSTATE_AVX_MASK \ >> + =A0(I386_XSTATE_SSE_MASK | bit_I386_XSTATE_AVX) >> +#define I386_XSTATE_MAX_MASK \ >> + =A0I386_XSTATE_AVX_MASK >> + >> +#define I386_XSTATE_SSE_SIZE =A0 =A0 =A0 =A0 576 >> +#define I386_XSTATE_AVX_SIZE =A0 =A0 =A0 =A0 832 >> +#define I386_XSTATE_MAX_SIZE =A0 =A0 =A0 =A0 832 >> + >> +/* Get I386 XSAVE extended state size. =A0*/ >> +#define I386_XSTATE_SIZE(XCR0) =A0 =A0 =A0 \ >> + =A0(((XCR0) & bit_I386_XSTATE_AVX) !=3D 0 \ >> + =A0 ? I386_XSTATE_AVX_SIZE : I386_XSTATE_SSE_SIZE) >> + >> +#endif /* I386_XSTATE_H */ > > > Please don't introduce new nm-xxx.h files. =A0We'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 =A0 =A0 0x4204 >> +#endif >> + >> +#ifndef PTRACE_SETREGSET >> +#define PTRACE_SETREGSET =A0 =A0 0x4205 >> +#endif >> + >> +#endif =A0 =A0 =A0 /* NM_LINUX_XSTATE_H */ > > Do we really have to hardcode constants like this in GDB? =A0They should > be available in through kernel/libc headers. =A0Are 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. =A0We use array of int64 f= or >> + =A0 better alignment. =A0*/ >> +static unsigned int xstate_size_n_of_int64; > > Does alignment really matter? =A0I'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) >> +{ >> + =A0unsigned long long xstateregs[xstate_size_n_of_int64]; >> + =A0struct iovec iov; >> + >> + =A0if (!have_ptrace_getregset) >> + =A0 =A0return 0; >> + >> + =A0iov.iov_base =3D xstateregs; >> + =A0iov.iov_len =3D xstate_size; >> + =A0if (ptrace (PTRACE_GETREGSET, tid, (unsigned int) NT_X86_XSTATE, >> + =A0 =A0 =A0 =A0 =A0 (int) &iov) < 0) > > This can't be right! Why? That is the kernel interface in 2.6.34-rc1. >> + =A0 =A0perror_with_name (_("Couldn't read extended state status")); >> + >> + =A0i387_supply_xsave (regcache, -1, xstateregs); >> + =A0return 1; >> +} >> + >> +/* Store all valid registers in GDB's register array covered by the >> + =A0 PTRACE_SETREGSET request into the process/thread specified by TID. >> + =A0 Return non-zero if successful, zero otherwise. =A0*/ >> + >> +static int >> +store_xstateregs (const struct regcache *regcache, int tid, int regno) >> +{ >> + =A0unsigned 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_SIZ= E 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. >> + =A0struct iovec iov; >> + >> + =A0if (!have_ptrace_getregset) >> + =A0 =A0return 0; >> + >> + =A0iov.iov_base =3D xstateregs; >> + =A0iov.iov_len =3D xstate_size; >> + =A0if (ptrace (PTRACE_GETREGSET, tid, (unsigned int) NT_X86_XSTATE, >> + =A0 =A0 =A0 =A0 =A0 (int) &iov) < 0) >> + =A0 =A0perror_with_name (_("Couldn't read extended state status")); > > This can't be right either! > >> =A0 =A0 =A0 =A0if (store_fpxregs (regcache, tid, regno)) >> @@ -858,7 +943,49 @@ i386_linux_child_post_startup_inferior (ptid_t ptid) >> =A0static const struct target_desc * >> =A0i386_linux_read_description (struct target_ops *ops) >> =A0{ >> - =A0return tdesc_i386_linux; >> + =A0static unsigned long long xcr0; > > Is it really ok, to cache this? =A0Will 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. --=20 H.J.