From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8993 invoked by alias); 27 Mar 2010 16:09:32 -0000 Received: (qmail 8983 invoked by uid 22791); 27 Mar 2010 16:09:32 -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 16:09:28 +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 o2RG9M3q012961; Sat, 27 Mar 2010 17:09:22 +0100 (CET) Received: (from kettenis@localhost) by glazunov.sibelius.xs4all.nl (8.14.3/8.14.3/Submit) id o2RG9KrM007013; Sat, 27 Mar 2010 17:09:20 +0100 (CET) Date: Sat, 27 Mar 2010 16:09:00 -0000 Message-Id: <201003271609.o2RG9KrM007013@glazunov.sibelius.xs4all.nl> From: Mark Kettenis To: hjl.tools@gmail.com CC: gdb-patches@sourceware.org In-reply-to: <6dc9ffc81003270833ha338c40pf9aece3d4f902ac6@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> <201003271454.o2REsujU007688@glazunov.sibelius.xs4all.nl> <6dc9ffc81003270833ha338c40pf9aece3d4f902ac6@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/msg00932.txt.bz2 > Date: Sat, 27 Mar 2010 08:33:01 -0700 > From: "H.J. Lu" > >> 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. > > 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 != 0x4204 > # error PTRACE_GETREGSET != 0x4204 > #endif > #else > #define PTRACE_GETREGSET 0x4204 > #endif Ugh, no. That's even worse. Let's leave it as it was in your diff. > >> >> +    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: > > I will remove I386_XSTATE_MAX_SIZE since it isn't needed by kernel. Huh? You're missing the point here. GDB is supposed to be written in C90, which doesn't support variable-length arrays. So you need a compile-time constant to size the xstateregs array. And I386_XSTATE_MAX_SIZE fits the bill there perfectly.