Re: [PATCH v5 6/8] Create xml from target descriptions

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

> On 18 Apr 2018, at 03:43, Simon Marchi <simark@simark.ca> wrote:
> 
> Hi Alan,
> 
> The patch looks good to me.  I noted a few formatting/minor comments.

Thanks!
Updated as requested and pushed. New version included at the bottom for reference.


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

diff --git a/gdb/common/tdesc.c b/gdb/common/tdesc.c
index b9e9ddb3fa..68584a7f94 100644
--- a/gdb/common/tdesc.c
+++ b/gdb/common/tdesc.c
@@ -290,3 +290,112 @@ 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 for type \"%s\"."), t->name.c_str ());
+}
+
+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 for type \"%s\"."),
+	     t->name.c_str ());
+    }
+
+  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 8b826ec436..6868bf47d9 100644
--- a/gdb/common/tdesc.h
+++ b/gdb/common/tdesc.h
@@ -380,4 +380,30 @@ 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_)
+  {}
+
+  void visit_pre (const target_desc *e) override;
+  void visit_post (const target_desc *e) override;
+  void visit_pre (const tdesc_feature *e) override;
+  void visit_post (const tdesc_feature *e) override;
+  void visit (const tdesc_type_builtin *type) override;
+  void visit (const tdesc_type_vector *type) override;
+  void visit (const tdesc_type_with_fields *type) override;
+  void visit (const tdesc_reg *reg) override;
+
+private:
+  std::string *m_buffer;
+};
+
 #endif /* ARCH_TDESC_H */
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 64c72bdd58..5027df5e10 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -940,7 +940,7 @@ get_features_xml (const char *annex)

   if (strcmp (annex, "target.xml") == 0)
     {
-      const char *ret = tdesc_get_features_xml ((target_desc*) desc);
+      const char *ret = tdesc_get_features_xml (desc);

       if (*ret == '@')
 	return ret + 1;
diff --git a/gdb/gdbserver/tdesc.c b/gdb/gdbserver/tdesc.c
index 7603a90a59..126589f3e3 100644
--- a/gdb/gdbserver/tdesc.c
+++ b/gdb/gdbserver/tdesc.c
@@ -47,6 +47,18 @@ bool target_desc::operator== (const target_desc &other) const

 #endif

+void target_desc::accept (tdesc_element_visitor &v) const
+{
+#ifndef IN_PROCESS_AGENT
+  v.visit_pre (this);
+
+  for (const tdesc_feature_up &feature : features)
+    feature->accept (v);
+
+  v.visit_post (this);
+#endif
+}
+
 void
 init_target_desc (struct target_desc *tdesc)
 {
@@ -138,11 +150,10 @@ set_tdesc_osabi (struct target_desc *target_desc, const char *name)
   target_desc->osabi = xstrdup (name);
 }

-/* Return a string which is of XML format, including XML target
-   description to be sent to GDB.  */
+/* See common/tdesc.h.  */

 const char *
-tdesc_get_features_xml (target_desc *tdesc)
+tdesc_get_features_xml (const target_desc *tdesc)
 {
   /* Either .xmltarget or .features is not NULL.  */
   gdb_assert (tdesc->xmltarget != NULL
@@ -151,31 +162,9 @@ tdesc_get_features_xml (target_desc *tdesc)

   if (tdesc->xmltarget == NULL)
     {
-      std::string buffer ("@<?xml version=\"1.0\"?>");
-
-      buffer += "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">";
-      buffer += "<target>";
-      buffer += "<architecture>";
-      buffer += tdesc_architecture_name (tdesc);
-      buffer += "</architecture>";
-
-      const char *osabi = tdesc_osabi_name (tdesc);
-      if (osabi != nullptr)
-	{
-	  buffer += "<osabi>";
-	  buffer += osabi;
-	  buffer += "</osabi>";
-	}
-
-      for (const tdesc_feature_up &feature : tdesc->features)
-	{
-	  buffer += "<xi:include href=\"";
-	  buffer += feature->name;
-	  buffer += "\"/>";
-	}
-
-      buffer += "</target>";
-
+      std::string buffer ("@");
+      print_xml_feature v (&buffer);
+      tdesc->accept (v);
       tdesc->xmltarget = xstrdup (buffer.c_str ());
     }

diff --git a/gdb/gdbserver/tdesc.h b/gdb/gdbserver/tdesc.h
index 197fb59127..61a3e4ecdd 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.  */
@@ -49,9 +49,9 @@ struct target_desc
      verbatim XML code (prefixed with a '@') or else the name of the
      actual XML file to be used in place of "target.xml".

-     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;
+     If NULL then its content will be generated by parsing the target
+     description into xml.  */
+  mutable const char *xmltarget = NULL;

   /* 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..c0f85112b1 100644
--- a/gdb/target-descriptions.c
+++ b/gdb/target-descriptions.c
@@ -333,6 +333,9 @@ struct target_desc : tdesc_element
   /* The features associated with this target.  */
   std::vector<tdesc_feature_up> features;

+  /* Used to cache the generated xml version of the target description.  */
+  mutable char *xmltarget = nullptr;
+
   void accept (tdesc_element_visitor &v) const override
   {
     v.visit_pre (this);
@@ -1667,6 +1670,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 +1757,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 != '@')
+    {
+      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 +1811,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 1c3409d53b..87c6be428f 100644
--- a/gdb/xml-tdesc.c
+++ b/gdb/xml-tdesc.c
@@ -752,3 +752,15 @@ 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)
+{
+  return tdesc_parse_xml (xml, [] (const char *href, void *baton)
+    {
+      error (_("xincludes are unsupported with this method"));
+      return gdb::optional<gdb::char_vector> ();
+    }, nullptr);
+}
diff --git a/gdb/xml-tdesc.h b/gdb/xml-tdesc.h
index 8f0679707a..1e3caba745 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 *xml);
+
 #endif /* XML_TDESC_H */

--
2.14.3 (Apple Git-98)