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

[Philipp Rudo <prudo at linux dot vnet dot ibm dot com>]

On Thu, 1 Mar 2018 11:41:04 +0000
Alan Hayward <Alan.Hayward@arm.com> wrote:

[...]

> diff --git a/gdb/common/tdesc.c b/gdb/common/tdesc.c
> index 115e7cd77942b86dedced946773d1d71950e2bb3..ce5b89045c48c65ac4c6c48f6646f8d5c6ce9d8b 100644
> --- a/gdb/common/tdesc.c
> +++ b/gdb/common/tdesc.c
> @@ -288,4 +288,158 @@ tdesc_add_enum_value (tdesc_type_with_fields *type, int value,
>    type->fields.emplace_back (name,
>  			     tdesc_predefined_type (TDESC_TYPE_INT32),
>  			     value, -1);
> -}
> \ No newline at end of file

This looks like an artefact to me.  It also appears in the commit on your
branch but not in the code.  Very strange ...

> +}
> +
> +void print_xml_feature::visit_post (const target_desc *e)
> +{
> +  *m_buffer += "</target>\n";
> +}
> +
> +void print_xml_feature::visit_pre (const tdesc_feature *e)
> +{
> +  *m_buffer += "<feature name=\"";
> +  *m_buffer += e->name;
> +  *m_buffer += "\">\n";
> +}

Have you considered using string_printf from common-utils? I think this could
make the code a lot better readable.  Maybe its even worth adding a
string_sprintf function.

Thanks
Philipp

