This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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



> On 23 Mar 2018, at 21:24, Simon Marchi <simon.marchi@ericsson.com> wrote:
> 
> 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?

Sorry, missed that. Will do.

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

The mutable keyword is exactly what I was looking for here!

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

Will do.

> 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 this code was executed as part of a general run of gdb, then I’d agree and
move one of them out. However, given this is part of test the unit tests and it’s not
slowing anything down then I’m more inclined to leave it in. Maybe if someone
added new tdesc fields and didn’t include them correctly in the equals operator
and/or xml generation. (Although I see the argument for removing it to simply
the code).


>> +
>> +      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.  */

Ok.

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

Ok.

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

Will do.


Will update when I get back in two weeks.

Alan.




Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]