From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2112 invoked by alias); 17 Feb 2018 03:15:48 -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 1944 invoked by uid 89); 17 Feb 2018 03:15:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=11267 X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 17 Feb 2018 03:15:31 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id w1H3FKDS024938 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Fri, 16 Feb 2018 22:15:25 -0500 Received: by simark.ca (Postfix, from userid 112) id 59F041E76C; Fri, 16 Feb 2018 22:15:20 -0500 (EST) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 5422D1E093; Fri, 16 Feb 2018 22:15:18 -0500 (EST) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Sat, 17 Feb 2018 03:15:00 -0000 From: Simon Marchi To: Yao Qi Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 05/10] Class detached_regcache In-Reply-To: <1517999572-14987-6-git-send-email-yao.qi@linaro.org> References: <1517999572-14987-1-git-send-email-yao.qi@linaro.org> <1517999572-14987-6-git-send-email-yao.qi@linaro.org> Message-ID: <3d06d590f8fcf3516ba5e497c2193453@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.4 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Sat, 17 Feb 2018 03:15:20 +0000 X-IsSubscribed: yes X-SW-Source: 2018-02/txt/msg00226.txt.bz2 Hi Yao, Just some nits. On 2018-02-07 05:32, Yao Qi wrote: > jit.c uses the regcache in a slightly different way, the regcache > dosn't "doesn't" > write through to target, but it has read and write methods. If I apply > regcache in record-full.c, it has the similar use pattern. This patch > adds a new class detached_regcache, a register buffer, but can be > red and written. "read" > > Since jit.c doesn't want to write registers through to target, it uses > regcache as a readonly regcache (because only readonly regcache > disconnects from the target), but it adds a hole in regcache > (raw_set_cached_value) in order to modify a readonly regcache. This > patch > fixes this hole completely. > > regcache inherits detached_regcache, and detached_regcache inherits > readable_regcache. The ideal design is that both detached_regcache and > readable_regcache inherit reg_buffer, and regcache inherit > detached_regcache and regcache_read (virtual inheritance). I concern > about the performance overhead of virtual inheritance, so I don't do it > in > the patch. > > gdb: > > 2017-11-28 Yao Qi > > * jit.c (struct jit_unwind_private) : Change its type to > reg_buffer_rw *. > (jit_unwind_reg_set_impl): Call raw_supply. > (jit_frame_sniffer): Use reg_buffer_rw. > * record-full.c (record_full_core_regbuf): Change its type. > (record_full_core_open_1): Use reg_buffer_rw. > (record_full_close): Likewise. > (record_full_core_fetch_registers): Use regcache->raw_supply. > (record_full_core_store_registers): Likewise. > * regcache.c (regcache::get_register_status): Move it to > reg_buffer. > (regcache_raw_set_cached_value): Remove. > (regcache::raw_set_cached_value): Remove. > (regcache::raw_write): Call raw_supply. > (regcache::raw_supply): Move it to reg_buffer_rw. > * regcache.h (regcache_raw_set_cached_value): Remove. > (reg_buffer_rw): New class. > --- > gdb/jit.c | 10 ++++------ > gdb/record-full.c | 21 +++++++++------------ > gdb/regcache.c | 32 +++++--------------------------- > gdb/regcache.h | 42 ++++++++++++++++++++++++++---------------- > 4 files changed, 44 insertions(+), 61 deletions(-) > > diff --git a/gdb/jit.c b/gdb/jit.c > index 01ead45..e67e1d2 100644 > --- a/gdb/jit.c > +++ b/gdb/jit.c > @@ -1097,7 +1097,7 @@ struct jit_unwind_private > { > /* Cached register values. See jit_frame_sniffer to see how this > works. */ > - struct regcache *regcache; > + detached_regcache *regcache; > > /* The frame being unwound. */ > struct frame_info *this_frame; > @@ -1126,7 +1126,7 @@ jit_unwind_reg_set_impl (struct > gdb_unwind_callbacks *cb, int dwarf_regnum, > return; > } > > - regcache_raw_set_cached_value (priv->regcache, gdb_reg, > value->value); > + priv->regcache->raw_supply (gdb_reg, value->value); > value->free (value); > } > > @@ -1188,7 +1188,6 @@ jit_frame_sniffer (const struct frame_unwind > *self, > struct jit_unwind_private *priv_data; > struct gdb_unwind_callbacks callbacks; > struct gdb_reader_funcs *funcs; > - struct gdbarch *gdbarch; > > callbacks.reg_get = jit_unwind_reg_get_impl; > callbacks.reg_set = jit_unwind_reg_set_impl; > @@ -1201,11 +1200,10 @@ jit_frame_sniffer (const struct frame_unwind > *self, > > gdb_assert (!*cache); > > - gdbarch = get_frame_arch (this_frame); > - > *cache = XCNEW (struct jit_unwind_private); > priv_data = (struct jit_unwind_private *) *cache; > - priv_data->regcache = new regcache (gdbarch); > + /* Take a snapshot of current regcache. */ > + priv_data->regcache = new detached_regcache (get_frame_arch > (this_frame), true); This line is too long. > diff --git a/gdb/regcache.h b/gdb/regcache.h > index d4a9d9b..9a1ac41 100644 > --- a/gdb/regcache.h > +++ b/gdb/regcache.h > @@ -68,14 +68,6 @@ extern void regcache_raw_write_unsigned (struct > regcache *regcache, > extern LONGEST regcache_raw_get_signed (struct regcache *regcache, > int regnum); > > -/* Set a raw register's value in the regcache's buffer. Unlike > - regcache_raw_write, this is not write-through. The intention is > - allowing to change the buffer contents of a read-only regcache > - allocated with new. */ > - > -extern void regcache_raw_set_cached_value > - (struct regcache *regcache, int regnum, const gdb_byte *buf); > - > /* Partial transfer of raw registers. These perform read, modify, > write style operations. The read variant returns the status of the > register. */ > @@ -229,12 +221,13 @@ public: > /* Return regcache's architecture. */ > gdbarch *arch () const; > > + enum register_status get_register_status (int regnum) const; > + > virtual ~reg_buffer () > { > xfree (m_registers); > xfree (m_register_status); > } > - > protected: > /* Assert on the range of REGNUM. */ > void assert_regnum (int regnum) const; > @@ -257,6 +250,7 @@ protected: > signed char *m_register_status; > > friend class regcache; > + friend class detached_regcache; > }; > > /* An abstract class which only has methods doing read. */ > @@ -291,11 +285,33 @@ protected: > bool is_raw); > }; > > +/* Buffer of registers, can be red and written. */ "read" Simon