diff mbox

[v2,2/3] Avoid memcpys in regcache read_part/write_part for full registers.

Message ID 20180621093802.79342-3-alan.hayward@arm.com
State New, archived
Headers

Commit Message

Alan Hayward June 21, 2018, 9:38 a.m. UTC 
  Additionally, tidy up the functions: Remove asserts, use gdb_byte,
update comments.

2018-06-21  Alan Hayward  <alan.hayward@arm.com>

	* regcache.c (readable_regcache::read_part): Avoid memcpy when
	possible.
	(regcache::write_part): Likewise.
	(readable_regcache::cooked_read_part): Update comment.
	(readable_regcache::cooked_write_part): Likewise.
	* regcache.h: (readable_regcache::read_part): Likewise.
	(regcache::write_part): Likewise.
---
 gdb/regcache.c | 92 ++++++++++++++++++++++++++++++----------------------------
 gdb/regcache.h | 12 +++++---
 2 files changed, 55 insertions(+), 49 deletions(-)

Comments

Simon Marchi June 21, 2018, 1:59 p.m. UTC | #1 
On 2018-06-21 05:38 AM, Alan Hayward wrote:
> Additionally, tidy up the functions: Remove asserts, use gdb_byte,
> update comments.

Thanks, that's a nice cleanup.

Just two nits, the patch LGTM with those fixed.

> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 6276b149a2..fe4742c2ee 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -775,77 +775,75 @@ regcache::cooked_write (int regnum, const gdb_byte *buf)
>  				   regnum, buf);
>  }
>  
> -/* Perform a partial register transfer using a read, modify, write
> -   operation.  */
> +/* See regcache.h.  */
>  
>  enum register_status
> -readable_regcache::read_part (int regnum, int offset, int len, void *in,
> -			      bool is_raw)
> +readable_regcache::read_part (int regnum, int offset, int len,
> +			      gdb_byte *out, bool is_raw)
>  {
> -  struct gdbarch *gdbarch = arch ();
> -  gdb_byte *reg = (gdb_byte *) alloca (register_size (gdbarch, regnum));
> +  int reg_size = register_size (arch (), regnum);
>  
> -  gdb_assert (in != NULL);
> -  gdb_assert (offset >= 0 && offset <= m_descr->sizeof_register[regnum]);
> -  gdb_assert (len >= 0 && offset + len <= m_descr->sizeof_register[regnum]);
> -  /* Something to do?  */
> -  if (offset + len == 0)
> +  gdb_assert (out != NULL);
> +  gdb_assert (offset >= 0 && len >= 0 && offset + len <= reg_size);
> +
> +  if (offset == 0 && len == 0)
>      return REG_VALID;
> -  /* Read (when needed) ...  */
> +
> +  if (offset == 0 && len == reg_size)
> +    return (is_raw) ? raw_read (regnum, out) : cooked_read (regnum, out);

Could you add some comments to help the reader quickly know what's
going on?  Something like:

  if (offset == 0 && len == 0)
    {
      /* We don't read anything at all.  */
      return REG_VALID;
    }

  if (offset == 0 && len == reg_size)
    {
      /* We read the whole register.  */
      return (is_raw) ? raw_read (regnum, out) : cooked_read (regnum, out);
    }

  /* We read only part of the register.  */

Same for write_part.

> @@ -355,9 +358,10 @@ private:
>  			int regnum, const void *in_buf,
>  			void *out_buf, size_t size) const;
>  
> +  /* Perform a partial register transfer using a read, modify, write
> +     operation.  */
>    enum register_status write_part (int regnum, int offset, int len,
> -				   const void *out, bool is_raw);
> -
> +				   const gdb_byte *out, bool is_raw);

Rename out to in?

Thanks,

Simon
diff mbox

Patch

diff --git a/gdb/regcache.c b/gdb/regcache.c
index 6276b149a2..fe4742c2ee 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -775,77 +775,75 @@  regcache::cooked_write (int regnum, const gdb_byte *buf)
 				   regnum, buf);
 }
 
-/* Perform a partial register transfer using a read, modify, write
-   operation.  */
+/* See regcache.h.  */
 
 enum register_status
-readable_regcache::read_part (int regnum, int offset, int len, void *in,
-			      bool is_raw)
+readable_regcache::read_part (int regnum, int offset, int len,
+			      gdb_byte *out, bool is_raw)
 {
-  struct gdbarch *gdbarch = arch ();
-  gdb_byte *reg = (gdb_byte *) alloca (register_size (gdbarch, regnum));
+  int reg_size = register_size (arch (), regnum);
 
-  gdb_assert (in != NULL);
-  gdb_assert (offset >= 0 && offset <= m_descr->sizeof_register[regnum]);
-  gdb_assert (len >= 0 && offset + len <= m_descr->sizeof_register[regnum]);
-  /* Something to do?  */
-  if (offset + len == 0)
+  gdb_assert (out != NULL);
+  gdb_assert (offset >= 0 && len >= 0 && offset + len <= reg_size);
+
+  if (offset == 0 && len == 0)
     return REG_VALID;
-  /* Read (when needed) ...  */
+
+  if (offset == 0 && len == reg_size)
+    return (is_raw) ? raw_read (regnum, out) : cooked_read (regnum, out);
+
   enum register_status status;
+  gdb_byte *reg = (gdb_byte *) alloca (reg_size);
 
-  if (is_raw)
-    status = raw_read (regnum, reg);
-  else
-    status = cooked_read (regnum, reg);
+  /* Read full register to buffer.  */
+  status = (is_raw) ? raw_read (regnum, reg) : cooked_read (regnum, reg);
   if (status != REG_VALID)
     return status;
 
-  /* ... modify ...  */
-  memcpy (in, reg + offset, len);
-
+  /* Copy out.  */
+  memcpy (out, reg + offset, len);
   return REG_VALID;
 }
 
