[v4,08/10] Create xml from target descriptions
Commit Message
From: Alan Hayward <alan.hayward@arm.com>
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 V3 are:
I now use string_appendf to print to the buffer.
regdat.sh ensures tdesc->xmltarget is null for all targets with new
style target descriptions. tdesc_get_features_xml() will only generate
xml if xmltarget is null. This ensures older targets continue to send
just the name of the xml file.
Added a save_restore check when parsing registers.
Alan.
2018-03-21 Alan Hayward <alan.hayward@arm.com>
gdb/
* common/tdesc.c (print_xml_feature::visit_post): Add xml parsing.
(print_xml_feature::visit_pre): Likewise.
(print_xml_feature::visit_post): Likewise.
(print_xml_feature::visit): Likewise.
(print_xml_feature::visit): Likewise.
(print_xml_feature::visit): Likewise.
(print_xml_feature::visit): Likewise.
* common/tdesc.h (print_xml_feature): Add new class.
* regformats/regdat.sh: Null xmltarget on feature targets.
* target-descriptions.c (struct target_desc): Add xmltarget.
(print_xml_feature::visit_pre): Add xml vistor.
(tdesc_get_features_xml): Add function to get xml.
(maintenance_check_xml_descriptions): Test xml generation.
* xml-tdesc.c (target_read_description_xml_string): Add function.
* xml-tdesc.h (target_read_description_xml_string): Add declaration.
gdbserver/
* tdesc.c (void target_desc::accept): Fill in function.
(tdesc_get_features_xml): Remove old xml creation.
(print_xml_feature::visit_pre): Add xml vistor.
---
gdb/common/tdesc.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++
gdb/common/tdesc.h | 29 +++++++++++++
gdb/gdbserver/tdesc.c | 43 +++++++------------
gdb/gdbserver/tdesc.h | 4 +-
gdb/regformats/regdat.sh | 4 +-
gdb/target-descriptions.c | 48 ++++++++++++++++++++-
gdb/xml-tdesc.c | 9 ++++
gdb/xml-tdesc.h | 5 +++
8 files changed, 218 insertions(+), 30 deletions(-)
Comments
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?
> 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).
> + 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.
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 (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. */
> +
> +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.
> +{
> + 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);
}
Thanks,
Simon
> 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.
@@ -289,3 +289,109 @@ 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 ());
+}
+
+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 ());
+ }
+
+ 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");
+}
@@ -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_)
+ : m_buffer (buffer_)
+ {}
+
+ ~print_xml_feature ()
+ {}
+
+ void visit_pre (const target_desc *e) override;
+ void visit_post (const target_desc *e) override;
+ void visit_pre (const tdesc_feature *e) override;
+ void visit_post (const tdesc_feature *e) override;
+ void visit (const tdesc_type_builtin *type) override;
+ void visit (const tdesc_type_vector *type) override;
+ void visit (const tdesc_type_with_fields *type) override;
+ void visit (const tdesc_reg *reg) override;
+
+private:
+ std::string &m_buffer;
+};
+
#endif /* ARCH_TDESC_H */
@@ -56,6 +56,18 @@ bool target_desc::operator== (const target_desc &other) const
#endif
+void target_desc::accept (tdesc_element_visitor &v) const
+{
+#ifndef IN_PROCESS_AGENT
+ v.visit_pre (this);
+
+ for (const tdesc_feature_up &feature : features)
+ feature->accept (v);
+
+ v.visit_post (this);
+#endif
+}
+
void
init_target_desc (struct target_desc *tdesc)
{
@@ -162,8 +174,7 @@ set_tdesc_osabi (struct target_desc *target_desc, const char *name)
target_desc->osabi = xstrdup (name);
}
-/* Return a string which is of XML format, including XML target
- description to be sent to GDB. */
+/* See common/tdesc.h. */
const char *
tdesc_get_features_xml (target_desc *tdesc)
@@ -175,31 +186,9 @@ tdesc_get_features_xml (target_desc *tdesc)
if (tdesc->xmltarget == NULL)
{
- std::string buffer ("@<?xml version=\"1.0\"?>");
-
- buffer += "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">";
- buffer += "<target>";
- buffer += "<architecture>";
- buffer += tdesc_architecture_name (tdesc);
- buffer += "</architecture>";
-
- const char *osabi = tdesc_osabi_name (tdesc);
- if (osabi != nullptr)
- {
- buffer += "<osabi>";
- buffer += osabi;
- buffer += "</osabi>";
- }
-
- for (const tdesc_feature_up &feature : tdesc->features)
- {
- buffer += "<xi:include href=\"";
- buffer += feature->name;
- buffer += "\"/>";
- }
-
- buffer += "</target>";
-
+ std::string buffer ("@");
+ print_xml_feature v (buffer);
+ tdesc->accept (v);
tdesc->xmltarget = xstrdup (buffer.c_str ());
}
@@ -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. */
@@ -73,6 +73,8 @@ public:
return !(*this == other);
}
#endif
+
+ void accept (tdesc_element_visitor &v) const;
};
/* Copy target description SRC to DEST. */
@@ -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
@@ -333,6 +333,8 @@ struct target_desc : tdesc_element
/* The features associated with this target. */
std::vector<tdesc_feature_up> features;
+ char *xmltarget = nullptr;
+
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 (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)
{
@@ -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);
+ 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);
+
+ 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);
@@ -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. */
+
+const struct target_desc *
+target_read_description_xml_string (const char *xml_str)
+{
+ return tdesc_parse_xml (xml_str, nullptr, nullptr);
+}
@@ -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 *target_read_description_xml_string (const char *);
+
#endif /* XML_TDESC_H */