From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 72654 invoked by alias); 16 May 2017 12:02: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 72604 invoked by uid 89); 16 May 2017 12:02:25 -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=Share, Six 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:02:24 +0000 Received: from pps.filterd (m0098420.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v4GBxjg1002649 for ; Tue, 16 May 2017 08:02:25 -0400 Received: from e06smtp15.uk.ibm.com (e06smtp15.uk.ibm.com [195.75.94.111]) by mx0b-001b2d01.pphosted.com with ESMTP id 2afvpgwbkf-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Tue, 16 May 2017 08:02:24 -0400 Received: from localhost by e06smtp15.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 16 May 2017 13:02:22 +0100 Received: from b06cxnps4074.portsmouth.uk.ibm.com (9.149.109.196) by e06smtp15.uk.ibm.com (192.168.101.145) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Tue, 16 May 2017 13:02:20 +0100 Received: from d06av22.portsmouth.uk.ibm.com (d06av22.portsmouth.uk.ibm.com [9.149.105.58]) by b06cxnps4074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v4GC2J1S32768020; Tue, 16 May 2017 12:02:19 GMT Received: from d06av22.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id A2F154C05A; Tue, 16 May 2017 13:01:03 +0100 (BST) Received: from d06av22.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 8D99A4C044; Tue, 16 May 2017 13:01:03 +0100 (BST) Received: from ThinkPad (unknown [9.152.212.148]) by d06av22.portsmouth.uk.ibm.com (Postfix) with ESMTP; Tue, 16 May 2017 13:01:03 +0100 (BST) Date: Tue, 16 May 2017 12:02:00 -0000 From: Philipp Rudo To: Yao Qi Cc: gdb-patches@sourceware.org Subject: Re: [RFC 4/7] Share code in initialize_tdesc_ functions In-Reply-To: <20170511205504.cnufjdz6ehnml5wv@localhost> References: <1494518105-15412-1-git-send-email-yao.qi@linaro.org> <20170511205504.cnufjdz6ehnml5wv@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 x-cbid: 17051612-0020-0000-0000-000003697775 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17051612-0021-0000-0000-000041C07095 Message-Id: <20170516140217.1f2d3c64@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/msg00341.txt.bz2 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? Philipp