Re: [RFC 2/7] Add unit test to builtin tdesc generated by xml

[Philipp Rudo <prudo@linux.vnet.ibm.com>]

Hi Yao,

I really like the direction this series is going to.  Generating the target
description dynamically during runtime definitely makes the code much nicer and
reduces the number of XML files in the feature directory.

The only thing you could have done to make it better is to get rid of the
conversion to C altogether and create the target description directly by
parsing the XML file. I don't see a benefit in the conversion and removing it
would simplify the code even more.  But that is a long term goal and your patch
set definitely helps towards that goal.

See more comments below

On Thu, 11 May 2017 16:55:00 +0100
Yao Qi <qiyaoltc@gmail.com> wrote:

[...]

> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
> index 9a7e2dd..754f15b 100644
> --- a/gdb/target-descriptions.c
> +++ b/gdb/target-descriptions.c
> @@ -468,6 +468,101 @@ tdesc_osabi (const struct target_desc *target_desc)
>    return target_desc->osabi;
>  }
> 
> +/* Return true if REG1 and REG2 are identical.  */
> +
> +static bool
> +tdesc_reg_equals (const struct tdesc_reg *reg1,
> +		  const struct tdesc_reg *reg2)
> +{
> +  return (strcmp (reg1->name, reg2->name) == 0
> +	  && reg1->target_regnum == reg2->target_regnum
> +	  && reg1->save_restore == reg2->save_restore
> +	  && reg1->bitsize == reg2->bitsize
> +	  && (reg1->group == reg2->group
> +	      || strcmp (reg1->group, reg2->group) == 0)
> +	  && strcmp (reg1->type, reg2->type) == 0);
> +}

inline?
strcmp (...) == 0 -> utils.h:streq (...)? Makes it at least a little shorter.

> +/* Return true if TYPE1 and TYPE2 are identical.  */
> +
> +static bool
> +tdesc_type_equals (const struct tdesc_type *type1,
> +		   const struct tdesc_type *type2)
> +{
> +  return (strcmp (type1->name, type2->name) == 0
> +	  && type1->kind == type2->kind);
> +}

Same as above.

