From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8921 invoked by alias); 18 Apr 2018 01:57: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 8588 invoked by uid 89); 18 Apr 2018 01:57:24 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=UD:ca, states, held X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 18 Apr 2018 01:57:22 +0000 Received: from [10.0.0.11] (unknown [192.222.164.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 82FDF1E17E; Tue, 17 Apr 2018 21:57:19 -0400 (EDT) Subject: Re: [PATCH v5 1/8] Commonise tdesc_reg To: Alan Hayward , gdb-patches@sourceware.org Cc: nd@arm.com References: <20180410143337.71768-1-alan.hayward@arm.com> <20180410143337.71768-2-alan.hayward@arm.com> From: Simon Marchi Message-ID: <633ee25e-6f5e-5747-1f5d-8c86fafd19dd@simark.ca> Date: Wed, 18 Apr 2018 01:57:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180410143337.71768-2-alan.hayward@arm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-04/txt/msg00362.txt.bz2 On 2018-04-10 10:33 AM, Alan Hayward wrote: > This patch commonises tdesc_reg and makes use of it in gdbserver tdesc. > > gdbserver tdesc_create_reg is changed to create a tdesc_reg instead of > a reg_defs entry. The vector of tdesc_reg is held inside tdesc_feature. > > However, other modules in gdbserver directly access the reg_defs structure. > To work around this, init_target_desc fills in reg_defs by parsing the > tdesc_reg vector. > The long term goal is to remove reg_defs, replacing with accessor funcs. > > I wanted to make tdesc_create_reg common, but I cannot do that until > the next patch. > > I also had to commonise tdesc_element_visitor and tdesc_element. > > This patch only differs from the V4 version in init_target_desc() and > the changing of constructors for regdef. Hi Alan, Just two small comment, but the patch LGTM with those answered or addressed. > diff --git a/gdb/gdbserver/tdesc.h b/gdb/gdbserver/tdesc.h > index 85139d948c..8eb88eedce 100644 > --- a/gdb/gdbserver/tdesc.h > +++ b/gdb/gdbserver/tdesc.h > @@ -25,7 +25,10 @@ > #include > > struct tdesc_feature > -{}; > +{ > + /* The registers associated with this feature. */ > + std::vector registers; > +}; > > /* A target description. Inherit from tdesc_feature so that target_desc > can be used as tdesc_feature. */ > diff --git a/gdb/regformats/regdef.h b/gdb/regformats/regdef.h > index 4775e863e9..7c80d45d48 100644 > --- a/gdb/regformats/regdef.h > +++ b/gdb/regformats/regdef.h > @@ -21,15 +21,15 @@ > > struct reg > { > - reg () > + reg (int _offset) > : name (""), > - offset (0), > + offset (_offset), > size (0) > {} If this constructor is only used for padding entries, shouldn't name be NULL, as the documentation for the field states? Also, did you notice something failing if the padding entries don't have the offset field to the "current" offset at the time they are created? If we could leave them at 0, I think it would keep things simpler. I stopped for a few seconds, wondering why init_target_desc did: tdesc->reg_defs.resize (regnum, reg (offset)); and not just: tdesc->reg_defs.resize (regnum); Simon