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

[Pedro Alves <palves at redhat dot com>]

On 05/11/2017 04:55 PM, Yao Qi wrote:
>    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);
> +
>    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);

Since you're using the string as a C string -- c_str() is clearer
than data().

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

I think this "lists" either needs a comment or a more descriptive name.

> +
> +void
> +record_xml_tdesc (std::string xml_file, const struct target_desc *tdesc)
> +{
> +  lists.emplace_back (xml_file, tdesc);

Write:

 lists.emplace_back (std::move (xml_file), tdesc);

to avoid another copy.

Though, I can't tell why do we need to store a dynamically allocated
std::string, when seemingly a "const char *" would do?  All callers pass
in a string literal, and all you do with the strings in the list is
iterate over all list elements and build new strings from those.

> +}
> +
> +/* 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)

(void) -> ()

> +{
> +  std::string feature_dir (ldirname (__FILE__));
> +  struct stat st;

Ugh.  Obviously this can't work if gdb is installed / copied elsewhere,
remote host testing, etc.

> +
> +  /* 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, "..");
> +
> +      /* If still can't find the path, something is wrong.  */
> +      SELF_CHECK (stat (feature_dir.data (), &st) == 0);

Do we want to flag this as an error / unit test failure?
Maybe it should be a warning instead?

> +    }
> +
> +  feature_dir = feature_dir + SLASH_STRING + "features";

  feature_dir += SLASH_STRING + "features";

> +
> +  for (auto const &e : lists)
> +    {
> +      std::string tdesc_xml = (feature_dir + SLASH_STRING + e.first);
> +      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);

Any reason this and the other equals functions aren't operator== implementations?
It's not obvious since the comments say "identical", which would maybe suggest that
there may be some property that is not being compared and thus this is not
strict value equality, but then function name says "equals".

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

Thanks,
Pedro Alves