Re: [PATCH v4 08/10] Create xml from target descriptions

[Simon Marchi <simon.marchi@ericsson.com>]

On 2018-03-22 04:44 AM, alan.hayward@arm.com wrote:
> diff --git a/gdb/common/tdesc.h b/gdb/common/tdesc.h
> index 8ab77e365f..45eb24ea2b 100644
> --- a/gdb/common/tdesc.h
> +++ b/gdb/common/tdesc.h
> @@ -371,4 +371,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 (target_desc *tdesc);
> +
> +/* Print target description as xml.  */
> +
> +class print_xml_feature : public tdesc_element_visitor
> +{
> +public:
> +  print_xml_feature (std::string &buffer_)

I made a suggestion earlier that we don't use non-const references parameters
(I did not get any feedback yet though):

https://sourceware.org/ml/gdb-patches/2018-03/msg00145.html

Would you mind changing this parameter for a pointer?

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

Instead of casting away the const, tdesc_get_features_xml should probably take
a const target_desc *.  Because it's just used as some kind of cache/memoization,
the xmltarget field of target_desc can be made mutable (changing its value doesn't
really change the value of the target_desc).

> +      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);

Spaces around the +.

Can you try to find better names than xml and xml2?

Instead of doing everything twice, maybe it would be clearer to have a separate
function that takes a single target_desc and converts it to xml and back, and
verifies that the resulting target_desc is equal to the initial one.  You can
then call this function twice.

But stepping back a little bit, is it relevant to do this test on both target_desc,
even after we check that they are equal?  Maybe it is, I'm just asking :)

> +
> +      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.c b/gdb/xml-tdesc.c
> index 9190d5f3c6..f793f07c96 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.  */

Should be /* See xml-tdesc.h.  */

> +
> +const struct target_desc *
> +target_read_description_xml_string (const char *xml_str)

I think this function is misnamed.  If you look at the other similar functions,
the prefix (target_, file_) refers to the source of the file.  So to follow the
same convention, this function could be named string_read_description_xml.

> +{
> +  return tdesc_parse_xml (xml_str, nullptr, nullptr);

Instead of passing nullptr for fetcher, could you pass a dummy function that
just errors out?  If it ever happens, it will be more graceful than a segfault.
You can use a lambda like this:

const struct target_desc *
target_read_description_xml_string (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);
}

Thanks,

Simon