Re: PATCH: 6/6 [2nd try]: Add AVX support (gdbserver changes)

[Daniel Jacobowitz <dan@codesourcery.com>]

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