[v5,6/8] Create xml from target descriptions

This patch adds a print_xml_feature visitor class which turns a C
target description into xml. Both gdb and gdbserver can do this.

An accept function is added to gdbserver tdesc to allow it to use
vistor classes.

Tests are added to maintenance_check_xml_descriptions which takes
each pair of tested descriptions, turns them both into xml, then back
again, and confirms the descriptions still match.

Differences from V4 are:

print_xml_feature now uses a ptr.
tdesc xmltarget is now mutable.
Added maintenance_check_tdesc_xml_convert test function.
Improved string_read_description_xml.

This patch requires the next patch in the series, otherwise target
description will be rejected by gdb when using gdbserver. I'll make sure
the two patches get committed in a single commit.

Alan.

2018-04-10  Alan Hayward  <alan.hayward@arm.com>

gdb/
	* common/tdesc.c (print_xml_feature::visit_pre): Add xml parsing.
	(print_xml_feature::visit_post): Likewise.
	(print_xml_feature::visit): Likewise.
	* common/tdesc.h (tdesc_get_features_xml): Use const tdesc.
	(print_xml_feature): Add new class.
	* regformats/regdat.sh: Null xmltarget on feature targets.
	* target-descriptions.c (struct target_desc): Add xmltarget.
	(maintenance_check_tdesc_xml_convert): Add unittest function.
	(tdesc_get_features_xml): Add function to get xml.
	(maintenance_check_xml_descriptions): Test xml generation.
	* xml-tdesc.c (string_read_description_xml): Add function.
	* xml-tdesc.h (string_read_description_xml): Add declaration.

gdbserver/
	* gdb/gdbserver/server.c (get_features_xml): Remove cast.
	* tdesc.c (void target_desc::accept): Fill in function.
	(tdesc_get_features_xml): Remove old xml creation.
	(print_xml_feature::visit_pre): Add xml vistor.
	* tdesc.h (struct target_desc): Make xmltarget mutable.
	(tdesc_get_features_xml): Remove declaration.
---
 gdb/common/tdesc.c        | 108 ++++++++++++++++++++++++++++++++++++++++++++++
 gdb/common/tdesc.h        |  29 +++++++++++++
 gdb/gdbserver/server.c    |   2 +-
 gdb/gdbserver/tdesc.c     |  45 ++++++++-----------
 gdb/gdbserver/tdesc.h     |  10 ++---
 gdb/regformats/regdat.sh  |   4 +-
 gdb/target-descriptions.c |  56 ++++++++++++++++++++++++
 gdb/xml-tdesc.c           |  11 +++++
 gdb/xml-tdesc.h           |   5 +++
 9 files changed, 234 insertions(+), 36 deletions(-)

On 2018-04-10 10:33 AM, Alan Hayward wrote:
> This patch adds a print_xml_feature visitor class which turns a C
> target description into xml. Both gdb and gdbserver can do this.
> 
> An accept function is added to gdbserver tdesc to allow it to use
> vistor classes.
> 
> Tests are added to maintenance_check_xml_descriptions which takes
> each pair of tested descriptions, turns them both into xml, then back
> again, and confirms the descriptions still match.
> 
> Differences from V4 are:
> 
> print_xml_feature now uses a ptr.
> tdesc xmltarget is now mutable.
> Added maintenance_check_tdesc_xml_convert test function.
> Improved string_read_description_xml.
> 
> This patch requires the next patch in the series, otherwise target
> description will be rejected by gdb when using gdbserver. I'll make sure
> the two patches get committed in a single commit.

Hi Alan,

The patch looks good to me.  I noted a few formatting/minor comments.

