From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17515 invoked by alias); 27 Mar 2010 14:55:09 -0000 Received: (qmail 17503 invoked by uid 22791); 27 Mar 2010 14:55:07 -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; Sat, 27 Mar 2010 14:55:03 +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 o2REsvqq018410; Sat, 27 Mar 2010 15:54:57 +0100 (CET) Received: (from kettenis@localhost) by glazunov.sibelius.xs4all.nl (8.14.3/8.14.3/Submit) id o2REsujU007688; Sat, 27 Mar 2010 15:54:56 +0100 (CET) Date: Sat, 27 Mar 2010 14:55:00 -0000 Message-Id: <201003271454.o2REsujU007688@glazunov.sibelius.xs4all.nl> From: Mark Kettenis To: hjl.tools@gmail.com CC: gdb-patches@sourceware.org In-reply-to: <6dc9ffc81003111600k66be499cqf6639a07d20f5cce@mail.gmail.com> (hjl.tools@gmail.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> <201003112237.o2BMb4XR024283@glazunov.sibelius.xs4all.nl> <6dc9ffc81003111600k66be499cqf6639a07d20f5cce@mail.gmail.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/msg00924.txt.bz2 > Date: Thu, 11 Mar 2010 16:00:05 -0800 > From: "H.J. Lu" > > >> + > >> +#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? > > 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. Yes, we have done that in the past, but I think we should stop adding #defines like that. > >> + > >> +/* 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. > > "xcr0" is a 64bit value. It 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. > >> +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! > > 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. But casting a pointer to an int acts as a red flag to me. Given 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. > >> +    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. > > That is how the kernel interface works. Whatever 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. But I don't see a problem with that, since you'll have to make changes to GDB to support those variants anyway. That reminds me: > >> +  struct iovec iov; > >> + > >> +  if (!have_ptrace_getregset) > >> +    return 0; > >> + > >> +  iov.iov_base = xstateregs; > >> +  iov.iov_len = xstate_size; You probably should set iov.iov_len to sizeof(xstateregs) here. > >>        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? > > xcr0 is a processor value and will be the same for all processes. ok; but could you change this to uint64_t?