From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15153 invoked by alias); 27 Mar 2010 15:33:09 -0000 Received: (qmail 14975 invoked by uid 22791); 27 Mar 2010 15:33:08 -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-vw0-f41.google.com (HELO mail-vw0-f41.google.com) (209.85.212.41) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 27 Mar 2010 15:33:03 +0000 Received: by vws20 with SMTP id 20so5052315vws.0 for ; Sat, 27 Mar 2010 08:33:01 -0700 (PDT) MIME-Version: 1.0 Received: by 10.220.85.193 with HTTP; Sat, 27 Mar 2010 08:33:01 -0700 (PDT) In-Reply-To: <201003271454.o2REsujU007688@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> <6dc9ffc81003111600k66be499cqf6639a07d20f5cce@mail.gmail.com> <201003271454.o2REsujU007688@glazunov.sibelius.xs4all.nl> Date: Sat, 27 Mar 2010 15:33:00 -0000 Received: by 10.220.123.95 with SMTP id o31mr1642894vcr.23.1269703981610; Sat, 27 Mar 2010 08:33:01 -0700 (PDT) Message-ID: <6dc9ffc81003270833ha338c40pf9aece3d4f902ac6@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/msg00928.txt.bz2 On Sat, Mar 27, 2010 at 7:54 AM, Mark Kettenis wr= ote: >> Date: Thu, 11 Mar 2010 16:00:05 -0800 >> From: "H.J. Lu" >> >> >> + >> >> +#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 shou= ld >> > be available in through kernel/libc headers. =A0Are Drepper and Torval= ds >> > 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. =A0There are: >> >> #ifndef PTRACE_GET_THREAD_AREA >> #define PTRACE_GET_THREAD_AREA 25 >> =A0... >> #ifndef PTRACE_ARCH_PRCTL >> #define PTRACE_ARCH_PRCTL =A0 =A0 =A030 >> >> in amd64-linux-nat.c. > > Yes, we have done that in the past, but I think we should stop adding > #defines like that. AVX gdb support only needs PTRACE_GETREGSET/PTRACE_SETREGSET, which are fixed constants. I don't think we should require new kernel/glibc header files for AVX support. I can change it to #ifdef PTRACE_GETREGSET #if PTRACE_GETREGSET !=3D 0x4204 # error PTRACE_GETREGSET !=3D 0x4204 #endif #else #define PTRACE_GETREGSET 0x4204 #endif >> >> + >> >> +/* The extended state size in unit of int64. =A0We use array of int6= 4 for >> >> + =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. =A0It is nice to use array of uint64 to access = it. > > But there are also 32-bit, 128-bit and 256-bit fields in the xstate. > Therefore I think that typing it as an array of 64-bit values is > misleading. I will change 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. > > Well, at least your usage of casts here and further on in the code is > inconsistent. =A0But casting a pointer to an int acts as a red flag to > me. =A0Given that the userland prototype for ptrace(2) is: > > extern long int ptrace (enum __ptrace_request __request, ...) __THROW; > > I believe those casts shouldn't be necessary. I will change it. >> >> + =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 T= ID. >> >> + =A0 Return non-zero if successful, zero otherwise. =A0*/ >> >> + >> >> +static int >> >> +store_xstateregs (const struct regcache *regcache, int tid, int regn= o) >> >> +{ >> >> + =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. =A0Whatever 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. > > Yes, you'll need to raise I386_XSTATE_MAX_SIZE whenever the kernel > gains support for different/larger xstates. =A0But I don't see a problem > with that, since you'll have to make changes to GDB to support those > variants anyway. =A0That reminds me: I will remove I386_XSTATE_MAX_SIZE since it isn't needed by kernel. >> >> + =A0struct iovec iov; >> >> + >> >> + =A0if (!have_ptrace_getregset) >> >> + =A0 =A0return 0; >> >> + >> >> + =A0iov.iov_base =3D xstateregs; >> >> + =A0iov.iov_len =3D xstate_size; > > You probably should set iov.iov_len to sizeof(xstateregs) here. I will make the change. >> >> =A0 =A0 =A0 =A0if (store_fpxregs (regcache, tid, regno)) >> >> @@ -858,7 +943,49 @@ i386_linux_child_post_startup_inferior (ptid_t p= tid) >> >> =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. > > ok; but could you change this to uint64_t? > I will make the change. Thanks. --=20 H.J.