From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17587 invoked by alias); 18 Apr 2018 02:43:27 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 17569 invoked by uid 89); 18 Apr 2018 02:43:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=replying, DOCTYPE, doctype, !DOCTYPE X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 18 Apr 2018 02:43:24 +0000 Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 0F93A1E17E; Tue, 17 Apr 2018 22:43:21 -0400 (EDT) Subject: Re: [PATCH v5 6/8] Create xml from target descriptions To: Alan Hayward , gdb-patches@sourceware.org Cc: nd@arm.com References: <20180410143337.71768-1-alan.hayward@arm.com> <20180410143337.71768-7-alan.hayward@arm.com> From: Simon Marchi Message-ID: <4cde060f-100c-2293-68a3-dee0ffff707a@simark.ca> Date: Wed, 18 Apr 2018 02:43:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180410143337.71768-7-alan.hayward@arm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-04/txt/msg00364.txt.bz2 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, "\n", e->name.c_str ()); > +} > + > +void print_xml_feature::visit_post (const tdesc_feature *e) > +{ > + string_appendf (*m_buffer, "\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, "\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, " + 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, " \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, " \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, "\n", types[t->kind - TDESC_TYPE_STRUCT]); > +} > + > +void print_xml_feature::visit (const tdesc_reg *r) > +{ > + string_appendf (*m_buffer, > + " + 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, "\n"); > + string_appendf (*m_buffer, "\n"); > + string_appendf (*m_buffer, "\n%s\n", > + tdesc_architecture_name (e)); > + > + const char *osabi = tdesc_osabi_name (e); > + if (osabi != nullptr) > + string_appendf (*m_buffer, "%s", osabi); > +#endif > +} > + > +void print_xml_feature::visit_post (const target_desc *e) > +{ > + string_appendf (*m_buffer, "\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 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 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 (); > + }, nullptr); You'll need to change this to return gdb::optional (); 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 (); }, 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 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