Re: [PATCH v2 0/8] Remove XML files from gdbserver

[Alan Hayward <Alan.Hayward@arm.com>]

> On 29 Jan 2018, at 18:18, Pedro Alves <palves@redhat.com> wrote:
> 
> On 01/24/2018 09:26 AM, Alan Hayward wrote:
>> In exisiting code, gdbserver uses C code auto generated from xml files to
>> create target descriptions. When sending an xml description to GDB, it
>> creates an xml containing just the name of the original xml file.
>> Upon receipt, GDB reads and parses a local copy of xml file.
> 
> Huh?  What do you mean reads and parses a _local copy_?
> That's false AFAICS.
> 
> GDB fetches named file from the server, not from the local filesystem.
> E.g., on x86-64:
> 
> Sending packet: $qXfer:features:read:target.xml:0,fff#7d...Packet received: l<?xml version="1.0"?><!DOCTYPE target SYSTEM "gdb-target.dtd"><target><architecture>i386:x86-64</architecture><osabi>GNU/Linux</osabi><xi:include href="64bit-core.xml"/><xi:include href="64bit-sse.xml"/><xi:include href="64bit-linux.xml"/><xi:include href="64bit-segments.xml"/><xi:include href="64bit-avx.xml"/></target>
> Sending packet: $qXfer:features:read:64bit-core.xml:0,fff#75...Packet received: l<?xml version="1.0"?>\n<!-- Copyright (C) 2010-2018 Free Software Foundation, Inc.\n\n     Copying and distribution of this file, with or without modification,\n     are permitted in any medium without royalty provided the copyright\n     notice and this notice are preserved.  -->\n\n<!DOCTYPE feature SYSTEM "gdb-target.dtd">\n<feature name="org.gnu.gdb.i386.core">\n  <flags id="i386_eflags" size="4">\n    <field name="CF" start="0" end="0"/>\n    <field name="" start="1" end="1"/>\n    <field name="PF" start="2" end=[12 bytes omitted]

Yes, I misunderstood what was happening here.
(The review of the final patch of v1 cleared this up for me, but I never updated
 this part of the abstract for v2).

Instead I should have said something like:

In exisiting code, gdbserver uses C code auto generated from xml files to
create target descriptions. When sending an xml description to GDB, it
creates an xml containing the name of the original xml file(s).
GDB parses the xml and then requests from gdbserver a copy of each of
the included files.
With this new patch, we add common code that allows gdbserver (and gdb)
to turn a C target description structure into xml. Now when sending an xml
description to gdb, gdbserver creates a single xml structure containing the
entire target description, without any includes. GDB no longer needs to ask
for additional xml packets from gdbserver.

> 
> Above we see GDB processing the xi:includes by subsequently fetching the
> xi:included files from the server.
> 
> There is already some support in gdbserver for baking in the XML
> code directly in gdbserver instead of reading it from (gdbserver's)
> filesystem:
> 
>  /* `desc->xmltarget' defines what to return when looking for the
>     "target.xml" file.  Its contents can either be verbatim XML code
>     (prefixed with a '@') or else the name of the actual XML file to
>     be used in place of "target.xml".
> 
>     This variable is set up from the auto-generated
>     init_registers_... routine for the current target.  */
> 
>  if (strcmp (annex, "target.xml") == 0)
>    {
>      const char *ret = tdesc_get_features_xml ((target_desc*) desc);
> 
>      if (*ret == '@')
> 	return ret + 1;
>      else
> 	annex = ret;
>    }

Essentially, tdesc_get_features_xml() is the function I am rewriting with this series.

In the existing code it will produce something like:

<!DOCTYPE target SYSTEM "gdb-target.dtd">
<target>
  <architecture>i386</architecture>
  <osabi>GNU/Linux</osabi>
  <xi:include href="32bit-core.xml"/>
  <xi:include href="32bit-sse.xml"/>
  <xi:include href="32bit-linux.xml"/>
  <xi:include href="32bit-avx.xml"/>
</target>

My patch removes the include sections and instead dumps the entire xml:

<!DOCTYPE target SYSTEM "gdb-target.dtd">
<target>
  <architecture>i386</architecture>
  <osabi>GNU/Linux</osabi>
  <feature name="org.gnu.gdb.i386.core">
    <flags id="i386_eflags" size="4">
      <field name="CF" start="0" end="0"/>
      <field name="" start="1" end="1"/>
      <field name="PF" start="2" end="2"/>
      <field name="AF" start="4" end="4"/>
...etc 

> 
> Can you please clarify the rationale for the series?
> 
> The current justification doesn't look very convincing to me.  This
> is going to be probably something around dynamic generation of XML
> descriptions based on CPU optional features, instead of having
> to have gdbserver carry a bunch of different XML files for each possible
> combination of optional features.  (I.e., a continuation of Yao's
> earlier work).

The reason for me writing the patch series is eventually to support aarch64 sve.
With sve I want to avoid creating an xml file for every sve size possibility.
With this patch in place, I can avoid creating xml files for sve and instead write
the .c code (that would normally be generated from the xml)
Maybe I should have mentioned that in the abstract, but I wanted to avoid binding
this patch to future work.

The rational for this patch then is that is removes the need for gdbserver to include
xml files and it means a given target can have just a .c file in features/ instead of
an xml and generated .c file.

> 
> I haven't read the patches, but it's likely that you should need
> to tweak the individual patches' rationales too.

I’ll look through them.

Thanks for the review!

> 
> Thanks,
> Pedro Alves