From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 66418 invoked by alias); 18 Apr 2018 13:54:23 -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 66401 invoked by uid 89); 18 Apr 2018 13:54:22 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.4 required=5.0 tests=AWL,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=states 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 13:54:20 +0000 Received: by simark.ca (Postfix, from userid 112) id 777701EF60; Wed, 18 Apr 2018 09:54:18 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 5D5A21E17E; Wed, 18 Apr 2018 09:54:16 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Date: Wed, 18 Apr 2018 13:54:00 -0000 From: Simon Marchi To: Alan Hayward Cc: gdb-patches@sourceware.org, nd Subject: Re: [PATCH v5 1/8] Commonise tdesc_reg In-Reply-To: <41D75C72-120B-4EEC-BAA5-D48232DD51ED@arm.com> References: <20180410143337.71768-1-alan.hayward@arm.com> <20180410143337.71768-2-alan.hayward@arm.com> <633ee25e-6f5e-5747-1f5d-8c86fafd19dd@simark.ca> <41D75C72-120B-4EEC-BAA5-D48232DD51ED@arm.com> Message-ID: <861e04ade36a72491ba9f58dd6bf6514@simark.ca> X-Sender: simark@simark.ca User-Agent: Roundcube Webmail/1.3.4 X-SW-Source: 2018-04/txt/msg00369.txt.bz2 On 2018-04-18 05:03, Alan Hayward wrote: >> On 18 Apr 2018, at 02:57, Simon Marchi wrote: >> >> 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? >> > > That creates two issues: > > The reg:operator== segfaults in the strcmp due to the null. > Easily fixable, but a little ugly. > > With that patched, the gdbserver unit tests will fail. > That is because the creation of target descriptions in the > -generated.c files > in the build dir create the gaps using: > tdesc_create_reg (feature, “", 0, 0, NULL, 0, NULL); > > Would fixing that cause more issues? Not sure. > > Having “” rather than 0 does make sense when you format these to xml > files. > We are explicitly setting to a blank string. Whereas null is more for > uninitialised. > > I was going to suggest fixing in a follow on patch, but the more I > think about it, > The more I’m thinking “” is correct. The important thing is that documentation is accurate, so could you adjust the doc of the name field accordingly? >> 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); > > Again, this creates unit test failures due to the same tdesc_create_reg > above. > Here offset is a little more explicit, and setting to 0 says it’s at > the start of the > description, which isn’t right. Ok. Thanks, Simon