From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 70954 invoked by alias); 10 Jun 2018 22:21:40 -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 70928 invoked by uid 89); 10 Jun 2018 22:21:38 -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=H*f:sk:CEE89B1, implemented, responsibility, H*i:sk:CEE89B1 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; Sun, 10 Jun 2018 22:21:30 +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 B80E11E529; Sun, 10 Jun 2018 18:21:28 -0400 (EDT) Subject: Re: [PATCH v2 03/10] Add reg_buffer_common To: Alan Hayward , Simon Marchi Cc: "gdb-patches@sourceware.org" , nd References: <20180606151629.36602-1-alan.hayward@arm.com> <20180606151629.36602-4-alan.hayward@arm.com> <17367496-22c7-d61c-6800-cbdfd856f308@ericsson.com> From: Simon Marchi Message-ID: <70bd7635-2615-0602-9b3b-a85ec9fc93f1@simark.ca> Date: Sun, 10 Jun 2018 22:21:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-SW-Source: 2018-06/txt/msg00259.txt.bz2 On 2018-06-08 10:14 AM, Alan Hayward wrote: > >> On 7 Jun 2018, at 21:18, Simon Marchi wrote: >> >> Hi Alan, >> >> Just some quick comments. >> > > > >>> --- >>> gdb/common/common-regcache.h | 8 ++++++++ >>> gdb/gdbserver/regcache.c | 47 ++++++++++++++++++++++++++++++++------------ >>> gdb/gdbserver/regcache.h | 18 +++++++++++------ >>> gdb/regcache.h | 19 ++++++++++++++---- >>> 4 files changed, 69 insertions(+), 23 deletions(-) >>> >>> diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h >>> index 696ba00955..487da0a7fb 100644 >>> --- a/gdb/common/common-regcache.h >>> +++ b/gdb/common/common-regcache.h >>> @@ -62,4 +62,12 @@ extern enum register_status regcache_raw_read_unsigned >>> >>> ULONGEST regcache_raw_get_unsigned (struct regcache *regcache, int regnum); >>> >>> +struct reg_buffer_common >>> +{ >>> + virtual ~reg_buffer_common () = default; >>> + virtual void raw_supply (int regnum, const void *buf) = 0; >>> + virtual void raw_collect (int regnum, void *buf) const = 0; >>> + virtual register_status get_register_status (int regnum) const = 0; >>> +}; >> >> Ideally, we would gather the documentation for these methods here. Where they >> are implemented/overriden, we can maybe add a reference such as >> >> /* See struct reg_buffer_common. */ >> >> ? > > Agreed. Updated all the comments for all the moved functions. > > I noticed the comment for transfer_regset had become detached from the function, > so I move it back to the right place. > >> >>> diff --git a/gdb/regcache.h b/gdb/regcache.h >>> index 3edddf47e1..b559a10752 100644 >>> --- a/gdb/regcache.h >>> +++ b/gdb/regcache.h >>> @@ -139,7 +139,7 @@ typedef struct cached_reg >>> >>> /* Buffer of registers. */ >>> >>> -class reg_buffer >>> +class reg_buffer : public reg_buffer_common >>> { >>> public: >>> reg_buffer (gdbarch *gdbarch, bool has_pseudo); >>> @@ -151,13 +151,24 @@ public: >>> >>> /* Get the availability status of the value of register REGNUM in this >>> buffer. */ >>> - enum register_status get_register_status (int regnum) const; >>> + enum register_status get_register_status (int regnum) const override; >>> >>> virtual ~reg_buffer () >>> { >>> xfree (m_registers); >>> xfree (m_register_status); >>> } >>> + >>> + virtual void raw_supply (int regnum, const void *buf) override >>> + { >>> + gdb_assert (false); >>> + } >>> + >>> + virtual void raw_collect (int regnum, void *buf) const override >>> + { >>> + gdb_assert (false); >>> + } >> >> Hmm, I understand why you need to do this right now. But what do you think of the >> idea of moving the supply and collect implementations up to reg_buffer? I think >> that the supply/collect operations are a good fit to go in reg_buffer. Essentially >> they just peek/poke in the buffer. The regcache layer's responsibility is then to >> use that register buffer to implement a cache in front of the target registers, >> and offer the API to properly read/write registers (including pseudo ones). >> >> For reference here's the patch in the regcache-for-alan branch that did this: >> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=0e04bb35765d2717818dddd88328cb975f417b2c;hp=ca9f66e37913be55abaea44813a768b40673a39a >> > > I’m happy with that. I hadn’t that there was no methods for copying in and out > of reg_buffer. Your change improves that. > > Ok, so updated with changes as above. Are you ok with this version? Yes, that LGTM, thanks. Simon