> +
> +void print_xml_feature::visit_post (const tdesc_feature *e)
> +{
> +  *m_buffer += "</feature>\n";
> +}
> +
> +void print_xml_feature::visit (const tdesc_type_builtin *type)
> +{
> +  error (_("xml output is not supported type \"%s\"."), type->name.c_str ());
> +}
> +
> +void print_xml_feature::visit (const tdesc_type_vector *type)
> +{
> +  *m_buffer += "<vector id=\"";
> +  *m_buffer += type->name;
> +  *m_buffer += "\" type=\"";
> +  *m_buffer += type->element_type->name;
> +  *m_buffer += "\" count=\"";
> +  *m_buffer += std::to_string (type->count);
> +  *m_buffer += "\"/>\n";
> +}
> +
> +void print_xml_feature::visit (const tdesc_type_with_fields *type)
> +{
> +  struct tdesc_type_field *f;
> +  const static char *types[] = { "struct", "union", "flags", "enum" };
> +
> +  gdb_assert (type->kind >= TDESC_TYPE_STRUCT && type->kind <= TDESC_TYPE_ENUM);
> +  *m_buffer += "<";
> +  *m_buffer += types[type->kind - TDESC_TYPE_STRUCT];
> +
> +  switch (type->kind)
> +    {
> +    case TDESC_TYPE_STRUCT:
> +    case TDESC_TYPE_FLAGS:
> +      *m_buffer += " id=\"";
> +      *m_buffer += type->name;
> +      if (type->size > 0)
> +	{
> +	  *m_buffer += "\" size=\"";
> +	  *m_buffer += std::to_string (type->size);
> +	}
> +	*m_buffer += "\">\n";
> +
> +      for (const tdesc_type_field &f : type->fields)
> +	{
> +	  *m_buffer += "  <field name=\"";
> +	  *m_buffer += f.name;
> +	  if (f.start == -1)
> +	  {
> +	    *m_buffer += "\" type=\"";
> +	    *m_buffer += f.type->name;
> +	  }
> +	  else
> +	  {
> +	    *m_buffer += "\" start=\"";
> +	    *m_buffer += std::to_string (f.start);
> +	    *m_buffer += "\" end=\"";
> +	    *m_buffer += std::to_string (f.end);
> +	  }
> +
> +	  *m_buffer += "\"/>\n";
> +	}
> +    break;
> +
> +    case TDESC_TYPE_ENUM:
> +      *m_buffer += " id=\"";
> +      *m_buffer += type->name;
> +      *m_buffer += "\">\n";
> +
> +      for (const tdesc_type_field &f : type->fields)
> +	{
> +	  *m_buffer += "  <field name=\"";
> +	  *m_buffer += f.name;
> +	  *m_buffer += "\" start=\"";
> +	  *m_buffer += std::to_string (f.start);
> +	  *m_buffer += "\"/>\n";
> +	}
> +      break;
> +
> +    case TDESC_TYPE_UNION:
> +      *m_buffer += " id=\"";
> +      *m_buffer += type->name;
> +      *m_buffer += "\">\n";
> +
> +      for (const tdesc_type_field &f : type->fields)
> +	{
> +	  *m_buffer += "  <field name=\"";
> +	  *m_buffer += f.name;
> +	  *m_buffer += "\" type=\"";
> +	  *m_buffer += f.type->name;
> +	  *m_buffer += "\"/>\n";
> +	}
> +      break;
> +
> +    default:
> +      error (_("xml output is not supported type \"%s\"."),
> +	     type->name.c_str ());
> +    }
> +
> +  *m_buffer += "</";
> +  *m_buffer += types[type->kind - TDESC_TYPE_STRUCT];
> +  *m_buffer += ">\n";
> +}
> +
> +void print_xml_feature::visit (const tdesc_reg *reg)
> +{
> +  *m_buffer += "<reg name=\"";
> +  *m_buffer += reg->name;
> +  *m_buffer += "\" bitsize=\"";
> +  *m_buffer += std::to_string (reg->bitsize);
> +  *m_buffer += "\" type=\"";
> +  *m_buffer += reg->type;
> +  *m_buffer += "\" regnum=\"";
> +  *m_buffer += std::to_string (reg->target_regnum);
> +  if (reg->group.length () > 0)
> +    {
> +      *m_buffer += "\" group=\"";
> +      *m_buffer += reg->group;
> +    }
> +  *m_buffer += "\"/>\n";
> +}
> +
> +void print_xml_feature::visit_pre (const target_desc *e)
> +{
> +#ifndef IN_PROCESS_AGENT
> +  *m_buffer += "<?xml version=\"1.0\"?>\n";
> +  *m_buffer += "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">\n";
> +  *m_buffer += "<target>\n";
> +  *m_buffer += "<architecture>";
> +  *m_buffer += tdesc_architecture_name (e);
> +  *m_buffer += "</architecture>\n";
> +
> +  const char *osabi = tdesc_osabi_name (e);
> +  if (osabi != nullptr)
> +    {
> +      *m_buffer += "<osabi>";
> +      *m_buffer += osabi;
> +      *m_buffer += "</osabi>\n";
> +    }
> +#endif
> +}
> diff --git a/gdb/gdbserver/tdesc.h b/gdb/gdbserver/tdesc.h
> index 8534069d616b97dc82df04939a4ac935aa4e1cad..1c0c4b15f3cf2a7c4afd190963c1ca1ac77e1b59 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.  */
> @@ -73,6 +73,8 @@ public:
>      return !(*this == other);
>    }
>  #endif
> +
> +  void accept (tdesc_element_visitor &v) const;
>  };
> 
>  /* Copy target description SRC to DEST.  */
> diff --git a/gdb/gdbserver/tdesc.c b/gdb/gdbserver/tdesc.c
> index e11344762a3a4114ed9aa459d7d739bd96a90ae5..0d9609bd4c3446b7a15df569923d22fc10105b74 100644
> --- a/gdb/gdbserver/tdesc.c
> +++ b/gdb/gdbserver/tdesc.c
> @@ -59,6 +59,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)
>  {
> @@ -158,8 +170,7 @@ 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)
> @@ -171,31 +182,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/regformats/regdat.sh b/gdb/regformats/regdat.sh
> index 8c6e191596350fb4e983f8736985d9832f41e2d3..e6e06bdab0bdecc579686f3525e9f93555e0dd83 100755
> --- a/gdb/regformats/regdat.sh
> +++ b/gdb/regformats/regdat.sh
> @@ -180,7 +180,6 @@ echo
>  cat <<EOF
>  #ifndef IN_PROCESS_AGENT
>    result->expedite_regs = expedite_regs_${name};
> -  result->xmltarget = xmltarget_${name};
>  #endif
> 
>    init_target_desc (result);
> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
> index da2c1ce34531c1b23281c42f2dacbc85444ef544..93e73571177c980b4cd1975256c89e8ffb9fcdd6 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;
> 
> +  char *xmltarget = nullptr;
> +
>    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 (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)
>  {
> @@ -1760,7 +1777,36 @@ maintenance_check_xml_descriptions (const char *dir, int from_tty)
>  	= file_read_description_xml (tdesc_xml.data ());
> 
>        if (tdesc == NULL || *tdesc != *e.second)
> -	failed++;
> +	{
> +	  printf_filtered ( _("Descriptions for %s do not match\n"), e.first);
> +	  failed++;
> +	  continue;
> +	}
> +
> +      /* Convert both descriptions to xml, and then back again.  Confirm all
> +	 descriptions are identical.  */
> +
> +      const char *xml = tdesc_get_features_xml ((target_desc *) tdesc);
> +      const char *xml2 = tdesc_get_features_xml ((target_desc *) e.second);
> +      gdb_assert (*xml == '@');
> +      gdb_assert (*xml2 == '@');
> +      const target_desc *t_trans = target_read_description_xml_string (xml+1);
> +      const target_desc *t_trans2 = target_read_description_xml_string (xml2+1);
> +
> +      if (t_trans == NULL || t_trans2 == NULL)
> +	{
> +	  printf_filtered (
> +	    _("Could not convert descriptions for %s back to xml (%p %p)\n"),
> +	    e.first, t_trans, t_trans2);
> +	  failed++;
> +	}
> +      else if (*tdesc != *t_trans || *tdesc != *t_trans2)
> +	{
> +	  printf_filtered
> +	    (_("Translated descriptions for %s do not match (%d %d)\n"),
> +	    e.first, *tdesc == *t_trans, *tdesc == *t_trans2);
> +	  failed++;
> +	}
>      }
>    printf_filtered (_("Tested %lu XML files, %d failed\n"),
>  		   (long) selftests::xml_tdesc.size (), failed);
> diff --git a/gdb/xml-tdesc.h b/gdb/xml-tdesc.h
> index 8f0679707ad0f1e04d803f955f7fb98b4cc0c8c8..fee60e86dd10e1543b935c3cebce505d4dc828e2 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 *target_read_description_xml_string (const char *);
> +
>  #endif /* XML_TDESC_H */
> 
> diff --git a/gdb/xml-tdesc.c b/gdb/xml-tdesc.c
> index 9190d5f3c64ffdc6d7987d651527f597d695c5a6..f793f07c96847a3d61188fff1c5d63952cc37565 100644
> --- a/gdb/xml-tdesc.c
> +++ b/gdb/xml-tdesc.c
> @@ -752,3 +752,12 @@ target_fetch_description_xml (struct target_ops *ops)
>    return output;
>  #endif
>  }
> +
> +/* Take an xml string, parse it, and return the parsed description.  Does not
> +   handle a string containing includes.  */
> +
> +const struct target_desc *
> +target_read_description_xml_string (const char *xml_str)
> +{
> +  return tdesc_parse_xml (xml_str, nullptr, nullptr);
> +}
>