From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 70F3F395C04B for ; Thu, 14 May 2020 14:49:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 70F3F395C04B Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [10.0.0.193] (unknown [192.222.164.54]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 19EC01ED39; Thu, 14 May 2020 10:49:04 -0400 (EDT) Subject: Re: [PATCH v2 1/4] arc: Add XML target features for Linux targets To: Shahab Vahedi , gdb-patches@sourceware.org Cc: Shahab Vahedi , Anton Kolesov , Tom Tromey , Francois Bedard References: <20200326125206.13120-1-shahab.vahedi@gmail.com> <20200428160437.1585-1-shahab.vahedi@gmail.com> <20200428160437.1585-2-shahab.vahedi@gmail.com> From: Simon Marchi Message-ID: Date: Thu, 14 May 2020 10:49:04 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <20200428160437.1585-2-shahab.vahedi@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: tl Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 14 May 2020 14:49:16 -0000 On 2020-04-28 12:04 p.m., Shahab Vahedi via Gdb-patches wrote: > From: Anton Kolesov > > Add XML target features for Linux targets. Compared to default > Linux features: > > - Explicitly specify CPU machine. > - Remove baremetal only ILINK{,1,2} registers. > - Add LP_START and LP_END registers for hardware loops - required to > properly evaluate possible next instruction during software single > instruction stepping. > - Add BTA register which contains branch target address - address of > next instruction when processor is in the delay slot. > - ARC HS description also adds R30, R58 and R59 registers, specific to > this architecture. > > gdb/ChangeLog: > 2020-04-28 Anton Kolesov > > * arch/arc.h (arc_create_target_description): Support Linux targets. > * arch/arc.c (arc_create_target_description): Likewise. > * arc-tdep.c (arc_tdesc_init): Update invocation of > arc_read_description. > * features/Makefile (FEATURE_XMLFILES): Add new files. > * features/arc/aux-arcompact-linux.xml: New file. > * features/arc/aux-v2-linux.xml: Likewise. > * features/arc/core-arcompact-linux.xml: Likewise. > * features/arc/core-v2-linux.xml: Likewise. > * features/arc/aux-arcompact-linux.c: Generate. > * features/arc/aux-v2-linux.c: Likewise. > * features/arc/core-arcompact-linux.c: Likewise. > * features/arc/core-v2-linux.c: Likewise. Hi Shahab, Instead of having multiple almost identical features, it's now usual to build small features and compose them into the full target description. For example, instead of having both: - core-arcompact.xml - core-arcompact-linux.xml I think you should have: - core-arcompact.xml, containing everything that's common in the two above - core-arcompact-baremetal.xml, containing only the registers that are only present in the baremetal version. When building the target description, the pseudo code would look like: if (arcompact) create_core_arcompact_feature (); else create_core_v2_feature (); if (baremetal) // or if (!linux) { if (arcompact) create_core_arccompact_baremetal_feature (); else create_core_v2_baremetal_feature (); } Same goes for aux. The i386 and arm target descriptions have examples of how to do that. Note that I don't know if the order of the registers in the target description matters... if so it could make this more difficult. > @@ -37,21 +41,39 @@ arc_create_target_description (arc_sys_type sys_type) > long regnum = 0; > > #ifndef IN_PROCESS_AGENT > - if (sys_type == ARC_SYS_TYPE_ARCV2) > + if (sys_type == ARC_SYS_TYPE_ARCV2_BMT) > set_tdesc_architecture (tdesc, "arc:ARCv2"); > + else if (sys_type == ARC_SYS_TYPE_ARCV2_LNX) > + /* If this is ARCv2 Linux, then it is ARC HS. */ > + set_tdesc_architecture (tdesc, "arc:HS"); > else > set_tdesc_architecture (tdesc, "arc:ARC700"); > + > + if (sys_type == ARC_SYS_TYPE_ARCOMPACT_LNX > + || sys_type == ARC_SYS_TYPE_ARCV2_LNX) > + set_tdesc_osabi (tdesc, "GNU/Linux"); > #endif > > - if (sys_type == ARC_SYS_TYPE_ARCV2) > - { > - regnum = create_feature_arc_core_v2 (tdesc, regnum); > - regnum = create_feature_arc_aux_v2 (tdesc, regnum); > - } > - else > + switch (sys_type) > { > - regnum = create_feature_arc_core_arcompact (tdesc, regnum); > - regnum = create_feature_arc_aux_arcompact (tdesc, regnum); > + case ARC_SYS_TYPE_ARCOMPACT_BMT: > + regnum = create_feature_arc_core_arcompact (tdesc, regnum); > + regnum = create_feature_arc_aux_arcompact (tdesc, regnum); > + break; > + case ARC_SYS_TYPE_ARCOMPACT_LNX: > + regnum = create_feature_arc_core_arcompact_linux (tdesc, regnum); > + regnum = create_feature_arc_aux_arcompact_linux (tdesc, regnum); > + break; > + case ARC_SYS_TYPE_ARCV2_BMT: > + regnum = create_feature_arc_core_v2 (tdesc, regnum); > + regnum = create_feature_arc_aux_v2 (tdesc, regnum); > + break; > + case ARC_SYS_TYPE_ARCV2_LNX: > + regnum = create_feature_arc_core_v2_linux (tdesc, regnum); > + regnum = create_feature_arc_aux_v2_linux (tdesc, regnum); > + break; > + default: > + gdb_assert(!"Invalid arc_sys_type."); > } The goal of having descriptions built at runtime with features is to avoid this kind of combinatorial explosion. Now, you have two axis of configuration (arcompact vs arcv2, bare-metal vs linux). If you had more, it would become less and less practical. That's why I would encourage you to consider what I said above. > > return tdesc; > diff --git a/gdb/arch/arc.h b/gdb/arch/arc.h > index fd806ae7d34..3c19118b946 100644 > --- a/gdb/arch/arc.h > +++ b/gdb/arch/arc.h > @@ -23,8 +23,10 @@ > /* Supported ARC system hardware types. */ > enum arc_sys_type > { > - ARC_SYS_TYPE_ARCOMPACT = 0, /* ARC600 or ARC700 */ > - ARC_SYS_TYPE_ARCV2, /* ARC EM or ARC HS */ > + ARC_SYS_TYPE_ARCOMPACT_BMT = 0, /* ARC600 or ARC700 (baremetal) */ > + ARC_SYS_TYPE_ARCOMPACT_LNX, /* ARC600 or ARC700 (linux) */ > + ARC_SYS_TYPE_ARCV2_BMT, /* ARC EM or ARC HS (baremetal) */ > + ARC_SYS_TYPE_ARCV2_LNX, /* ARC HS (linux) */ > ARC_SYS_TYPE_NUM That's also not expected. The fact that the target system is running Linux should be handled by the fact that the selected osabi will be Linux. So you shouldn't need any changes here. Simon