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
@@ -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);
priv_data->this_frame = this_frame;
callbacks.priv_data = priv_data;
@@ -168,7 +168,7 @@ struct record_full_core_buf_entry
};
/* Record buf with core target. */
-static gdb_byte *record_full_core_regbuf = NULL;
+static detached_regcache *record_full_core_regbuf = NULL;
static struct target_section *record_full_core_start;
static struct target_section *record_full_core_end;
static struct record_full_core_buf_entry *record_full_core_buf_list = NULL;
@@ -780,16 +780,16 @@ record_full_core_open_1 (const char *name, int from_tty)
/* Get record_full_core_regbuf. */
target_fetch_registers (regcache, -1);
- record_full_core_regbuf = (gdb_byte *) xmalloc (MAX_REGISTER_SIZE * regnum);
+ record_full_core_regbuf = new detached_regcache (regcache->arch (), false);
+
for (i = 0; i < regnum; i ++)
- regcache_raw_collect (regcache, i,
- record_full_core_regbuf + MAX_REGISTER_SIZE * i);
+ record_full_core_regbuf->raw_supply (i, *regcache);
/* Get record_full_core_start and record_full_core_end. */
if (build_section_table (core_bfd, &record_full_core_start,
&record_full_core_end))
{
- xfree (record_full_core_regbuf);
+ delete record_full_core_regbuf;
record_full_core_regbuf = NULL;
error (_("\"%s\": Can't find sections: %s"),
bfd_get_filename (core_bfd), bfd_errmsg (bfd_get_error ()));
@@ -869,7 +869,7 @@ record_full_close (struct target_ops *self)
/* Release record_full_core_regbuf. */
if (record_full_core_regbuf)
{
- xfree (record_full_core_regbuf);
+ delete record_full_core_regbuf;
record_full_core_regbuf = NULL;
}
@@ -2030,12 +2030,10 @@ record_full_core_fetch_registers (struct target_ops *ops,
int i;
for (i = 0; i < num; i ++)
- regcache_raw_supply (regcache, i,
- record_full_core_regbuf + MAX_REGISTER_SIZE * i);
+ regcache->raw_supply (i, *record_full_core_regbuf);
}
else
- regcache_raw_supply (regcache, regno,
- record_full_core_regbuf + MAX_REGISTER_SIZE * regno);
+ regcache->raw_supply (regno, *record_full_core_regbuf);
}
/* "to_prepare_to_store" method for prec over corefile. */
@@ -2054,8 +2052,7 @@ record_full_core_store_registers (struct target_ops *ops,
int regno)
{
if (record_full_gdb_operation_disable)
- regcache_raw_collect (regcache, regno,
- record_full_core_regbuf + MAX_REGISTER_SIZE * regno);
+ record_full_core_regbuf->raw_supply (regno, *regcache);
else
error (_("You can't do that without a process to debug."));
}
@@ -205,7 +205,7 @@ regcache::regcache (gdbarch *gdbarch, const address_space *aspace_,
/* The register buffers. A read-only register cache can hold the
full [0 .. gdbarch_num_regs + gdbarch_num_pseudo_regs) while a
read/write register cache can only hold [0 .. gdbarch_num_regs). */
- : readable_regcache (gdbarch, readonly_p_),
+ : detached_regcache (gdbarch, readonly_p_),
m_aspace (aspace_), m_readonly_p (readonly_p_)
{
m_ptid = minus_one_ptid;
@@ -353,13 +353,9 @@ regcache_register_status (const struct regcache *regcache, int regnum)
}
enum register_status
-regcache::get_register_status (int regnum) const
+reg_buffer::get_register_status (int regnum) const
{
- gdb_assert (regnum >= 0);
- if (m_readonly_p)
- gdb_assert (regnum < m_descr->nr_cooked_registers);
- else
- gdb_assert (regnum < num_raw_registers ());
+ assert_regnum (regnum);
return (enum register_status) m_register_status[regnum];
}
@@ -802,23 +798,6 @@ regcache_cooked_write_unsigned (struct regcache *regcache, int regnum,
regcache->cooked_write (regnum, val);
}
-/* See regcache.h. */
-
-void
-regcache_raw_set_cached_value (struct regcache *regcache, int regnum,
- const gdb_byte *buf)
-{
- regcache->raw_set_cached_value (regnum, buf);
-}
-
-void
-regcache::raw_set_cached_value (int regnum, const gdb_byte *buf)
-{
- memcpy (register_buffer (regnum), buf,
- m_descr->sizeof_register[regnum]);
- m_register_status[regnum] = REG_VALID;
-}
-
void
regcache_raw_write (struct regcache *regcache, int regnum,
const gdb_byte *buf)
@@ -848,7 +827,7 @@ regcache::raw_write (int regnum, const gdb_byte *buf)
return;
target_prepare_to_store (this);
- raw_set_cached_value (regnum, buf);
+ raw_supply (regnum, buf);
/* Invalidate the register after it is written, in case of a
failure. */
@@ -1024,13 +1003,12 @@ regcache_raw_supply (struct regcache *regcache, int regnum, const void *buf)
}
void
-regcache::raw_supply (int regnum, const void *buf)
+detached_regcache::raw_supply (int regnum, const void *buf)
{
void *regbuf;
size_t size;
assert_regnum (regnum);
- gdb_assert (!m_readonly_p);
regbuf = register_buffer (regnum);
size = m_descr->sizeof_register[regnum];
@@ -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. */
+
+class detached_regcache : public readable_regcache
+{
+public:
+ detached_regcache (gdbarch *gdbarch, bool has_pseudo)
+ : readable_regcache (gdbarch, has_pseudo)
+ {}
+
+ void raw_supply (int regnum, const void *buf);
+
+ void raw_supply (int regnum, const reg_buffer &src)
+ {
+ raw_supply (regnum, src.register_buffer (regnum));
+ }
+
+ void raw_update (int regnum) override
+ {}
+
+ DISABLE_COPY_AND_ASSIGN (detached_regcache);
+};
+
class readonly_detached_regcache;
/* The register cache for storing raw register values. */
-class regcache : public readable_regcache
+class regcache : public detached_regcache
{
public:
regcache (gdbarch *gdbarch)
@@ -339,17 +355,11 @@ public:
void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
bool is_signed) const;
- void raw_supply (int regnum, const void *buf);
-
void raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len,
bool is_signed);
void raw_supply_zeroed (int regnum);
- enum register_status get_register_status (int regnum) const;
-
- void raw_set_cached_value (int regnum, const gdb_byte *buf);
-
void invalidate (int regnum);
void raw_write_part (int regnum, int offset, int len, const gdb_byte *buf);