From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10292 invoked by alias); 25 Jan 2018 13:14:35 -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 10283 invoked by uid 89); 25 Jan 2018 13:14:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.2 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,KAM_LOTSOFHASH,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 spammy= X-HELO: mx0a-001b2d01.pphosted.com Received: from mx0b-001b2d01.pphosted.com (HELO mx0a-001b2d01.pphosted.com) (148.163.158.5) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 25 Jan 2018 13:14:31 +0000 Received: from pps.filterd (m0098419.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id w0PDEMNk136244 for ; Thu, 25 Jan 2018 08:14:30 -0500 Received: from e06smtp11.uk.ibm.com (e06smtp11.uk.ibm.com [195.75.94.107]) by mx0b-001b2d01.pphosted.com with ESMTP id 2fqf9v9n7h-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Thu, 25 Jan 2018 08:14:29 -0500 Received: from localhost by e06smtp11.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 25 Jan 2018 13:14:27 -0000 Received: from b06cxnps4076.portsmouth.uk.ibm.com (9.149.109.198) by e06smtp11.uk.ibm.com (192.168.101.141) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Thu, 25 Jan 2018 13:14:25 -0000 Received: from d06av22.portsmouth.uk.ibm.com (d06av22.portsmouth.uk.ibm.com [9.149.105.58]) by b06cxnps4076.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id w0PDEOVC48824326; Thu, 25 Jan 2018 13:14:24 GMT Received: from d06av22.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id F2A2C4C044; Thu, 25 Jan 2018 13:08:27 +0000 (GMT) Received: from d06av22.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id BFBE54C040; Thu, 25 Jan 2018 13:08:27 +0000 (GMT) Received: from ThinkPad (unknown [9.152.212.63]) by d06av22.portsmouth.uk.ibm.com (Postfix) with ESMTP; Thu, 25 Jan 2018 13:08:27 +0000 (GMT) Date: Thu, 25 Jan 2018 13:14:00 -0000 From: Philipp Rudo To: Alan Hayward Cc: "gdb-patches@sourceware.org" , nd Subject: Re: [PATCH v2 6/8] Create xml from target descriptions In-Reply-To: References: <7C97CC6A-92CB-4702-820D-206022F07102@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 x-cbid: 18012513-0040-0000-0000-00000428BBD0 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 18012513-0041-0000-0000-000020CC4581 Message-Id: <20180125141423.1ff34025@ThinkPad> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2018-01-25_05:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1709140000 definitions=main-1801250179 X-IsSubscribed: yes X-SW-Source: 2018-01/txt/msg00524.txt.bz2 i don't really like print_xml_feature::visit_pre being declared in arch/tdesc.h with different implementations in target-descriptions.c and gdbserver/tdesc.c while all other visit_* implementations are in arch/tdesc.c. I would prefere visit_pre being implemented in arch/tdesc.c, too. Even when it means to add some #ifdef GDBSERVER ... #else ... #endif blocks until there is a common tdesc. Same for tdesc_type::make_gdb_type (patch #5). But here i would prefer to not even declare the method for GDBserver, i.e. struct tdesc_type { [...] #ifndef GDBSERVER virtual type *make_gdb_type (struct gdbarch *gdbarch) const = 0; #endif }; The problem i see with implementing stubs calling error is that you cannot find out you made a mistake until you call the function during run-time. This gives room to nasty bugs which could easily be prevented when there is a compile bug. Philipp On Wed, 24 Jan 2018 09:29:50 +0000 Alan Hayward wrote: > This patch adds a print_xml_feature visitor class which turns a C > target description into xml. > > 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. > > Alan. > > 2018-01-24 Alan Hayward > > gdb/ > * arch/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. > * arch/tdesc.h (print_xml_feature): Add new class. > * regformats/regdat.sh: obtain xml. > * 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. > > > diff --git a/gdb/arch/tdesc.h b/gdb/arch/tdesc.h > index 9a5bf6f11b670e04e2b51f8334bc0adaf0b43962..b2e99c5ce1f7da0e6a194ca721eea96082e1fa68 100644 > --- a/gdb/arch/tdesc.h > +++ b/gdb/arch/tdesc.h > @@ -195,7 +195,7 @@ struct tdesc_type_builtin : tdesc_type > : tdesc_type (name, kind) > {} > > - void accept (tdesc_element_visitor &v) const override; > + void accept (tdesc_element_visitor &v) const override > { > v.visit (this); > } > @@ -366,4 +366,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 */ > diff --git a/gdb/arch/tdesc.c b/gdb/arch/tdesc.c > index 9518571d03d394ee7cbf78b31974818201c889cd..388c6af1b93d40cf7cb5fb97c55912b91601814b 100644 > --- a/gdb/arch/tdesc.c > +++ b/gdb/arch/tdesc.c > @@ -288,4 +288,138 @@ tdesc_add_enum_value (tdesc_type_with_fields *type, int value, > type->fields.emplace_back (name, > tdesc_predefined_type (TDESC_TYPE_INT32), > value, -1); > -} > \ No newline at end of file > +} > + > +void print_xml_feature::visit_post (const target_desc *e) > +{ > + *m_buffer += "\n"; > +} > + > +void print_xml_feature::visit_pre (const tdesc_feature *e) > +{ > + *m_buffer += " + *m_buffer += e->name; > + *m_buffer += "\">\n"; > +} > + > +void print_xml_feature::visit_post (const tdesc_feature *e) > +{ > + *m_buffer += "\n"; > +} > + > +void print_xml_feature::visit (const tdesc_type_builtin *type) > +{ > + error (_("xml output is not supported type \"%s\"."), type->name.c_str ()); > +} > + > +void print_xml_feature::visit (const tdesc_type_vector *type) > +{ > + *m_buffer += " + *m_buffer += type->name; > + *m_buffer += "\" type=\""; > + *m_buffer += type->element_type->name; > + *m_buffer += "\" count=\""; > + *m_buffer += std::to_string (type->count); > + *m_buffer += "\"/>\n"; > +} > + > +void print_xml_feature::visit (const tdesc_type_with_fields *type) > +{ > + struct tdesc_type_field *f; > + const static char *types[] = { "struct", "union", "flags", "enum" }; > + > + gdb_assert (type->kind >= TDESC_TYPE_STRUCT && type->kind <= TDESC_TYPE_ENUM); > + *m_buffer += "<"; > + *m_buffer += types[type->kind - TDESC_TYPE_STRUCT]; > + > + switch (type->kind) > + { > + case TDESC_TYPE_STRUCT: > + case TDESC_TYPE_FLAGS: > + *m_buffer += " id=\""; > + *m_buffer += type->name; > + if (type->size > 0) > + { > + *m_buffer += "\" size=\""; > + *m_buffer += std::to_string (type->size); > + } > + *m_buffer += "\">\n"; > + > + for (const tdesc_type_field &f : type->fields) > + { > + *m_buffer += " + *m_buffer += f.name; > + if (f.start == -1) > + { > + *m_buffer += "\" type=\""; > + *m_buffer += f.type->name; > + } > + else > + { > + *m_buffer += "\" start=\""; > + *m_buffer += std::to_string (f.start); > + *m_buffer += "\" end=\""; > + *m_buffer += std::to_string (f.end); > + } > + > + *m_buffer += "\"/>\n"; > + } > + break; > + > + case TDESC_TYPE_ENUM: > + *m_buffer += " id=\""; > + *m_buffer += type->name; > + *m_buffer += "\">\n"; > + > + for (const tdesc_type_field &f : type->fields) > + { > + *m_buffer += " + *m_buffer += f.name; > + *m_buffer += "\" start=\""; > + *m_buffer += std::to_string (f.start); > + *m_buffer += "\"/>\n"; > + } > + break; > + > + case TDESC_TYPE_UNION: > + *m_buffer += " id=\""; > + *m_buffer += type->name; > + *m_buffer += "\">\n"; > + > + for (const tdesc_type_field &f : type->fields) > + { > + *m_buffer += " + *m_buffer += f.name; > + *m_buffer += "\" type=\""; > + *m_buffer += f.type->name; > + *m_buffer += "\"/>\n"; > + } > + break; > + > + default: > + error (_("xml output is not supported type \"%s\"."), > + type->name.c_str ()); > + } > + > + *m_buffer += " + *m_buffer += types[type->kind - TDESC_TYPE_STRUCT]; > + *m_buffer += ">\n"; > +} > + > +void print_xml_feature::visit (const tdesc_reg *reg) > +{ > + *m_buffer += " + *m_buffer += reg->name; > + *m_buffer += "\" bitsize=\""; > + *m_buffer += std::to_string (reg->bitsize); > + *m_buffer += "\" type=\""; > + *m_buffer += reg->type; > + *m_buffer += "\" regnum=\""; > + *m_buffer += std::to_string (reg->target_regnum); > + if (reg->group.length () > 0) > + { > + *m_buffer += "\" group=\""; > + *m_buffer += reg->group; > + } > + *m_buffer += "\"/>\n"; > +} > diff --git a/gdb/gdbserver/tdesc.h b/gdb/gdbserver/tdesc.h > index 8ae6cddc1896af99d86206d159fb952a0f666043..6a86f66cf2aaf9aa2b20203cad1d272d70026822 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. */ > @@ -72,7 +72,10 @@ public: > { > return !(*this == other); > } > + > #endif > + > + void accept (tdesc_element_visitor &v) const; > }; > > /* Copy target description SRC to DEST. */ > diff --git a/gdb/gdbserver/tdesc.c b/gdb/gdbserver/tdesc.c > index f0bd266a54601484df74ee1c5f8dce6fe04661c4..42a657f6827ed06c9d98275c7566d0c02890e7b3 100644 > --- a/gdb/gdbserver/tdesc.c > +++ b/gdb/gdbserver/tdesc.c > @@ -59,6 +59,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) > { > @@ -155,30 +167,9 @@ tdesc_get_features_xml (target_desc *tdesc) > > if (tdesc->xmltarget == NULL) > { > - std::string buffer ("@"); > - > - buffer += ""; > - buffer += ""; > - buffer += ""; > - buffer += tdesc->arch; > - buffer += ""; > - > - if (tdesc->osabi != nullptr) > - { > - buffer += ""; > - buffer += tdesc->osabi; > - buffer += ""; > - } > - > - for (const tdesc_feature_up &feature : tdesc->features) > - { > - buffer += " - buffer += feature->name; > - buffer += "\"/>"; > - } > - > - buffer += ""; > - > + std::string buffer (""); > + print_xml_feature v (&buffer); > + tdesc->accept (v); > tdesc->xmltarget = xstrdup (buffer.c_str ()); > } > > @@ -211,3 +202,22 @@ type *tdesc_type_with_fields::make_gdb_type (struct gdbarch *gdbarch) const > { > error (_("Cannot create gdbtypes.")); > } > + > +void print_xml_feature::visit_pre (const target_desc *e) > +{ > +#ifndef IN_PROCESS_AGENT > + *m_buffer += "@\n"; > + *m_buffer += "\n"; > + *m_buffer += "\n"; > + *m_buffer += ""; > + *m_buffer += e->arch; > + *m_buffer += "\n"; > + > + if (e->osabi != nullptr) > + { > + *m_buffer += ""; > + *m_buffer += e->osabi; > + *m_buffer += "\n"; > + } > +#endif > + } > diff --git a/gdb/regformats/regdat.sh b/gdb/regformats/regdat.sh > index 8c6e191596350fb4e983f8736985d9832f41e2d3..e6e06bdab0bdecc579686f3525e9f93555e0dd83 100755 > --- a/gdb/regformats/regdat.sh > +++ b/gdb/regformats/regdat.sh > @@ -180,7 +180,6 @@ echo > cat < #ifndef IN_PROCESS_AGENT > result->expedite_regs = expedite_regs_${name}; > - result->xmltarget = xmltarget_${name}; > #endif > > init_target_desc (result); > diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c > index 4f11120dce4092f15de99680a7c4868c4a2f4493..68a85758f7f63e6b862afa29697adee77e776d89 100644 > --- a/gdb/target-descriptions.c > +++ b/gdb/target-descriptions.c > @@ -316,6 +316,8 @@ struct target_desc : tdesc_element > /* The features associated with this target. */ > std::vector features; > > + char *xmltarget = nullptr; > + > void accept (tdesc_element_visitor &v) const override > { > v.visit_pre (this); > @@ -1633,6 +1635,39 @@ private: > int m_next_regnum = 0; > }; > > +void print_xml_feature::visit_pre (const target_desc *e) > +{ > + *m_buffer += "\n"; > + *m_buffer += "\n"; > + *m_buffer += "\n"; > + *m_buffer += ""; > + *m_buffer += tdesc_architecture (e)->printable_name; > + *m_buffer += "\n"; > + > + if (e->osabi != GDB_OSABI_UNKNOWN) > + { > + *m_buffer += ""; > + *m_buffer += gdbarch_osabi_name (tdesc_osabi (e)); > + *m_buffer += "\n"; > + } > +} > + > +/* Return a string which is of XML format, including XML target > + description to be sent to GDB. */ > + > +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) > { > @@ -1726,7 +1761,34 @@ 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); > + const target_desc *t_trans = target_read_description_xml_string (xml); > + const target_desc *t_trans2 = target_read_description_xml_string (xml2); > + > + 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.h b/gdb/xml-tdesc.h > index 8f0679707ad0f1e04d803f955f7fb98b4cc0c8c8..fee60e86dd10e1543b935c3cebce505d4dc828e2 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 *target_read_description_xml_string (const char *); > + > #endif /* XML_TDESC_H */ > > diff --git a/gdb/xml-tdesc.c b/gdb/xml-tdesc.c > index 9190d5f3c64ffdc6d7987d651527f597d695c5a6..f793f07c96847a3d61188fff1c5d63952cc37565 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. */ > + > +const struct target_desc * > +target_read_description_xml_string (const char *xml_str) > +{ > + return tdesc_parse_xml (xml_str, nullptr, nullptr); > +} > >