Re: [PATCH 05/10] Class detached_regcache

Simon Marchi
2018-02-17T03:15:48Z

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