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

["H.J. Lu" <hjl dot tools at gmail dot 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.