Re: [patch v9 04/23] xml, btrace: define btrace xml document style

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Mon, 04 Mar 2013 18:05:51 +0100, Markus Metzger wrote:
> Define the xml document style for transferring branch trace data.
> 
> Add a function to parse a btrace xml document into a vector of branch trace
> blocks.
> 
> 2013-03-04  Markus Metzger  <markus.t.metzger@intel.com>
> 
> 	* features/btrace.dtd: New file.
> 	* Makefile.in (XMLFILES): Add btrace.dtd.
> 	* btrace.h (parse_xml_btrace): New declaration.
> 	* btrace.c: Include xml-support.h.
> 	(parse_xml_btrace): New function.
> 	(parse_xml_btrace_block): New function.
> 	(block_attributes): New struct.
> 	(btrace_attributes): New struct.
> 	(btrace_children): New struct.
> 	(btrace_elements): New struct.
[...]
> --- a/gdb/btrace.c
> +++ b/gdb/btrace.c
> @@ -29,6 +29,7 @@
>  #include "disasm.h"
>  #include "source.h"
>  #include "filenames.h"
> +#include "xml-support.h"
>  
>  /* Print a record debug message.  Use do ... while (0) to avoid ambiguities
>     when used in if statements.  */
> @@ -465,3 +466,97 @@ btrace_free_objfile (struct objfile *objfile)
>    ALL_THREADS (tp)
>      btrace_clear (tp);
>  }
> +
> +#if defined(HAVE_LIBEXPAT)

There should be space before '(' according to GNU Coding Standards.

It is 7 times in the whole patchet, ther other instances I have not commented.

Choose one:

#ifdef HAVE_LIBEXPAT
#if defined HAVE_LIBEXPAT
#if defined (HAVE_LIBEXPAT)


> +
> +/* Check the btrace document version.  */
> +
> +static void
> +check_xml_btrace_version (struct gdb_xml_parser *parser,
> +			  const struct gdb_xml_element *element,
> +			  void *user_data, VEC (gdb_xml_value_s) *attributes)
> +{
> +  const char *version = xml_find_attribute (attributes, "version")->value;
> +
> +  if (strcmp (version, "1.0") != 0)
> +    gdb_xml_error (parser, _("Unsupported btrace version: \"%s\""), version);
> +}
> +
> +/* Parse a btrace "block" xml record.  */
> +
> +static void
> +parse_xml_btrace_block (struct gdb_xml_parser *parser,
> +			const struct gdb_xml_element *element,
> +			void *user_data, VEC (gdb_xml_value_s) *attributes)
> +{
> +  VEC (btrace_block_s) **btrace;
> +  struct btrace_block *block;
> +  ULONGEST *begin, *end;
> +
> +  btrace = user_data;
> +  block = VEC_safe_push (btrace_block_s, *btrace, NULL);
> +
> +  begin = xml_find_attribute (attributes, "begin")->value;
> +  end = xml_find_attribute (attributes, "end")->value;
> +
> +  block->begin = *begin;
> +  block->end = *end;
> +}
> +
> +static const struct gdb_xml_attribute block_attributes[] = {
> +  { "begin", GDB_XML_AF_NONE, gdb_xml_parse_attr_ulongest, NULL },
> +  { "end", GDB_XML_AF_NONE, gdb_xml_parse_attr_ulongest, NULL },
> +  { NULL, GDB_XML_AF_NONE, NULL, NULL }
> +};
> +
> +static const struct gdb_xml_attribute btrace_attributes[] = {
> +  { "version", GDB_XML_AF_NONE, NULL, NULL },
> +  { NULL, GDB_XML_AF_NONE, NULL, NULL }
> +};
> +
> +static const struct gdb_xml_element btrace_children[] = {
> +  { "block", block_attributes, NULL,
> +    GDB_XML_EF_REPEATABLE | GDB_XML_EF_OPTIONAL, parse_xml_btrace_block, NULL },
> +  { NULL, NULL, NULL, GDB_XML_EF_NONE, NULL, NULL }
> +};
> +
> +static const struct gdb_xml_element btrace_elements[] = {
> +  { "btrace", btrace_attributes, btrace_children, GDB_XML_EF_NONE,
> +    check_xml_btrace_version, NULL },
> +  { NULL, NULL, NULL, GDB_XML_EF_NONE, NULL, NULL }
> +};
> +
> +#endif /* defined(HAVE_LIBEXPAT) */
> +
> +/* See btrace.h.  */
> +
> +VEC (btrace_block_s) *
> +parse_xml_btrace (const char *buffer)
> +{
> +  VEC (btrace_block_s) *btrace = NULL;
> +  struct cleanup *cleanup;
> +  int errcode;
> +
> +#if defined(HAVE_LIBEXPAT)
> +
> +  cleanup = make_cleanup (VEC_cleanup (btrace_block_s), &btrace);
> +  errcode = gdb_xml_parse_quick (_("btrace"), "btrace.dtd", btrace_elements,
> +				 buffer, &btrace);
> +  if (errcode != 0)
> +    {
> +      do_cleanups (cleanup);
> +      errno = errcode;

gdb_xml_parse_quick returns 0 or -1, -1 is not valid errno code.  Also I do
not see errno used by the caller of parse_xml_btrace.

Remove that errno setting, returning NULL is enough.


> +      return NULL;
> +    }
> +
> +  /* Keep parse results.  */
> +  discard_cleanups (cleanup);
> +
> +#else  /* !defined(HAVE_LIBEXPAT) */
> +
> +  error (_("Cannot process branch tracing result. XML parsing not supported."));

Two spaces after dot ('.').  Maybe also s/not/is not/.


> +
> +#endif  /* !defined(HAVE_LIBEXPAT) */
> +
> +  return btrace;
> +}
> diff --git a/gdb/btrace.h b/gdb/btrace.h
> index a1b01c8..405bcff 100644
> --- a/gdb/btrace.h
> +++ b/gdb/btrace.h
> @@ -132,4 +132,7 @@ extern void btrace_clear (struct thread_info *);
>  /* Clear the branch trace for all threads when an object file goes away.  */
>  extern void btrace_free_objfile (struct objfile *);
>  
> +/* Parse a branch trace xml document into a block vector.  */
> +extern VEC (btrace_block_s) *parse_xml_btrace (const char*);
> +
>  #endif /* BTRACE_H */
> diff --git a/gdb/features/btrace.dtd b/gdb/features/btrace.dtd
> new file mode 100644
> index 0000000..18c5b2a
> --- /dev/null
> +++ b/gdb/features/btrace.dtd
> @@ -0,0 +1,12 @@
> +<!-- Copyright (C) 2012 Free Software Foundation, Inc.

Besides all the .c/.h sources files here chould be also 2013.


> +
> +     Copying and distribution of this file, with or without modification,
> +     are permitted in any medium without royalty provided the copyright
> +     notice and this notice are preserved.  -->
> +
> +<!ELEMENT btrace  (block)* >
> +<!ATTLIST btrace  version CDATA   #FIXED "1.0">
> +
> +<!ELEMENT block        EMPTY>
> +<!ATTLIST block        begin  CDATA   #REQUIRED
> +                       end    CDATA   #REQUIRED>
> -- 
> 1.7.1

OK for check in with those few small changes.



Thanks,
Jan