Patchwork [05/10] Class detached_regcache

login
register
mail settings
Submitter Yao Qi
Date Feb. 7, 2018, 10:32 a.m.
Message ID <1517999572-14987-6-git-send-email-yao.qi@linaro.org>
Download mbox | patch
Permalink /patch/25848/
State New
Headers show

Comments

Yao Qi - Feb. 7, 2018, 10:32 a.m.
jit.c uses the regcache in a slightly different way, the regcache dosn'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.

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(-)
Simon Marchi - Feb. 17, 2018, 3:15 a.m.
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

Patch

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);
   priv_data->this_frame = this_frame;
 
   callbacks.priv_data = priv_data;
diff --git a/gdb/record-full.c b/gdb/record-full.c
index 13a9738..82b3c74 100644
--- a/gdb/record-full.c
+++ b/gdb/record-full.c
@@ -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."));
 }
diff --git a/gdb/regcache.c b/gdb/regcache.c
index ebe3c7b..c757231 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -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];
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.  */
+
+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);