> diff --git a/gdb/common/tdesc.c b/gdb/common/tdesc.c
> index b9e9ddb3fa..b4bd7bfb00 100644
> --- a/gdb/common/tdesc.c
> +++ b/gdb/common/tdesc.c
> @@ -290,3 +290,111 @@ tdesc_add_enum_value (tdesc_type_with_fields *type, int value,
>  			     tdesc_predefined_type (TDESC_TYPE_INT32),
>  			     value, -1);
>  }
> +
> +void print_xml_feature::visit_pre (const tdesc_feature *e)
> +{
> +  string_appendf (*m_buffer, "<feature name=\"%s\">\n", e->name.c_str ());
> +}
> +
> +void print_xml_feature::visit_post (const tdesc_feature *e)
> +{
> +  string_appendf (*m_buffer, "</feature>\n");
> +}
> +
> +void print_xml_feature::visit (const tdesc_type_builtin *t)
> +{
> +  error (_("xml output is not supported type \"%s\"."), t->name.c_str ());

Just a nit, but it seems to be missing something like a "for" between "supported" and "type".

> +}
> +
> +void print_xml_feature::visit (const tdesc_type_vector *t)
> +{
> +  string_appendf (*m_buffer, "<vector id=\"%s\" type=\"%s\" count=\"%d\"/>\n",
> +		  t->name.c_str (), t->element_type->name.c_str (), t->count);
> +}
> +
> +void print_xml_feature::visit (const tdesc_type_with_fields *t)
> +{
> +  struct tdesc_type_field *f;
> +  const static char *types[] = { "struct", "union", "flags", "enum" };
> +
> +  gdb_assert (t->kind >= TDESC_TYPE_STRUCT && t->kind <= TDESC_TYPE_ENUM);
> +
> +  string_appendf (*m_buffer,
> +		  "<%s id=\"%s\"", types[t->kind - TDESC_TYPE_STRUCT],
> +		  t->name.c_str ());
> +
> +  switch (t->kind)
> +    {
> +    case TDESC_TYPE_STRUCT:
> +    case TDESC_TYPE_FLAGS:
> +      if (t->size > 0)
> +	string_appendf (*m_buffer, " size=\"%d\"", t->size);
> +      string_appendf (*m_buffer, ">\n");
> +
> +      for (const tdesc_type_field &f : t->fields)
> +	{
> +	  string_appendf (*m_buffer, "  <field name=\"%s\" ", f.name.c_str ());
> +	  if (f.start == -1)
> +	    string_appendf (*m_buffer, "type=\"%s\"/>\n",
> +			    f.type->name.c_str ());
> +	  else
> +	    string_appendf (*m_buffer, "start=\"%d\" end=\"%d\"/>\n", f.start,
> +			    f.end);
> +	}
> +      break;
> +
> +    case TDESC_TYPE_ENUM:
> +      string_appendf (*m_buffer, ">\n");
> +      for (const tdesc_type_field &f : t->fields)
> +	string_appendf (*m_buffer, "  <field name=\"%s\" start=\"%d\"/>\n",
> +			f.name.c_str (), f.start);
> +      break;
> +
> +    case TDESC_TYPE_UNION:
> +      string_appendf (*m_buffer, ">\n");
> +      for (const tdesc_type_field &f : t->fields)
> +	string_appendf (*m_buffer, "  <field name=\"%s\" type=\"%s\"/>\n",
> +			f.name.c_str (), f.type->name.c_str ());
> +      break;
> +
> +    default:
> +      error (_("xml output is not supported type \"%s\"."), t->name.c_str ());

"supported for type" here too.

> +    }
> +
> +  string_appendf (*m_buffer, "</%s>\n", types[t->kind - TDESC_TYPE_STRUCT]);
> +}
> +
> +void print_xml_feature::visit (const tdesc_reg *r)
> +{
> +  string_appendf (*m_buffer,
> +		  "<reg name=\"%s\" bitsize=\"%d\" type=\"%s\" regnum=\"%ld\"",
> +		  r->name.c_str (), r->bitsize, r->type.c_str (),
> +		  r->target_regnum);
> +
> +  if (r->group.length () > 0)
> +    string_appendf (*m_buffer, " group=\"%s\"", r->group.c_str ());
> +
> +  if (r->save_restore == 0)
> +    string_appendf (*m_buffer, " save-restore=\"no\"");
> +
> +  string_appendf (*m_buffer, "/>\n");
> +}
> +
> +void print_xml_feature::visit_pre (const target_desc *e)
> +{
> +#ifndef IN_PROCESS_AGENT
> +  string_appendf (*m_buffer, "<?xml version=\"1.0\"?>\n");
> +  string_appendf (*m_buffer, "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">\n");
> +  string_appendf (*m_buffer, "<target>\n<architecture>%s</architecture>\n",
> +		  tdesc_architecture_name (e));
> +
> +  const char *osabi = tdesc_osabi_name (e);
> +  if (osabi != nullptr)
> +    string_appendf (*m_buffer, "<osabi>%s</osabi>", osabi);
> +#endif
> +}
> +
> +void print_xml_feature::visit_post (const target_desc *e)
> +{
> +  string_appendf (*m_buffer, "</target>\n");
> +}
> diff --git a/gdb/common/tdesc.h b/gdb/common/tdesc.h
> index 311341da0d..00eaada072 100644
> --- a/gdb/common/tdesc.h
> +++ b/gdb/common/tdesc.h
> @@ -381,4 +381,33 @@ void tdesc_create_reg (struct tdesc_feature *feature, const char *name,
>  		       int regnum, int save_restore, const char *group,
>  		       int bitsize, const char *type);
>  
> +/* Return the tdesc in string XML format.  */
> +
> +const char *tdesc_get_features_xml (const target_desc *tdesc);
> +
> +/* Print target description as xml.  */
> +
> +class print_xml_feature : public tdesc_element_visitor
> +{
> +public:
> +  print_xml_feature (std::string *buffer_)
> +    : m_buffer (buffer_)
> +  {}
> +
> +  ~print_xml_feature ()
> +  {}

You can remove the explicit destructor.

> diff --git a/gdb/gdbserver/tdesc.h b/gdb/gdbserver/tdesc.h
> index 197fb59127..2152b17947 100644
> --- a/gdb/gdbserver/tdesc.h
> +++ b/gdb/gdbserver/tdesc.h
> @@ -27,7 +27,7 @@
>  /* A target description.  Inherit from tdesc_feature so that target_desc
>     can be used as tdesc_feature.  */
>  
> -struct target_desc
> +struct target_desc : tdesc_element
>  {
>    /* A vector of elements of register definitions that
>       describe the inferior's register set.  */
> @@ -51,7 +51,7 @@ struct target_desc
>  
>       It can be NULL, then, its content is got from the following three
>       fields features, arch, and osabi in tdesc_get_features_xml.  */
> -  const char *xmltarget = NULL;
> +  mutable const char *xmltarget = NULL;

I think the comment above might need a bit of updating.

>  
>    /* The value of <architecture> element in the XML, replying GDB.  */
>    const char *arch = NULL;
> @@ -73,6 +73,8 @@ public:
>      return !(*this == other);
>    }
>  #endif
> +
> +  void accept (tdesc_element_visitor &v) const;

override

>  };
>  
>  /* Copy target description SRC to DEST.  */
> @@ -89,8 +91,4 @@ void init_target_desc (struct target_desc *tdesc);
>  
>  const struct target_desc *current_target_desc (void);
>  
> -#ifndef IN_PROCESS_AGENT
> -const char *tdesc_get_features_xml (struct target_desc *tdesc);
> -#endif
> -
>  #endif /* TDESC_H */
> diff --git a/gdb/regformats/regdat.sh b/gdb/regformats/regdat.sh
> index 18108d77eb..8f546fe276 100755
> --- a/gdb/regformats/regdat.sh
> +++ b/gdb/regformats/regdat.sh
> @@ -163,7 +163,9 @@ done
>  
>  echo
>  echo "static const char *expedite_regs_${name}[] = { \"`echo ${expedite} | sed 's/,/", "/g'`\", 0 };"
> -if test "${xmltarget}" = x; then
> +if test "${feature}" != x; then
> +  echo "static const char *xmltarget_${name} = 0;"
> +elif test "${xmltarget}" = x; then
>    if test "${xmlarch}" = x && test "${xmlosabi}" = x; then
>      echo "static const char *xmltarget_${name} = 0;"
>    else
> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
> index da2c1ce345..b96ef961a4 100644
> --- a/gdb/target-descriptions.c
> +++ b/gdb/target-descriptions.c
> @@ -333,6 +333,8 @@ struct target_desc : tdesc_element
>    /* The features associated with this target.  */
>    std::vector<tdesc_feature_up> features;
>  
> +  mutable char *xmltarget = nullptr;

Can you add a comment for this member?  Just to mention that this
is used for caching/memoization of the generated xml.

> +
>    void accept (tdesc_element_visitor &v) const override
>    {
>      v.visit_pre (this);
> @@ -1667,6 +1669,21 @@ private:
>    int m_next_regnum = 0;
>  };
>  
> +/* See common/tdesc.h.  */
> +
> +const char *
> +tdesc_get_features_xml (const target_desc *tdesc)
> +{
> +  if (tdesc->xmltarget == nullptr)
> +    {
> +      std::string buffer ("@");
> +      print_xml_feature v (&buffer);
> +      tdesc->accept (v);
> +      tdesc->xmltarget = xstrdup (buffer.c_str ());
> +    }
> +  return tdesc->xmltarget;
> +}
> +
>  static void
>  maint_print_c_tdesc_cmd (const char *args, int from_tty)
>  {
> @@ -1739,6 +1756,39 @@ record_xml_tdesc (const char *xml_file, const struct target_desc *tdesc)
>  
>  }
>  
> +/* Test the convesion process of a target description to/from xml: Take a target
> +   description TDESC, convert to xml, back to a description, and confirm the new
> +   tdesc is identical to the original.  */
> +static bool
> +maintenance_check_tdesc_xml_convert (const target_desc *tdesc, const char *name)
> +{
> +  const char *xml = tdesc_get_features_xml (tdesc);
> +
> +  if (xml == nullptr && *xml != '@')

This condition is wrong.  You probably meant

  if (xml == nullptr || *xml != '@')

?

> +    {
> +      printf_filtered (_("Could not convert description for %s to xml.\n"),
> +		       name);
> +      return false;
> +    }
> +
> +  const target_desc *tdesc_trans = string_read_description_xml (xml + 1);
> +
> +  if (tdesc_trans == nullptr)
> +    {
> +      printf_filtered (_("Could not convert description for %s from xml.\n"),
> +		       name);
> +      return false;
> +    }
> +  else if (*tdesc != *tdesc_trans)
> +    {
> +      printf_filtered (_("Converted description for %s does not match.\n"),
> +		       name);
> +      return false;
> +    }
> +  return true;
> +}
> +
> +
>  /* Check that the target descriptions created dynamically by
>     architecture-specific code equal the descriptions created from XML files
>     found in the specified directory DIR.  */
> @@ -1760,6 +1810,12 @@ maintenance_check_xml_descriptions (const char *dir, int from_tty)
>  	= file_read_description_xml (tdesc_xml.data ());
>  
>        if (tdesc == NULL || *tdesc != *e.second)
> +	{
> +	  printf_filtered ( _("Descriptions for %s do not match.\n"), e.first);
> +	  failed++;
> +	}
> +      else if (!maintenance_check_tdesc_xml_convert (tdesc, e.first)
> +	       || !maintenance_check_tdesc_xml_convert (e.second, e.first))
>  	failed++;
>      }
>    printf_filtered (_("Tested %lu XML files, %d failed\n"),
> diff --git a/gdb/xml-tdesc.c b/gdb/xml-tdesc.c
> index 9190d5f3c6..5c6ee4c8cd 100644
> --- a/gdb/xml-tdesc.c
> +++ b/gdb/xml-tdesc.c
> @@ -752,3 +752,14 @@ target_fetch_description_xml (struct target_ops *ops)
>    return output;
>  #endif
>  }
> +
> +/* See xml-tdesc.h.  */
> +
> +const struct target_desc *
> +string_read_description_xml (const char *xml_str)
> +{
> +  return tdesc_parse_xml (xml_str, [] (const char *href, void *baton) {
> +     error (_("xincludes are unsupported with this method"));
> +     return gdb::unique_xmalloc_ptr<char> ();
> +     }, nullptr);

You'll need to change this to

  return gdb::optional<gdb::char_vector> ();

because of a recent change upstream.  Also, this indentation would follow a bit
more what we do:

  return tdesc_parse_xml (xml_str, [] (const char *href, void *baton)
    {
      error (_("xincludes are unsupported with this method"));
      return gdb::optional<gdb::char_vector> ();
    }, nullptr);

The idea is to format the lambda the same way we would format an if or for body.

> +}
> \ No newline at end of file
> diff --git a/gdb/xml-tdesc.h b/gdb/xml-tdesc.h
> index 8f0679707a..e39d5580bb 100644
> --- a/gdb/xml-tdesc.h
> +++ b/gdb/xml-tdesc.h
> @@ -44,5 +44,10 @@ const struct target_desc *target_read_description_xml (struct target_ops *);
>     otherwise.  */
>  gdb::optional<std::string> target_fetch_description_xml (target_ops *ops);
>  
> +/* Take an xml string, parse it, and return the parsed description.  Does not
> +   handle a string containing includes.  */
> +
> +const struct target_desc *string_read_description_xml (const char *);

Can you give a name to the parameter?  "xml" would be fine.

Thanks,

Simon