> +/* Return true if FEATURE1 and FEATURE2 are identical.  */
> +
> +static bool
> +tdesc_feature_equals (const struct tdesc_feature *feature1,
> +		      const struct tdesc_feature *feature2)
> +{
> +  if (feature1 == feature2)
> +    return true;
> +
> +  if (strcmp (feature1->name, feature2->name) != 0)
> +    return false;
> +
> +  struct tdesc_reg *reg;
> +
> +  for (int ix = 0;
> +       VEC_iterate (tdesc_reg_p, feature1->registers, ix, reg);
> +       ix++)
> +    {
> +      struct tdesc_reg *reg2
> +	= VEC_index (tdesc_reg_p, feature2->registers, ix);
> +
> +      if (!tdesc_reg_equals (reg, reg2))
> +	return false;
> +    }
> +
> +  struct tdesc_type *type;
> +
> +  for (int ix = 0;
> +       VEC_iterate (tdesc_type_p, feature1->types, ix, type);
> +       ix++)
> +    {
> +      struct tdesc_type *type2
> +	= VEC_index (tdesc_type_p, feature2->types, ix);
> +
> +      if (!tdesc_type_equals (type, type2))
> +	return false;
> +    }
> +
> +  return true;
> +}
> +
> +bool
> +tdesc_equals (const struct target_desc *tdesc1,
> +	      const struct target_desc *tdesc2)
> +{
> +  if (tdesc1->arch != tdesc2->arch)
> +    return false;
> +
> +  if (tdesc1->osabi != tdesc2->osabi)
> +    return false;
> +
> +  if (VEC_length (tdesc_feature_p, tdesc1->features)
> +      != VEC_length (tdesc_feature_p, tdesc2->features))
> +      return false;
> +
> +  struct tdesc_feature *feature;
> +
> +  for (int ix = 0;
> +       VEC_iterate (tdesc_feature_p, tdesc1->features, ix, feature);
> +       ix++)
> +    {
> +      struct tdesc_feature *feature2
> +	= VEC_index (tdesc_feature_p, tdesc2->features, ix);
> +
> +      if (!tdesc_feature_equals (feature, feature2))
> +	return false;
> +    }
> +  return true;
> +}
> +
>  \f
> 
>  /* Return 1 if this target description includes any registers.  */
> @@ -1734,6 +1829,12 @@ maint_print_c_tdesc_cmd (char *args, int from_tty)
>      error (_("The current target description did not come from an XML
> file."));
> 
>    filename = lbasename (target_description_filename);
> +  std::string filename_after_features (target_description_filename);
> +  auto loc = filename_after_features.rfind ("/features/");
> +
> +  if (loc != std::string::npos)
> +    filename_after_features = filename_after_features.substr (loc + 10);

This piece is hard to read.  This should do the same and (for my taste) is
better readable

std::string filename_with_subdir (target_description_filename);
std::string subdir (lbasename (ldirname (target_description_filename)));
if (subdir.campare ("features") != 0)
  filename_with_subdir = concat_path (subdir, filename);

If you prefer your way, please replace '10' by strlen ("/features/") (or
similar) in the last line.

>    function = (char *) alloca (strlen (filename) + 1);
>    for (inp = filename, outp = function; *inp != '\0'; inp++)
>      if (*inp == '.')
> @@ -1979,9 +2080,64 @@ feature = tdesc_create_feature (result, \"%s\");\n",
>      }
> 
>    printf_unfiltered ("  tdesc_%s = result;\n", function);
> +
> +  printf_unfiltered ("#if GDB_SELF_TEST\n");
> +  printf_unfiltered ("  selftests::record_xml_tdesc (\"%s\", tdesc_%s);\n",
> +		     filename_after_features.data (), function);
> +  printf_unfiltered ("#endif /* GDB_SELF_TEST */\n");
>    printf_unfiltered ("}\n");
>  }
> 
> +#if GDB_SELF_TEST
> +#include "selftest.h"
> +
> +namespace selftests {
> +
> +static std::vector<std::pair<std::string, const struct target_desc *>> lists;

Can you rename the vector? Just 'lists' is pretty vague.

> +void
> +record_xml_tdesc (std::string xml_file, const struct target_desc *tdesc)
> +{
> +  lists.emplace_back (xml_file, tdesc);
> +}
> +
> +/* Test these GDB builtin target descriptions are identical to these which
> +   are generated by the corresponding xml files.  */
> +
> +static void
> +xml_builtin_tdesc_test (void)
> +{
> +  std::string feature_dir (ldirname (__FILE__));
> +  struct stat st;
> +
> +  /* Look for the features directory.  If the directory of __FILE__ can't
> +     be found, __FILE__ is a file name with relative path.  Guess that
> +     GDB is executed in testsuite directory like ../gdb, because I don't
> +     expect that GDB is invoked somewhere else and run self tests.  */
> +  if (stat (feature_dir.data (), &st) < 0)
> +    {
> +      feature_dir.insert (0, SLASH_STRING);
> +      feature_dir.insert (0, "..");

This would be a perfect spot for concat_path (patch 2 from my lk series
https://sourceware.org/ml/gdb-patches/2017-03/msg00272.html).
Then it would read

feature_dir = concat_path ("..", feature_dir);

I actually want to bring some of my patches upstream (mostly the
s390-linux-tdep split up patch) to reduce maintenance cost of my
series.  This would be a good time for this patch, too.

> +
> +      /* If still can't find the path, something is wrong.  */
> +      SELF_CHECK (stat (feature_dir.data (), &st) == 0);
> +    }
> +
> +  feature_dir = feature_dir + SLASH_STRING + "features";

feature_dir = concat_path (feature_dir, "features"); ?

> +
> +  for (auto const &e : lists)
> +    {
> +      std::string tdesc_xml = (feature_dir + SLASH_STRING + e.first);

std::string tdesc_xml = concat_path (feature_dir, e.first); ?

Thanks
Philipp


> +      const struct target_desc *tdesc
> +	= file_read_description_xml (tdesc_xml.data ());
> +
> +      SELF_CHECK (tdesc != NULL);
> +      SELF_CHECK (tdesc_equals (tdesc, e.second));
> +    }
> +}
> +}
> +#endif /* GDB_SELF_TEST */
> +
>  /* Provide a prototype to silence -Wmissing-prototypes.  */
>  extern initialize_file_ftype _initialize_target_descriptions;
> 
> @@ -2022,4 +2178,8 @@ GDB will read the description from the target."),
>    add_cmd ("c-tdesc", class_maintenance, maint_print_c_tdesc_cmd, _("\
>  Print the current target description as a C source file."),
>  	   &maintenanceprintlist);
> +
> +#if GDB_SELF_TEST
> +  register_self_test (selftests::xml_builtin_tdesc_test);
> +#endif
>  }
> diff --git a/gdb/target-descriptions.h b/gdb/target-descriptions.h
> index 361ac97..78416ae 100644
> --- a/gdb/target-descriptions.h
> +++ b/gdb/target-descriptions.h
> @@ -162,6 +162,12 @@ enum gdb_osabi tdesc_osabi (const struct target_desc *);
>  int tdesc_compatible_p (const struct target_desc *,
>  			const struct bfd_arch_info *);
> 
> +/* Compare target descriptions TDESC1 and TDESC2, return true if they
> +   are identical.  */
> +
> +bool tdesc_equals (const struct target_desc *tdesc1,
> +		   const struct target_desc *tdesc2);
> +
>  /* Return the string value of a property named KEY, or NULL if the
>     property was not specified.  */
> 
> @@ -253,4 +259,14 @@ void tdesc_create_reg (struct tdesc_feature *feature,
> const char *name, int regnum, int save_restore, const char *group,
>  		       int bitsize, const char *type);
> 
> +#if GDB_SELF_TEST
> +namespace selftests {
> +
> +/* Record the target description TDESC generated by XML_FILE.  */
> +
> +void record_xml_tdesc (std::string xml_file,
> +		       const struct target_desc *tdesc);
> +}
> +#endif
> +
>  #endif /* TARGET_DESCRIPTIONS_H */