From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6961 invoked by alias); 27 Mar 2010 16:07:14 -0000 Received: (qmail 6947 invoked by uid 22791); 27 Mar 2010 16:07:13 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 27 Mar 2010 16:07:10 +0000 Received: (qmail 23569 invoked from network); 27 Mar 2010 16:07:08 -0000 Received: from unknown (HELO caradoc.them.org) (dan@127.0.0.2) by mail.codesourcery.com with ESMTPA; 27 Mar 2010 16:07:08 -0000 Date: Sat, 27 Mar 2010 16:07:00 -0000 From: Daniel Jacobowitz To: "H.J. Lu" Cc: GDB Subject: Re: PATCH: 6/6 [2nd try]: Add AVX support (gdbserver changes) Message-ID: <20100327160705.GB16019@caradoc.them.org> Mail-Followup-To: "H.J. Lu" , GDB References: <20100304180219.GA10826@intel.com> <20100304180408.GA10869@intel.com> <20100304180748.GC10869@intel.com> <20100304180901.GD10869@intel.com> <20100304181003.GE10869@intel.com> <20100306222250.GG21133@intel.com> <20100312172541.GB6643@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20100312172541.GB6643@intel.com> User-Agent: Mutt/1.5.20 (2009-06-14) 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/msg00931.txt.bz2 On Fri, Mar 12, 2010 at 09:25:41AM -0800, H.J. Lu wrote: > On Sat, Mar 06, 2010 at 02:22:50PM -0800, H.J. Lu wrote: > > Hi, > > > > Here are gdbserver changes to support AVX. OK to install? > > > > Thanks. > > > > > > Here is the updated patch. Any comments/suggestions? I guess you haven't tested this one :-) You may want to add an AVX test to the testsuite, if it's not too much trouble. You're checking for the "x86=xml" feature in the target, but only calling the target method for "x86:xstate=...". I don't see how it could work. The problem we're solving by modifying qSupported is that older versions of GDB, which do not support XML registers at all, assume a specific layout for the g/G packet. Newer versions, which do support XML, will use whatever the target supplies. So, you only want the target to supply the registers via XML if GDB will understand them. Is that accurate? If that's the scope of the problem, then how about we handle this in a way we can reuse for other targets? That doesn't have to change the implementation; just rename the feature to "xmlRegisters+". > @@ -264,21 +292,28 @@ x86_store_fpxregset (struct regcache *regcache, const void *buf) > struct regset_info target_regsets[] = > { > #ifdef HAVE_PTRACE_GETREGS > - { PTRACE_GETREGS, PTRACE_SETREGS, sizeof (elf_gregset_t), > + { PTRACE_GETREGS, PTRACE_SETREGS, 0, sizeof (elf_gregset_t), > GENERAL_REGS, > x86_fill_gregset, x86_store_gregset }, > + { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_X86_XSTATE, 0, > +# ifdef __x86_64__ > + FP_REGS, > +# else > + EXTENDED_REGS, > +# endif > + x86_fill_xstateregset, x86_store_xstateregset }, What's this #ifdef for? I don't think anything checks FP_REGS vs EXTENDED_REGS. > +int use_xml = > +#ifdef USE_XML > + 1; > +#else > + 0; > +#endif > + I know this is just a style nit, but please do: #ifndef USE_XML # define USE_XML 0 #endif int use_xml = USE_XML; > -#ifdef USE_XML > - { > - extern const char *const xml_builtin[][2]; > - int i; > + if (use_xml) > + { > + extern const char *const xml_builtin[][2]; > + int i; > > - /* Look for the annex. */ > - for (i = 0; xml_builtin[i][0] != NULL; i++) > - if (strcmp (annex, xml_builtin[i][0]) == 0) > - break; > + /* Look for the annex. */ > + for (i = 0; xml_builtin[i][0] != NULL; i++) > + if (strcmp (annex, xml_builtin[i][0]) == 0) > + break; > > - if (xml_builtin[i][0] != NULL) > - return xml_builtin[i][1]; > - } > -#endif > + if (xml_builtin[i][0] != NULL) > + return xml_builtin[i][1]; > + } > > return NULL; > } Has anything arranged for xml_builtin to be defined if !defined(USE_XML)? That is what the #ifdef is actually for. I am not convinced any of the fiddling of use_xml is necessary or does what you want it to do. xml_builtin is for returning static files, i.e. those included using xi:include or referenced via setting gdbserver_xmltarget. The register cache files set gdbserver_xmltarget which is above this check. Have you tested gdbserver with and without AVX? What does it do? I think it'll work if you remove use_xml, and leave USE_XML alone. If GDB does not support XML, you can adjust gdbserver_xmltarget to report just the architecture and OSABI the way it did before you added register XML files. -- Daniel Jacobowitz CodeSourcery