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

["H.J. Lu" <hjl.tools@gmail.com>]

On Sat, Mar 27, 2010 at 9:07 AM, Daniel Jacobowitz <dan@codesourcery.com> wrote:
> 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?

Yes,

> 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+".

I will make the change.

>> @@ -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.

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 =
>> +#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;

I will make the change.

>> -#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?

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.  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.
>

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.


-- 
H.J.