From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 68247 invoked by alias); 17 May 2017 15:41:30 -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 68220 invoked by uid 89); 17 May 2017 15:41:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Obviously X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 17 May 2017 15:41:27 +0000 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7C8B9C057FA4; Wed, 17 May 2017 15:41:29 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 7C8B9C057FA4 Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 7C8B9C057FA4 Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id AC9F160BE1; Wed, 17 May 2017 15:41:28 +0000 (UTC) Subject: Re: [RFC 2/7] Add unit test to builtin tdesc generated by xml To: Yao Qi , gdb-patches@sourceware.org References: <1494518105-15412-1-git-send-email-yao.qi@linaro.org> <1494518105-15412-3-git-send-email-yao.qi@linaro.org> From: Pedro Alves Message-ID: <692db623-3694-b809-4075-293c0d70cf5e@redhat.com> Date: Wed, 17 May 2017 15:41:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1494518105-15412-3-git-send-email-yao.qi@linaro.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-05/txt/msg00397.txt.bz2 On 05/11/2017 04:55 PM, Yao Qi wrote: > filename = lbasename (target_description_filename); > + std::string filename_after_features (target_description_filename); > + auto loc = filename_after_features.rfind ("/features/"); > + > + if (loc != std::string::npos) > + filename_after_features = filename_after_features.substr (loc + 10); > + > function = (char *) alloca (strlen (filename) + 1); > for (inp = filename, outp = function; *inp != '\0'; inp++) > if (*inp == '.') > @@ -1979,9 +2080,64 @@ feature = tdesc_create_feature (result, \"%s\");\n", > } > > printf_unfiltered (" tdesc_%s = 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); Since you're using the string as a C string -- c_str() is clearer than data(). > + printf_unfiltered ("#endif /* GDB_SELF_TEST */\n"); > printf_unfiltered ("}\n"); > } > > +#if GDB_SELF_TEST > +#include "selftest.h" > + > +namespace selftests { > + > +static std::vector> lists; I think this "lists" either needs a comment or a more descriptive name. > + > +void > +record_xml_tdesc (std::string xml_file, const struct target_desc *tdesc) > +{ > + lists.emplace_back (xml_file, tdesc); Write: lists.emplace_back (std::move (xml_file), tdesc); to avoid another copy. Though, I can't tell why do we need to store a dynamically allocated std::string, when seemingly a "const char *" would do? All callers pass in a string literal, and all you do with the strings in the list is iterate over all list elements and build new strings from those. > +} > + > +/* Test these GDB builtin target descriptions are identical to these which > + are generated by the corresponding xml files. */ > + > +static void > +xml_builtin_tdesc_test (void) (void) -> () > +{ > + std::string feature_dir (ldirname (__FILE__)); > + struct stat st; Ugh. Obviously this can't work if gdb is installed / copied elsewhere, remote host testing, etc. > + > + /* 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, ".."); > + > + /* If still can't find the path, something is wrong. */ > + SELF_CHECK (stat (feature_dir.data (), &st) == 0); Do we want to flag this as an error / unit test failure? Maybe it should be a warning instead? > + } > + > + feature_dir = feature_dir + SLASH_STRING + "features"; feature_dir += SLASH_STRING + "features"; > + > + for (auto const &e : lists) > + { > + std::string tdesc_xml = (feature_dir + SLASH_STRING + e.first); > + const struct target_desc *tdesc > + = file_read_description_xml (tdesc_xml.data ()); > + > + SELF_CHECK (tdesc != 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; > > @@ -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 *); > > +/* 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); Any reason this and the other equals functions aren't operator== implementations? It's not obvious since the comments say "identical", which would maybe suggest that there may be some property that is not being compared and thus this is not strict value equality, but then function name says "equals". > + > /* Return the string value of a property named KEY, or NULL if the > property was not specified. */ > > @@ -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); > > +#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); Thanks, Pedro Alves