Re: [PATCH 05/10] Class detached_regcache

[Simon Marchi <simon dot marchi at polymtl dot ca>]

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  <yao.qi@linaro.org>
>
> 	* jit.c (struct jit_unwind_private) <regcache>: 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