From: Simon Marchi <simark@simark.ca>
To: Alan Hayward <alan.hayward@arm.com>, gdb-patches@sourceware.org
Cc: nd@arm.com
Subject: Re: [PATCH v5 6/8] Create xml from target descriptions
Date: Wed, 18 Apr 2018 02:43:00 -0000 [thread overview]
Message-ID: <4cde060f-100c-2293-68a3-dee0ffff707a@simark.ca> (raw)
In-Reply-To: <20180410143337.71768-7-alan.hayward@arm.com>
On 2018-04-10 10:33 AM, Alan Hayward wrote:
> This patch adds a print_xml_feature visitor class which turns a C
> target description into xml. Both gdb and gdbserver can do this.
>
> An accept function is added to gdbserver tdesc to allow it to use
> vistor classes.
>
> Tests are added to maintenance_check_xml_descriptions which takes
> each pair of tested descriptions, turns them both into xml, then back
> again, and confirms the descriptions still match.
>
> Differences from V4 are:
>
> print_xml_feature now uses a ptr.
> tdesc xmltarget is now mutable.
> Added maintenance_check_tdesc_xml_convert test function.
> Improved string_read_description_xml.
>
> This patch requires the next patch in the series, otherwise target
> description will be rejected by gdb when using gdbserver. I'll make sure
> the two patches get committed in a single commit.
Hi Alan,
The patch looks good to me. I noted a few formatting/minor comments.
> diff --git a/gdb/common/tdesc.c b/gdb/common/tdesc.c
> index b9e9ddb3fa..b4bd7bfb00 100644
> --- a/gdb/common/tdesc.c
> +++ b/gdb/common/tdesc.c
> @@ -290,3 +290,111 @@ tdesc_add_enum_value (tdesc_type_with_fields *type, int value,
> tdesc_predefined_type (TDESC_TYPE_INT32),
> value, -1);
> }
> +
> +void print_xml_feature::visit_pre (const tdesc_feature *e)
> +{
> + string_appendf (*m_buffer, "<feature name=\"%s\">\n", e->name.c_str ());
> +}
> +
> +void print_xml_feature::visit_post (const tdesc_feature *e)
> +{
> + string_appendf (*m_buffer, "</feature>\n");
> +}
> +
> +void print_xml_feature::visit (const tdesc_type_builtin *t)
> +{
> + error (_("xml output is not supported type \"%s\"."), t->name.c_str ());
Just a nit, but it seems to be missing something like a "for" between "supported" and "type".
> +}
> +
> +void print_xml_feature::visit (const tdesc_type_vector *t)
> +{
> + string_appendf (*m_buffer, "<vector id=\"%s\" type=\"%s\" count=\"%d\"/>\n",
> + t->name.c_str (), t->element_type->name.c_str (), t->count);
> +}
> +
> +void print_xml_feature::visit (const tdesc_type_with_fields *t)
> +{
> + struct tdesc_type_field *f;
> + const static char *types[] = { "struct", "union", "flags", "enum" };
> +
> + gdb_assert (t->kind >= TDESC_TYPE_STRUCT && t->kind <= TDESC_TYPE_ENUM);
> +
> + string_appendf (*m_buffer,
> + "<%s id=\"%s\"", types[t->kind - TDESC_TYPE_STRUCT],
> + t->name.c_str ());
> +
> + switch (t->kind)
> + {
> + case TDESC_TYPE_STRUCT:
> + case TDESC_TYPE_FLAGS:
> + if (t->size > 0)
> + string_appendf (*m_buffer, " size=\"%d\"", t->size);
> + string_appendf (*m_buffer, ">\n");
> +
> + for (const tdesc_type_field &f : t->fields)
> + {
> + string_appendf (*m_buffer, " <field name=\"%s\" ", f.name.c_str ());
> + if (f.start == -1)
> + string_appendf (*m_buffer, "type=\"%s\"/>\n",
> + f.type->name.c_str ());
> + else
> + string_appendf (*m_buffer, "start=\"%d\" end=\"%d\"/>\n", f.start,
> + f.end);
> + }
> + break;
> +
> + case TDESC_TYPE_ENUM:
> + string_appendf (*m_buffer, ">\n");
> + for (const tdesc_type_field &f : t->fields)
> + string_appendf (*m_buffer, " <field name=\"%s\" start=\"%d\"/>\n",
> + f.name.c_str (), f.start);
> + break;
> +
> + case TDESC_TYPE_UNION:
> + string_appendf (*m_buffer, ">\n");
> + for (const tdesc_type_field &f : t->fields)
> + string_appendf (*m_buffer, " <field name=\"%s\" type=\"%s\"/>\n",
> + f.name.c_str (), f.type->name.c_str ());
> + break;
> +
> + default:
> + error (_("xml output is not supported type \"%s\"."), t->name.c_str ());
"supported for type" here too.
> + }
> +
> + string_appendf (*m_buffer, "</%s>\n", types[t->kind - TDESC_TYPE_STRUCT]);
> +}
> +
> +void print_xml_feature::visit (const tdesc_reg *r)
> +{
> + string_appendf (*m_buffer,
> + "<reg name=\"%s\" bitsize=\"%d\" type=\"%s\" regnum=\"%ld\"",
> + r->name.c_str (), r->bitsize, r->type.c_str (),
> + r->target_regnum);
> +
> + if (r->group.length () > 0)
> + string_appendf (*m_buffer, " group=\"%s\"", r->group.c_str ());
> +
> + if (r->save_restore == 0)
> + string_appendf (*m_buffer, " save-restore=\"no\"");
> +
> + string_appendf (*m_buffer, "/>\n");
> +}
> +
> +void print_xml_feature::visit_pre (const target_desc *e)
> +{
> +#ifndef IN_PROCESS_AGENT
> + string_appendf (*m_buffer, "<?xml version=\"1.0\"?>\n");
> + string_appendf (*m_buffer, "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">\n");
> + string_appendf (*m_buffer, "<target>\n<architecture>%s</architecture>\n",
> + tdesc_architecture_name (e));
> +
> + const char *osabi = tdesc_osabi_name (e);
> + if (osabi != nullptr)
> + string_appendf (*m_buffer, "<osabi>%s</osabi>", osabi);
> +#endif
> +}
> +
> +void print_xml_feature::visit_post (const target_desc *e)
> +{
> + string_appendf (*m_buffer, "</target>\n");
> +}
> diff --git a/gdb/common/tdesc.h b/gdb/common/tdesc.h
> index 311341da0d..00eaada072 100644
> --- a/gdb/common/tdesc.h
> +++ b/gdb/common/tdesc.h
> @@ -381,4 +381,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 (const target_desc *tdesc);
> +
> +/* Print target description as xml. */
> +
> +class print_xml_feature : public tdesc_element_visitor
> +{
> +public:
> + print_xml_feature (std::string *buffer_)
> + : m_buffer (buffer_)
> + {}
> +
> + ~print_xml_feature ()
> + {}
You can remove the explicit destructor.
> diff --git a/gdb/gdbserver/tdesc.h b/gdb/gdbserver/tdesc.h
> index 197fb59127..2152b17947 100644
> --- a/gdb/gdbserver/tdesc.h
> +++ b/gdb/gdbserver/tdesc.h
> @@ -27,7 +27,7 @@
> /* A target description. Inherit from tdesc_feature so that target_desc
> can be used as tdesc_feature. */
>
> -struct target_desc
> +struct target_desc : tdesc_element
> {
> /* A vector of elements of register definitions that
> describe the inferior's register set. */
> @@ -51,7 +51,7 @@ struct target_desc
>
> It can be NULL, then, its content is got from the following three
> fields features, arch, and osabi in tdesc_get_features_xml. */
> - const char *xmltarget = NULL;
> + mutable const char *xmltarget = NULL;
I think the comment above might need a bit of updating.
>
> /* The value of <architecture> element in the XML, replying GDB. */
> const char *arch = NULL;
> @@ -73,6 +73,8 @@ public:
> return !(*this == other);
> }
> #endif
> +
> + void accept (tdesc_element_visitor &v) const;
override
> };
>
> /* Copy target description SRC to DEST. */
> @@ -89,8 +91,4 @@ void init_target_desc (struct target_desc *tdesc);
>
> const struct target_desc *current_target_desc (void);
>
> -#ifndef IN_PROCESS_AGENT
> -const char *tdesc_get_features_xml (struct target_desc *tdesc);
> -#endif
> -
> #endif /* TDESC_H */
> diff --git a/gdb/regformats/regdat.sh b/gdb/regformats/regdat.sh
> index 18108d77eb..8f546fe276 100755
> --- a/gdb/regformats/regdat.sh
> +++ b/gdb/regformats/regdat.sh
> @@ -163,7 +163,9 @@ done
>
> echo
> echo "static const char *expedite_regs_${name}[] = { \"`echo ${expedite} | sed 's/,/", "/g'`\", 0 };"
> -if test "${xmltarget}" = x; then
> +if test "${feature}" != x; then
> + echo "static const char *xmltarget_${name} = 0;"
> +elif test "${xmltarget}" = x; then
> if test "${xmlarch}" = x && test "${xmlosabi}" = x; then
> echo "static const char *xmltarget_${name} = 0;"
> else
> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c
> index da2c1ce345..b96ef961a4 100644
> --- a/gdb/target-descriptions.c
> +++ b/gdb/target-descriptions.c
> @@ -333,6 +333,8 @@ struct target_desc : tdesc_element
> /* The features associated with this target. */
> std::vector<tdesc_feature_up> features;
>
> + mutable char *xmltarget = nullptr;
Can you add a comment for this member? Just to mention that this
is used for caching/memoization of the generated xml.
> +
> void accept (tdesc_element_visitor &v) const override
> {
> v.visit_pre (this);
> @@ -1667,6 +1669,21 @@ private:
> int m_next_regnum = 0;
> };
>
> +/* See common/tdesc.h. */
> +
> +const char *
> +tdesc_get_features_xml (const target_desc *tdesc)
> +{
> + if (tdesc->xmltarget == nullptr)
> + {
> + std::string buffer ("@");
> + print_xml_feature v (&buffer);
> + tdesc->accept (v);
> + tdesc->xmltarget = xstrdup (buffer.c_str ());
> + }
> + return tdesc->xmltarget;
> +}
> +
> static void
> maint_print_c_tdesc_cmd (const char *args, int from_tty)
> {
> @@ -1739,6 +1756,39 @@ record_xml_tdesc (const char *xml_file, const struct target_desc *tdesc)
>
> }
>
> +/* Test the convesion process of a target description to/from xml: Take a target
> + description TDESC, convert to xml, back to a description, and confirm the new
> + tdesc is identical to the original. */
> +static bool
> +maintenance_check_tdesc_xml_convert (const target_desc *tdesc, const char *name)
> +{
> + const char *xml = tdesc_get_features_xml (tdesc);
> +
> + if (xml == nullptr && *xml != '@')
This condition is wrong. You probably meant
if (xml == nullptr || *xml != '@')
?
> + {
> + printf_filtered (_("Could not convert description for %s to xml.\n"),
> + name);
> + return false;
> + }
> +
> + const target_desc *tdesc_trans = string_read_description_xml (xml + 1);
> +
> + if (tdesc_trans == nullptr)
> + {
> + printf_filtered (_("Could not convert description for %s from xml.\n"),
> + name);
> + return false;
> + }
> + else if (*tdesc != *tdesc_trans)
> + {
> + printf_filtered (_("Converted description for %s does not match.\n"),
> + name);
> + return false;
> + }
> + return true;
> +}
> +
> +
> /* Check that the target descriptions created dynamically by
> architecture-specific code equal the descriptions created from XML files
> found in the specified directory DIR. */
> @@ -1760,6 +1810,12 @@ maintenance_check_xml_descriptions (const char *dir, int from_tty)
> = file_read_description_xml (tdesc_xml.data ());
>
> if (tdesc == NULL || *tdesc != *e.second)
> + {
> + printf_filtered ( _("Descriptions for %s do not match.\n"), e.first);
> + failed++;
> + }
> + else if (!maintenance_check_tdesc_xml_convert (tdesc, e.first)
> + || !maintenance_check_tdesc_xml_convert (e.second, e.first))
> failed++;
> }
> printf_filtered (_("Tested %lu XML files, %d failed\n"),
> diff --git a/gdb/xml-tdesc.c b/gdb/xml-tdesc.c
> index 9190d5f3c6..5c6ee4c8cd 100644
> --- a/gdb/xml-tdesc.c
> +++ b/gdb/xml-tdesc.c
> @@ -752,3 +752,14 @@ target_fetch_description_xml (struct target_ops *ops)
> return output;
> #endif
> }
> +
> +/* See xml-tdesc.h. */
> +
> +const struct target_desc *
> +string_read_description_xml (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);
You'll need to change this to
return gdb::optional<gdb::char_vector> ();
because of a recent change upstream. Also, this indentation would follow a bit
more what we do:
return tdesc_parse_xml (xml_str, [] (const char *href, void *baton)
{
error (_("xincludes are unsupported with this method"));
return gdb::optional<gdb::char_vector> ();
}, nullptr);
The idea is to format the lambda the same way we would format an if or for body.
> +}
> \ No newline at end of file
> diff --git a/gdb/xml-tdesc.h b/gdb/xml-tdesc.h
> index 8f0679707a..e39d5580bb 100644
> --- a/gdb/xml-tdesc.h
> +++ b/gdb/xml-tdesc.h
> @@ -44,5 +44,10 @@ const struct target_desc *target_read_description_xml (struct target_ops *);
> otherwise. */
> gdb::optional<std::string> target_fetch_description_xml (target_ops *ops);
>
> +/* Take an xml string, parse it, and return the parsed description. Does not
> + handle a string containing includes. */
> +
> +const struct target_desc *string_read_description_xml (const char *);
Can you give a name to the parameter? "xml" would be fine.
Thanks,
Simon
next prev parent reply other threads:[~2018-04-18 2:43 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-10 14:34 [PATCH v5 0/8] Remove gdbserver dependency on xml files Alan Hayward
2018-04-10 14:34 ` [PATCH v5 6/8] Create xml from target descriptions Alan Hayward
2018-04-18 2:43 ` Simon Marchi [this message]
2018-04-18 21:26 ` Alan Hayward
2018-04-10 14:34 ` [PATCH v5 4/8] Add tdesc osabi and architecture functions Alan Hayward
2018-04-18 2:10 ` Simon Marchi
2018-04-10 14:34 ` [PATCH v5 2/8] Commonise tdesc_feature Alan Hayward
2018-04-10 14:34 ` [PATCH v5 7/8] Remove xml file references from target descriptions Alan Hayward
2018-04-10 14:34 ` [PATCH v5 5/8] Add feature reference in .dat files Alan Hayward
2018-04-10 14:34 ` [PATCH v5 1/8] Commonise tdesc_reg Alan Hayward
2018-04-18 1:57 ` Simon Marchi
2018-04-18 9:03 ` Alan Hayward
2018-04-18 13:54 ` Simon Marchi
2018-04-10 14:34 ` [PATCH v5 3/8] Commonise tdesc types Alan Hayward
2018-04-10 14:34 ` [PATCH v5 8/8] Remove xml files from gdbserver Alan Hayward
2018-04-18 2:49 ` [PATCH v5 0/8] Remove gdbserver dependency on xml files Simon Marchi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4cde060f-100c-2293-68a3-dee0ffff707a@simark.ca \
--to=simark@simark.ca \
--cc=alan.hayward@arm.com \
--cc=gdb-patches@sourceware.org \
--cc=nd@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).