From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 58332 invoked by alias); 16 May 2017 12:00:54 -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 49817 invoked by uid 89); 16 May 2017 12:00:42 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 spammy=KEY, sk:mainten 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; Tue, 16 May 2017 12:00:38 +0000 Received: from pps.filterd (m0098421.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v4GBxkFh115776 for ; Tue, 16 May 2017 08:00:37 -0400 Received: from e06smtp14.uk.ibm.com (e06smtp14.uk.ibm.com [195.75.94.110]) by mx0a-001b2d01.pphosted.com with ESMTP id 2afxgaqx6r-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Tue, 16 May 2017 08:00:36 -0400 Received: from localhost by e06smtp14.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 16 May 2017 13:00:34 +0100 Received: from b06cxnps4074.portsmouth.uk.ibm.com (9.149.109.196) by e06smtp14.uk.ibm.com (192.168.101.144) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Tue, 16 May 2017 13:00:32 +0100 Received: from d06av24.portsmouth.uk.ibm.com (d06av24.portsmouth.uk.ibm.com [9.149.105.60]) by b06cxnps4074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v4GC0WdL32440384; Tue, 16 May 2017 12:00:32 GMT Received: from d06av24.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 5FEA042045; Tue, 16 May 2017 12:58:54 +0100 (BST) Received: from d06av24.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 44C2142049; Tue, 16 May 2017 12:58:54 +0100 (BST) Received: from ThinkPad (unknown [9.152.212.148]) by d06av24.portsmouth.uk.ibm.com (Postfix) with ESMTP; Tue, 16 May 2017 12:58:54 +0100 (BST) Date: Tue, 16 May 2017 12:00:00 -0000 From: Philipp Rudo To: Yao Qi Cc: gdb-patches@sourceware.org Subject: Re: [RFC 2/7] Add unit test to builtin tdesc generated by xml In-Reply-To: <1494518105-15412-3-git-send-email-yao.qi@linaro.org> References: <1494518105-15412-1-git-send-email-yao.qi@linaro.org> <1494518105-15412-3-git-send-email-yao.qi@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-TM-AS-GCONF: 00 x-cbid: 17051612-0016-0000-0000-0000049F6BD4 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17051612-0017-0000-0000-000027AF7F63 Message-Id: <20170516140027.29636db3@ThinkPad> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-05-16_03:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1703280000 definitions=main-1705160101 X-IsSubscribed: yes X-SW-Source: 2017-05/txt/msg00340.txt.bz2 Hi Yao, I really like the direction this series is going to. Generating the target description dynamically during runtime definitely makes the code much nicer= and reduces the number of XML files in the feature directory. The only thing you could have done to make it better is to get rid of the conversion to C altogether and create the target description directly by parsing the XML file. I don't see a benefit in the conversion and removing = it would simplify the code even more. But that is a long term goal and your p= atch set definitely helps towards that goal. See more comments below On Thu, 11 May 2017 16:55:00 +0100 Yao Qi wrote: [...] > diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c > index 9a7e2dd..754f15b 100644 > --- a/gdb/target-descriptions.c > +++ b/gdb/target-descriptions.c > @@ -468,6 +468,101 @@ tdesc_osabi (const struct target_desc *target_desc) > return target_desc->osabi; > } >=20 > +/* Return true if REG1 and REG2 are identical. */ > + > +static bool > +tdesc_reg_equals (const struct tdesc_reg *reg1, > + const struct tdesc_reg *reg2) > +{ > + return (strcmp (reg1->name, reg2->name) =3D=3D 0 > + && reg1->target_regnum =3D=3D reg2->target_regnum > + && reg1->save_restore =3D=3D reg2->save_restore > + && reg1->bitsize =3D=3D reg2->bitsize > + && (reg1->group =3D=3D reg2->group > + || strcmp (reg1->group, reg2->group) =3D=3D 0) > + && strcmp (reg1->type, reg2->type) =3D=3D 0); > +} inline? strcmp (...) =3D=3D 0 -> utils.h:streq (...)? Makes it at least a little sh= orter. > +/* Return true if TYPE1 and TYPE2 are identical. */ > + > +static bool > +tdesc_type_equals (const struct tdesc_type *type1, > + const struct tdesc_type *type2) > +{ > + return (strcmp (type1->name, type2->name) =3D=3D 0 > + && type1->kind =3D=3D type2->kind); > +} Same as above. > +/* Return true if FEATURE1 and FEATURE2 are identical. */ > + > +static bool > +tdesc_feature_equals (const struct tdesc_feature *feature1, > + const struct tdesc_feature *feature2) > +{ > + if (feature1 =3D=3D feature2) > + return true; > + > + if (strcmp (feature1->name, feature2->name) !=3D 0) > + return false; > + > + struct tdesc_reg *reg; > + > + for (int ix =3D 0; > + VEC_iterate (tdesc_reg_p, feature1->registers, ix, reg); > + ix++) > + { > + struct tdesc_reg *reg2 > + =3D VEC_index (tdesc_reg_p, feature2->registers, ix); > + > + if (!tdesc_reg_equals (reg, reg2)) > + return false; > + } > + > + struct tdesc_type *type; > + > + for (int ix =3D 0; > + VEC_iterate (tdesc_type_p, feature1->types, ix, type); > + ix++) > + { > + struct tdesc_type *type2 > + =3D VEC_index (tdesc_type_p, feature2->types, ix); > + > + if (!tdesc_type_equals (type, type2)) > + return false; > + } > + > + return true; > +} > + > +bool > +tdesc_equals (const struct target_desc *tdesc1, > + const struct target_desc *tdesc2) > +{ > + if (tdesc1->arch !=3D tdesc2->arch) > + return false; > + > + if (tdesc1->osabi !=3D tdesc2->osabi) > + return false; > + > + if (VEC_length (tdesc_feature_p, tdesc1->features) > + !=3D VEC_length (tdesc_feature_p, tdesc2->features)) > + return false; > + > + struct tdesc_feature *feature; > + > + for (int ix =3D 0; > + VEC_iterate (tdesc_feature_p, tdesc1->features, ix, feature); > + ix++) > + { > + struct tdesc_feature *feature2 > + =3D VEC_index (tdesc_feature_p, tdesc2->features, ix); > + > + if (!tdesc_feature_equals (feature, feature2)) > + return false; > + } > + return true; > +} > + > =0C >=20 > /* Return 1 if this target description includes any registers. */ > @@ -1734,6 +1829,12 @@ maint_print_c_tdesc_cmd (char *args, int from_tty) > error (_("The current target description did not come from an XML > file.")); >=20 > filename =3D lbasename (target_description_filename); > + std::string filename_after_features (target_description_filename); > + auto loc =3D filename_after_features.rfind ("/features/"); > + > + if (loc !=3D std::string::npos) > + filename_after_features =3D filename_after_features.substr (loc + 10= ); This piece is hard to read. This should do the same and (for my taste) is better readable std::string filename_with_subdir (target_description_filename); std::string subdir (lbasename (ldirname (target_description_filename))); if (subdir.campare ("features") !=3D 0) filename_with_subdir =3D concat_path (subdir, filename); If you prefer your way, please replace '10' by strlen ("/features/") (or similar) in the last line. > function =3D (char *) alloca (strlen (filename) + 1); > for (inp =3D filename, outp =3D function; *inp !=3D '\0'; inp++) > if (*inp =3D=3D '.') > @@ -1979,9 +2080,64 @@ feature =3D tdesc_create_feature (result, \"%s\");= \n", > } >=20 > printf_unfiltered (" tdesc_%s =3D result;\n", function); > + > + printf_unfiltered ("#if GDB_SELF_TEST\n"); > + printf_unfiltered (" selftests::record_xml_tdesc (\"%s\", tdesc_%s);\= n", > + filename_after_features.data (), function); > + printf_unfiltered ("#endif /* GDB_SELF_TEST */\n"); > printf_unfiltered ("}\n"); > } >=20 > +#if GDB_SELF_TEST > +#include "selftest.h" > + > +namespace selftests { > + > +static std::vector> l= ists; Can you rename the vector? Just 'lists' is pretty vague. > +void > +record_xml_tdesc (std::string xml_file, const struct target_desc *tdesc) > +{ > + lists.emplace_back (xml_file, tdesc); > +} > + > +/* Test these GDB builtin target descriptions are identical to these whi= ch > + are generated by the corresponding xml files. */ > + > +static void > +xml_builtin_tdesc_test (void) > +{ > + std::string feature_dir (ldirname (__FILE__)); > + struct stat st; > + > + /* Look for the features directory. If the directory of __FILE__ can't > + be found, __FILE__ is a file name with relative path. Guess that > + GDB is executed in testsuite directory like ../gdb, because I don't > + expect that GDB is invoked somewhere else and run self tests. */ > + if (stat (feature_dir.data (), &st) < 0) > + { > + feature_dir.insert (0, SLASH_STRING); > + feature_dir.insert (0, ".."); This would be a perfect spot for concat_path (patch 2 from my lk series https://sourceware.org/ml/gdb-patches/2017-03/msg00272.html). Then it would read feature_dir =3D concat_path ("..", feature_dir); I actually want to bring some of my patches upstream (mostly the s390-linux-tdep split up patch) to reduce maintenance cost of my series. This would be a good time for this patch, too. > + > + /* If still can't find the path, something is wrong. */ > + SELF_CHECK (stat (feature_dir.data (), &st) =3D=3D 0); > + } > + > + feature_dir =3D feature_dir + SLASH_STRING + "features"; feature_dir =3D concat_path (feature_dir, "features"); ? > + > + for (auto const &e : lists) > + { > + std::string tdesc_xml =3D (feature_dir + SLASH_STRING + e.first); std::string tdesc_xml =3D concat_path (feature_dir, e.first); ? Thanks Philipp > + const struct target_desc *tdesc > + =3D file_read_description_xml (tdesc_xml.data ()); > + > + SELF_CHECK (tdesc !=3D NULL); > + SELF_CHECK (tdesc_equals (tdesc, e.second)); > + } > +} > +} > +#endif /* GDB_SELF_TEST */ > + > /* Provide a prototype to silence -Wmissing-prototypes. */ > extern initialize_file_ftype _initialize_target_descriptions; >=20 > @@ -2022,4 +2178,8 @@ GDB will read the description from the target."), > add_cmd ("c-tdesc", class_maintenance, maint_print_c_tdesc_cmd, _("\ > Print the current target description as a C source file."), > &maintenanceprintlist); > + > +#if GDB_SELF_TEST > + register_self_test (selftests::xml_builtin_tdesc_test); > +#endif > } > diff --git a/gdb/target-descriptions.h b/gdb/target-descriptions.h > index 361ac97..78416ae 100644 > --- a/gdb/target-descriptions.h > +++ b/gdb/target-descriptions.h > @@ -162,6 +162,12 @@ enum gdb_osabi tdesc_osabi (const struct target_desc= *); > int tdesc_compatible_p (const struct target_desc *, > const struct bfd_arch_info *); >=20 > +/* Compare target descriptions TDESC1 and TDESC2, return true if they > + are identical. */ > + > +bool tdesc_equals (const struct target_desc *tdesc1, > + const struct target_desc *tdesc2); > + > /* Return the string value of a property named KEY, or NULL if the > property was not specified. */ >=20 > @@ -253,4 +259,14 @@ void tdesc_create_reg (struct tdesc_feature *feature, > const char *name, int regnum, int save_restore, const char *group, > int bitsize, const char *type); >=20 > +#if GDB_SELF_TEST > +namespace selftests { > + > +/* Record the target description TDESC generated by XML_FILE. */ > + > +void record_xml_tdesc (std::string xml_file, > + const struct target_desc *tdesc); > +} > +#endif > + > #endif /* TARGET_DESCRIPTIONS_H */