+/* See regcache.h.  */
+
 enum register_status
 regcache::write_part (int regnum, int offset, int len,
-		     const void *out, bool is_raw)
+		      const gdb_byte *in, bool is_raw)
 {
-  struct gdbarch *gdbarch = arch ();
-  gdb_byte *reg = (gdb_byte *) alloca (register_size (gdbarch, regnum));
+  int reg_size = register_size (arch (), regnum);
 
-  gdb_assert (out != NULL);
-  gdb_assert (offset >= 0 && offset <= m_descr->sizeof_register[regnum]);
-  gdb_assert (len >= 0 && offset + len <= m_descr->sizeof_register[regnum]);
-  /* Something to do?  */
-  if (offset + len == 0)
+  gdb_assert (in != NULL);
+  gdb_assert (offset >= 0 && len >= 0 && offset + len <= reg_size);
+
+  if (offset == 0 && len == 0)
     return REG_VALID;
-  /* Read (when needed) ...  */
-  if (offset > 0
-      || offset + len < m_descr->sizeof_register[regnum])
-    {
-      enum register_status status;
 
-      if (is_raw)
-	status = raw_read (regnum, reg);
-      else
-	status = cooked_read (regnum, reg);
-      if (status != REG_VALID)
-	return status;
+  if (offset == 0 && len == reg_size)
+    {
+      (is_raw) ? raw_write (regnum, in) : cooked_write (regnum, in);
+      return REG_VALID;
     }
 
-  memcpy (reg + offset, out, len);
-  /* ... write (when needed).  */
-  if (is_raw)
-    raw_write (regnum, reg);
-  else
-    cooked_write (regnum, reg);
+  enum register_status status;
+  gdb_byte *reg = (gdb_byte *) alloca (reg_size);
+
+  /* Read existing register to buffer.  */
+  status = (is_raw) ? raw_read (regnum, reg) : cooked_read (regnum, reg);
+  if (status != REG_VALID)
+    return status;
 
+  /* Update buffer, then write back to regcache.  */
+  memcpy (reg + offset, in, len);
+  is_raw ? raw_write (regnum, reg) : cooked_write (regnum, reg);
   return REG_VALID;
 }
 
+/* See regcache.h.  */
+
 enum register_status
-readable_regcache::raw_read_part (int regnum, int offset, int len, gdb_byte *buf)
+readable_regcache::raw_read_part (int regnum, int offset, int len,
+				  gdb_byte *buf)
 {
   assert_regnum (regnum);
   return read_part (regnum, offset, len, buf, true);
@@ -861,6 +859,8 @@  regcache::raw_write_part (int regnum, int offset, int len,
   write_part (regnum, offset, len, buf, true);
 }
 
+/* See regcache.h.  */
+
 enum register_status
 readable_regcache::cooked_read_part (int regnum, int offset, int len,
 				     gdb_byte *buf)
@@ -869,6 +869,8 @@  readable_regcache::cooked_read_part (int regnum, int offset, int len,
   return read_part (regnum, offset, len, buf, false);
 }
 
+/* See regcache.h.  */
+
 void
 regcache::cooked_write_part (int regnum, int offset, int len,
 			     const gdb_byte *buf)
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 35c41888a7..c17ce09dee 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -253,8 +253,11 @@  public:
   struct value *cooked_read_value (int regnum);
 
 protected:
-  enum register_status read_part (int regnum, int offset, int len, void *in,
-				  bool is_raw);
+
+  /* Perform a partial register transfer using a read, modify, write
+     operation.  Will fail if register is currently invalid.  */
+  enum register_status read_part (int regnum, int offset, int len,
+				  gdb_byte *out, bool is_raw);
 };
 
 /* Buffer of registers, can be read and written.  */
@@ -355,9 +358,10 @@  private:
 			int regnum, const void *in_buf,
 			void *out_buf, size_t size) const;
 
+  /* Perform a partial register transfer using a read, modify, write
+     operation.  */
   enum register_status write_part (int regnum, int offset, int len,
-				   const void *out, bool is_raw);
-
+				   const gdb_byte *out, bool is_raw);
 
   /* The address space of this register cache (for registers where it
      makes sense, like PC or SP).  */