From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 71581 invoked by alias); 17 May 2017 15:43:24 -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 71564 invoked by uid 89); 17 May 2017 15:43:23 -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= 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:43:21 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C2E3380468; Wed, 17 May 2017 15:43:23 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C2E3380468 Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com C2E3380468 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 BE0AB179DD; Wed, 17 May 2017 15:43:21 +0000 (UTC) Subject: Re: [RFC 4/7] Share code in initialize_tdesc_ functions To: Philipp Rudo , Yao Qi References: <1494518105-15412-1-git-send-email-yao.qi@linaro.org> <20170511205504.cnufjdz6ehnml5wv@localhost> <20170516140217.1f2d3c64@ThinkPad> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <148caa86-619a-9941-071b-7f38c817db33@redhat.com> Date: Wed, 17 May 2017 15:43: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: <20170516140217.1f2d3c64@ThinkPad> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-05/txt/msg00398.txt.bz2 On 05/16/2017 01:02 PM, Philipp Rudo wrote: > Hi Yao, > > On Thu, 11 May 2017 21:55:04 +0100 > Yao Qi wrote: > >> diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c >> index 754f15b..a81ae0d 100644 >> --- a/gdb/target-descriptions.c >> +++ b/gdb/target-descriptions.c > > [...] > >> @@ -54,7 +57,10 @@ typedef struct tdesc_reg >> char *name; >> >> /* The register number used by this target to refer to this >> - register. This is used for remote p/P packets and to determine >> + register. Positive number means it is got by increasing >> + the previous register number by one. Negative number means >> + it is got from "regnum" attribute in the xml file. >> + This is used for remote p/P packets and to determine >> the ordering of registers in the remote g/G packets. */ >> long target_regnum; > > I'm not really sure if I like your idea with positive/negative regnums. It > could easily lead to hard to find bugs because someone forgot a '-' somewhere. > I would prefer an extra field like > > bool explicit_regnum; > or > bool automatic_regnum; > > [...] > >> +/* Return the unique name of FEATURE. The uniqueness means different >> + target description features have different values. This is used >> + to differentiate different features with the same name. */ >> + >> +static std::string >> +tdesc_feature_unique_name (const struct tdesc_feature* feature) >> +{ >> + std::string name = std::string (feature->name); >> + >> + if (name.compare ("org.gnu.gdb.arm.m-profile") == 0) >> + { >> + struct tdesc_reg *reg; >> + >> + for (int ix = 0; >> + VEC_iterate (tdesc_reg_p, feature->registers, ix, reg); >> + ix++) >> + if (reg->type != NULL && strcmp (reg->type, "arm_fpa_ext") == 0) >> + { >> + name.append ("_fpa"); >> + break; >> + } >> + } >> + else if (name.compare ("org.gnu.gdb.arm.vfp") == 0) >> + { >> + name.append ("_"); >> + name.append (std::to_string (VEC_length (tdesc_reg_p, >> + feature->registers) - 1)); >> + } >> + else if (name.compare ("org.gnu.gdb.i386.core") == 0) >> + { >> + struct tdesc_reg *reg = NULL; >> + >> + for (int ix = 0; >> + VEC_iterate (tdesc_reg_p, feature->registers, ix, reg); >> + ix++) >> + { >> + if (strcmp (reg->name, "ebp") == 0 >> + || strcmp (reg->name, "rbp") == 0) >> + break; >> + } >> + >> + name.append ("_"); >> + if (strcmp (reg->name, "ebp") == 0) >> + name.append ("i386"); >> + else >> + { >> + if (strcmp (reg->type, "data_ptr") == 0) >> + name.append ("amd64"); >> + else >> + name.append ("x32"); >> + } >> + } >> + else if (name.compare ("org.gnu.gdb.power.core") == 0) >> + { >> + struct tdesc_reg *reg = NULL; >> + struct tdesc_reg *reg_mq = NULL; >> + struct tdesc_reg *reg_r0 = NULL; >> + >> + /* Four variants. */ >> + for (int ix = 0; >> + VEC_iterate (tdesc_reg_p, feature->registers, ix, reg); >> + ix++) >> + { >> + if (strcmp (reg->name, "r0") == 0) >> + reg_r0 = reg; >> + if (strcmp (reg->name, "mq") == 0) >> + reg_mq = reg; >> + >> + if (reg_r0 != NULL && reg_mq != NULL) >> + break; >> + } >> + >> + name.append ("_"); >> + if (reg_mq == NULL) >> + name.append (std::to_string (reg_r0->bitsize)); >> + else >> + { >> + if (reg_mq->target_regnum == -124) >> + name.append ("601"); >> + else >> + name.append ("rs6000"); >> + } >> + } >> + else if (name.compare ("org.gnu.gdb.power.fpu") == 0) >> + { >> + struct tdesc_reg *reg = NULL; >> + >> + /* Three variants. */ >> + for (int ix = 0; >> + VEC_iterate (tdesc_reg_p, feature->registers, ix, reg); >> + ix++) >> + { >> + if (strcmp (reg->name, "fpscr") == 0) >> + break; >> + } >> + >> + name.append ("_"); >> + >> + if (reg->target_regnum == -71) >> + name.append ("rs6000"); >> + else >> + name.append (std::to_string (reg->bitsize)); >> + } >> + else if (name.compare ("org.gnu.gdb.power.linux") == 0) >> + { >> + struct tdesc_reg *reg = NULL; >> + >> + for (int ix = 0; >> + VEC_iterate (tdesc_reg_p, feature->registers, ix, reg); >> + ix++) >> + { >> + if (strcmp (reg->name, "orig_r3") == 0) >> + break; >> + } >> + >> + name.append (std::to_string (reg->bitsize)); >> + } >> + else if (name.compare ("org.gnu.gdb.s390.linux") == 0) >> + { >> + struct tdesc_reg *reg = NULL; >> + struct tdesc_reg *reg_orig_r2 = NULL; >> + struct tdesc_reg *reg_sc = NULL; >> + struct tdesc_reg *reg_lb = NULL; >> + >> + /* Six variants. */ >> + for (int ix = 0; >> + VEC_iterate (tdesc_reg_p, feature->registers, ix, reg); >> + ix++) >> + { >> + if (strcmp (reg->name, "orig_r2") == 0) >> + reg_orig_r2 = reg; >> + if (strcmp (reg->name, "last_break") == 0) >> + reg_lb = reg; >> + if (strcmp (reg->name, "system_call") == 0) >> + reg_sc = reg; >> + >> + if (reg_orig_r2 != NULL && reg_sc != NULL && reg_lb != NULL) >> + break; >> + } >> + >> + name.append ("_"); >> + name.append (std::to_string (reg_orig_r2->bitsize)); >> + >> + if (reg_lb != NULL) >> + { >> + name.append ("_"); >> + >> + if (reg_sc == NULL) >> + name.append ("v1"); >> + else >> + name.append ("v2"); >> + } >> + } >> + else if (name.compare ("org.gnu.gdb.s390.core") == 0) >> + { >> + struct tdesc_reg *reg = NULL; >> + struct tdesc_reg *reg_r0 = NULL; >> + struct tdesc_reg *reg_r0l = NULL; >> + >> + /* Six variants. */ >> + for (int ix = 0; >> + VEC_iterate (tdesc_reg_p, feature->registers, ix, reg); >> + ix++) >> + { >> + if (strcmp (reg->name, "r0") == 0) >> + reg_r0 = reg; >> + if (strcmp (reg->name, "r0l") == 0) >> + reg_r0l = reg; >> + >> + if (reg_r0 != NULL && reg_r0l != NULL) >> + break; >> + } >> + >> + name.append ("_"); >> + if (reg_r0l != NULL) >> + name.append ("s64"); >> + else >> + name.append (std::to_string (reg_r0->bitsize)); >> + } >> + else if (name.compare ("org.gnu.gdb.mips.cpu") == 0 >> + || name.compare ("org.gnu.gdb.mips.cp0") == 0 >> + || name.compare ("org.gnu.gdb.mips.fpu") == 0 >> + || name.compare ("org.gnu.gdb.mips.dsp") == 0 >> + || name.compare ("org.gnu.gdb.mips.linux") == 0) >> + { >> + struct tdesc_reg *reg = VEC_index (tdesc_reg_p, feature->registers, 0); >> + >> + name.append (std::to_string (reg->bitsize)); >> + } >> + >> + std::replace (name.begin (), name.end (), '.', '_'); >> + std::replace (name.begin (), name.end (), '-', '_'); >> + >> + return name; >> +} >> + > > This function is actually the part I like least of your implementation. Its > way to long and barely readable. The way I understand, it is needed to create > unique macro and function names. So why don't you simply use the filename of > the XML file where the feature is defined? It already is unique. Or use an > gdbarch hook so every architecture can decide for itself how to name them? Agreed. I was reading the patch and thinking how there must be a better way to handle this. Thanks, Pedro Alves