From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19684 invoked by alias); 28 Mar 2010 01:11:39 -0000 Received: (qmail 19659 invoked by uid 22791); 28 Mar 2010 01:11:37 -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; Sun, 28 Mar 2010 01:11:33 +0000 Received: by vws20 with SMTP id 20so5182597vws.0 for ; Sat, 27 Mar 2010 18:11:31 -0700 (PDT) MIME-Version: 1.0 Received: by 10.220.90.201 with HTTP; Sat, 27 Mar 2010 18:11:31 -0700 (PDT) In-Reply-To: <20100327160705.GB16019@caradoc.them.org> 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> <20100327160705.GB16019@caradoc.them.org> Date: Sun, 28 Mar 2010 01:11:00 -0000 Received: by 10.220.4.23 with SMTP id 23mr1212273vcp.71.1269738691428; Sat, 27 Mar 2010 18:11:31 -0700 (PDT) Message-ID: <6dc9ffc81003271811t5be04ef9yf888edbce6d85236@mail.gmail.com> Subject: Re: PATCH: 6/6 [2nd try]: Add AVX support (gdbserver changes) From: "H.J. Lu" To: GDB 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/msg00936.txt.bz2 On Sat, Mar 27, 2010 at 9:07 AM, Daniel Jacobowitz w= rote: > 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. =A0OK to install? >> > >> > Thanks. >> > >> > >> >> Here is the updated patch. =A0Any comments/suggestions? > > I guess you haven't tested this one :-) =A0You may want to add an AVX > test to the testsuite, if it's not too much trouble. =A0You're checking > for the "x86=3Dxml" feature in the target, but only calling the target > method for "x86:xstate=3D...". =A0I 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. =A0Newer versions, which do > support XML, will use whatever the target supplies. =A0So, you only want > the target to supply the registers via XML if GDB will understand > them. =A0Is that accurate? Yes, > If that's the scope of the problem, then how about we handle > this in a way we can reuse for other targets? =A0That doesn't have > to change the implementation; just rename the feature to > "xmlRegisters+". I will make the change. >> @@ -264,21 +292,28 @@ x86_store_fpxregset (struct regcache *regcache, co= nst void *buf) >> =A0struct regset_info target_regsets[] =3D >> =A0{ >> =A0#ifdef HAVE_PTRACE_GETREGS >> - =A0{ PTRACE_GETREGS, PTRACE_SETREGS, sizeof (elf_gregset_t), >> + =A0{ PTRACE_GETREGS, PTRACE_SETREGS, 0, sizeof (elf_gregset_t), >> =A0 =A0 =A0GENERAL_REGS, >> =A0 =A0 =A0x86_fill_gregset, x86_store_gregset }, >> + =A0{ PTRACE_GETREGSET, PTRACE_SETREGSET, NT_X86_XSTATE, 0, >> +# ifdef __x86_64__ >> + =A0 =A0FP_REGS, >> +# else >> + =A0 =A0EXTENDED_REGS, >> +# endif >> + =A0 =A0x86_fill_xstateregset, x86_store_xstateregset }, > > What's this #ifdef for? =A0I don't think anything checks FP_REGS vs > EXTENDED_REGS. I just follow the current format where SSE register set is marked with EXTENDED_REGS for i386 and FP_REGS for x86-64. I don't mind changing it to either of them for both i386 and x86-64. Just let me know which one I should use. >> +int use_xml =3D >> +#ifdef USE_XML >> + =A01; >> +#else >> + =A00; >> +#endif >> + > > I know this is just a style nit, but please do: > > #ifndef USE_XML > # define USE_XML 0 > #endif > int use_xml =3D USE_XML; I will make the change. >> -#ifdef USE_XML >> - =A0{ >> - =A0 =A0extern const char *const xml_builtin[][2]; >> - =A0 =A0int i; >> + =A0if (use_xml) >> + =A0 =A0{ >> + =A0 =A0 =A0extern const char *const xml_builtin[][2]; >> + =A0 =A0 =A0int i; >> >> - =A0 =A0/* Look for the annex. =A0*/ >> - =A0 =A0for (i =3D 0; xml_builtin[i][0] !=3D NULL; i++) >> - =A0 =A0 =A0if (strcmp (annex, xml_builtin[i][0]) =3D=3D 0) >> - =A0 =A0 break; >> + =A0 =A0 =A0/* Look for the annex. =A0*/ >> + =A0 =A0 =A0for (i =3D 0; xml_builtin[i][0] !=3D NULL; i++) >> + =A0 =A0 if (strcmp (annex, xml_builtin[i][0]) =3D=3D 0) >> + =A0 =A0 =A0 break; >> >> - =A0 =A0if (xml_builtin[i][0] !=3D NULL) >> - =A0 =A0 =A0return xml_builtin[i][1]; >> - =A0} >> -#endif >> + =A0 =A0 =A0if (xml_builtin[i][0] !=3D NULL) >> + =A0 =A0 return xml_builtin[i][1]; >> + =A0 =A0} >> >> =A0 =A0return NULL; >> =A0} > > 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. =A0xml_builtin is for returning static files, > i.e. those included using xi:include or referenced via > setting gdbserver_xmltarget. =A0The register cache files set > gdbserver_xmltarget which is above this check. =A0Have you tested > gdbserver with and without AVX? =A0What does it do? Yes, I have tested them. The logic is in x86_linux_process_qsupported which will set XML target to AVX if AVX is supported. > I think it'll work if you remove use_xml, and leave USE_XML alone. =A0If > 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. > I don't know how gdbserver_xmltarget should be set if gdb doesn't support XML. My current approach is to turn off XML support at run-time even if USE_XML is 1 when gdb doesn't support XML. Can you show me some example to how to properly turn of XML via gdbserver_xmltarget? Thanks. --=20 H.J.