[PATCH v2 0/3] Support large registers in regcache transfer_regset

[PATCH v2 0/3] Support large registers in regcache transfer_regset

[Alan Hayward]

Support core file reading/writing where the register size is either greater
or smaller than the space allocated to it in the core file.

This will prevent assert failures on Aarch64 SVE.

Whilst writing this patch I spotted potential issue for aarch64. The 32bit
CPSR register has a 64bit slot in the core file. The existing transfer_regset
did not ensure the extra 32bits were null padded.

This version addresses all the review comments for V1, and splits up into
smaller chunks.

Patch 1 will enble patch 3 to use unsigned ints.
Patch 2 is a cleanup of exisiting code.
Patch 3 fixes the two issues.

I did not update read_part/write_part to use unsigned ints as that would
have required spiralled out to quite a few target file changes.

Tested by generating cores on aarch64 and aarch64 sve, both via gdb and the
kernel, then ensuring the cores load back on the both systems.
Checked cores on x86 still look ok.
Ran make check on x86 and aarch64.


Alan Hayward (3):
  Use unsigned ints in regcache_map_entry All current uses of
    regcache_map_entry use static hard coded values.
  Avoid memcpys in regcache read_part/write_part for full registers.
  Use partial register read/writes in transfer_regset

 gdb/regcache.c | 189 ++++++++++++++++++++++++++++++++++++++++-----------------
 gdb/regcache.h |  26 ++++++--
 2 files changed, 154 insertions(+), 61 deletions(-)

-- 
2.15.2 (Apple Git-101.1)

[PATCH v2 1/3] Use unsigned ints in regcache_map_entry

[Alan Hayward]

All current uses of regcache_map_entry use static hard coded values.
Update transfer_regset which uses those values.

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

	* regcache.c (transfer_regset): Use unsigned ints.
	* regcache.h (regcache_map_entry): Likewise.
---
 gdb/regcache.c | 4 ++--
 gdb/regcache.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/gdb/regcache.c b/gdb/regcache.c
index 25436bb456..6276b149a2 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -988,14 +988,14 @@ regcache::transfer_regset (const struct regset *regset,
 			   void *out_buf, size_t size) const
 {
   const struct regcache_map_entry *map;
-  int offs = 0, count;
+  unsigned int offs = 0, count;
 
   for (map = (const struct regcache_map_entry *) regset->regmap;
        (count = map->count) != 0;
        map++)
     {
       int regno = map->regno;
-      int slot_size = map->size;
+      unsigned int slot_size = map->size;
 
       if (slot_size == 0 && regno != REGCACHE_MAP_SKIP)
 	slot_size = m_descr->sizeof_register[regno];
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 74ac8583bc..35c41888a7 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -83,9 +83,9 @@ extern void regcache_write_pc (struct regcache *regcache, CORE_ADDR pc);
 
 struct regcache_map_entry
 {
-  int count;
+  unsigned int count;
   int regno;
-  int size;
+  unsigned int size;
 };
 
 /* Special value for the 'regno' field in the struct above.  */
-- 
2.15.2 (Apple Git-101.1)

Re: [PATCH v2 1/3] Use unsigned ints in regcache_map_entry

[Simon Marchi]

On 2018-06-21 05:38 AM, Alan Hayward wrote:
> All current uses of regcache_map_entry use static hard coded values.
> Update transfer_regset which uses those values.

Can you explain what we gain from this patch?  In the previous discussion,
I mentioned that the parameters LEN and OFFSET in the regcache methods
(e.g. read_part) could be come unsigned, which would allow us to remove
the "offset >= 0 && len >= 0" assertions.  In turn, they won't be
needed in your raw_collect_part/raw_supply_part.  But I don't see
exactly what the current patch brings (though it's not incorrect).

Simon

Re: [PATCH v2 1/3] Use unsigned ints in regcache_map_entry

[Simon Marchi]

On 2018-06-21 09:27 AM, Simon Marchi wrote:
> On 2018-06-21 05:38 AM, Alan Hayward wrote:
>> All current uses of regcache_map_entry use static hard coded values.
>> Update transfer_regset which uses those values.
> 
> Can you explain what we gain from this patch?  In the previous discussion,
> I mentioned that the parameters LEN and OFFSET in the regcache methods
> (e.g. read_part) could be come unsigned, which would allow us to remove
> the "offset >= 0 && len >= 0" assertions.  In turn, they won't be
> needed in your raw_collect_part/raw_supply_part.  But I don't see
> exactly what the current patch brings (though it's not incorrect).
> 
> Simon
> 

This is what I would suggest:


From 22032e0f89b74daaa8186223508eb9d788772962 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Thu, 21 Jun 2018 09:42:05 -0400
Subject: [PATCH] Make some regcache method parameters unsigned

The parameters LEN and OFFSET in some of regcache's methods can only
have values >= 0, so we can make them unsigned.  Doing so allows us to
remove some asserts in read_part and write_part.

Also, when we have these two assertions ...

  offset <= m_descr->sizeof_register[regnum]
  offset + len <= m_descr->sizeof_register[regnum]

... if (offset + len) is smaller than the
register size, then offset is necessarily smaller than the register
size.  So we can remove the first one.

gdb/ChangeLog:

	* common/common-regcache.h (struct reg_buffer_common)
	<raw_compare>: Make OFFSET unsigned.
	* regcache.h (class reg_buffer) <raw_compare>: Likewise.
	(class readable_regcache) <raw_read_part, cooked_read_part,
	read_part>: Make OFFSET and LEN unsigned.
	(class regcache) <raw_write_part, cooked_write_part,
	write_part>: Likewise.
	* regcache.c (readable_regcache::read_part): Make OFFSET and LEN
	unsigned, remove assertions.
	(regcache::write_part): Likewise.
	(readable_regcache::raw_read_part): Make OFFSET and LEN
	unsigned.
	(regcache::raw_write_part): Likewise.
	(readable_regcache::cooked_read_part): Likewise.
	(regcache::cooked_write_part): Likewise.
	(reg_buffer::raw_compare): Make OFFSET unsigned.

gdb/gdbserver/ChangeLog:

	* regcache.h (struct regcache) <raw_compare>: Make OFFSET
	unsigned.
	* regcache.c (regcache::raw_compare): Likewise.
---
 gdb/common/common-regcache.h |  3 ++-
 gdb/gdbserver/regcache.c     |  2 +-
 gdb/gdbserver/regcache.h     |  3 ++-
 gdb/regcache.c               | 27 +++++++++++++--------------
 gdb/regcache.h               | 25 ++++++++++++++-----------
 5 files changed, 32 insertions(+), 28 deletions(-)

diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h
index 18080b2..7973254 100644
--- a/gdb/common/common-regcache.h
+++ b/gdb/common/common-regcache.h
@@ -79,7 +79,8 @@ struct reg_buffer_common
   /* Compare the contents of the register stored in the regcache (ignoring the
      first OFFSET bytes) to the contents of BUF (without any offset).  Returns
      true if the same.  */
-  virtual bool raw_compare (int regnum, const void *buf, int offset) const = 0;
+  virtual bool raw_compare (int regnum, const void *buf,
+			    unsigned int offset) const = 0;
 };

 #endif /* COMMON_REGCACHE_H */
diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
index 735ce7b..864333b 100644
--- a/gdb/gdbserver/regcache.c
+++ b/gdb/gdbserver/regcache.c
@@ -506,7 +506,7 @@ regcache::get_register_status (int regnum) const
 /* See common/common-regcache.h.  */

 bool
-regcache::raw_compare (int regnum, const void *buf, int offset) const
+regcache::raw_compare (int regnum, const void *buf, unsigned int offset) const
 {
   gdb_assert (buf != NULL);

diff --git a/gdb/gdbserver/regcache.h b/gdb/gdbserver/regcache.h
index b4c4c20..69cbeda 100644
--- a/gdb/gdbserver/regcache.h
+++ b/gdb/gdbserver/regcache.h
@@ -56,7 +56,8 @@ struct regcache : public reg_buffer_common
   void raw_collect (int regnum, void *buf) const override;

   /* See common/common-regcache.h.  */
-  bool raw_compare (int regnum, const void *buf, int offset) const override;
+  bool raw_compare (int regnum, const void *buf,
+		    unsigned int offset) const override;
 };

 struct regcache *init_register_cache (struct regcache *regcache,
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 25436bb..919f9a1 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -779,15 +779,14 @@ regcache::cooked_write (int regnum, const gdb_byte *buf)
    operation.  */

 enum register_status
-readable_regcache::read_part (int regnum, int offset, int len, void *in,
-			      bool is_raw)
+readable_regcache::read_part (int regnum, unsigned int offset, unsigned int len,
+			      void *in, bool is_raw)
 {
   struct gdbarch *gdbarch = arch ();
   gdb_byte *reg = (gdb_byte *) alloca (register_size (gdbarch, 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]);
+  gdb_assert (offset + len <= m_descr->sizeof_register[regnum]);
   /* Something to do?  */
   if (offset + len == 0)
     return REG_VALID;
@@ -808,15 +807,14 @@ readable_regcache::read_part (int regnum, int offset, int len, void *in,
 }

 enum register_status
-regcache::write_part (int regnum, int offset, int len,
-		     const void *out, bool is_raw)
+regcache::write_part (int regnum, unsigned int offset, unsigned int len,
+		      const void *out, bool is_raw)
 {
   struct gdbarch *gdbarch = arch ();
   gdb_byte *reg = (gdb_byte *) alloca (register_size (gdbarch, 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]);
+  gdb_assert (offset + len <= m_descr->sizeof_register[regnum]);
   /* Something to do?  */
   if (offset + len == 0)
     return REG_VALID;
@@ -845,7 +843,8 @@ regcache::write_part (int regnum, int offset, int len,
 }

 enum register_status
-readable_regcache::raw_read_part (int regnum, int offset, int len, gdb_byte *buf)
+readable_regcache::raw_read_part (int regnum, unsigned int offset,
+				  unsigned int len, gdb_byte *buf)
 {
   assert_regnum (regnum);
   return read_part (regnum, offset, len, buf, true);
@@ -854,7 +853,7 @@ readable_regcache::raw_read_part (int regnum, int offset, int len, gdb_byte *buf
 /* See regcache.h.  */

 void
-regcache::raw_write_part (int regnum, int offset, int len,
+regcache::raw_write_part (int regnum, unsigned int offset, unsigned int len,
 			  const gdb_byte *buf)
 {
   assert_regnum (regnum);
@@ -862,15 +861,15 @@ regcache::raw_write_part (int regnum, int offset, int len,
 }

 enum register_status
-readable_regcache::cooked_read_part (int regnum, int offset, int len,
-				     gdb_byte *buf)
+readable_regcache::cooked_read_part (int regnum, unsigned int offset,
+				     unsigned int len, gdb_byte *buf)
 {
   gdb_assert (regnum >= 0 && regnum < m_descr->nr_cooked_registers);
   return read_part (regnum, offset, len, buf, false);
 }

 void
-regcache::cooked_write_part (int regnum, int offset, int len,
+regcache::cooked_write_part (int regnum, unsigned int offset, unsigned int len,
 			     const gdb_byte *buf)
 {
   gdb_assert (regnum >= 0 && regnum < m_descr->nr_cooked_registers);
@@ -1077,7 +1076,7 @@ regcache::collect_regset (const struct regset *regset,
 /* See common/common-regcache.h.  */

 bool
-reg_buffer::raw_compare (int regnum, const void *buf, int offset) const
+reg_buffer::raw_compare (int regnum, const void *buf, unsigned int offset) const
 {
   gdb_assert (buf != NULL);
   assert_regnum (regnum);
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 74ac858..0bf7f1b 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -188,7 +188,8 @@ public:
   virtual ~reg_buffer () = default;

   /* See common/common-regcache.h.  */
-  bool raw_compare (int regnum, const void *buf, int offset) const override;
+  bool raw_compare (int regnum, const void *buf,
+		    unsigned int offset) const override;

 protected:
   /* Assert on the range of REGNUM.  */
@@ -232,8 +233,8 @@ public:
   enum register_status raw_read (int regnum, T *val);

   /* Partial transfer of raw registers.  Return the status of the register.  */
-  enum register_status raw_read_part (int regnum, int offset, int len,
-				      gdb_byte *buf);
+  enum register_status raw_read_part (int regnum, unsigned int offset,
+				      unsigned int len, gdb_byte *buf);

   /* Make certain that the register REGNUM is up-to-date.  */
   virtual void raw_update (int regnum) = 0;
@@ -245,16 +246,16 @@ public:
   enum register_status cooked_read (int regnum, T *val);

   /* Partial transfer of a cooked register.  */
-  enum register_status cooked_read_part (int regnum, int offset, int len,
-					 gdb_byte *buf);
+  enum register_status cooked_read_part (int regnum, unsigned int offset,
+					 unsigned int len, gdb_byte *buf);

   /* Read register REGNUM from the regcache and return a new value.  This
      will call mark_value_bytes_unavailable as appropriate.  */
   struct value *cooked_read_value (int regnum);

 protected:
-  enum register_status read_part (int regnum, int offset, int len, void *in,
-				  bool is_raw);
+  enum register_status read_part (int regnum, unsigned int offset,
+				  unsigned int len, void *in, bool is_raw);
 };

 /* Buffer of registers, can be read and written.  */
@@ -311,11 +312,12 @@ public:

   /* Partial transfer of raw registers.  Perform read, modify, write style
      operations.  */
-  void raw_write_part (int regnum, int offset, int len, const gdb_byte *buf);
+  void raw_write_part (int regnum, unsigned int offset, unsigned int len,
+		       const gdb_byte *buf);

   /* Partial transfer of a cooked register.  Perform read, modify, write style
      operations.  */
-  void cooked_write_part (int regnum, int offset, int len,
+  void cooked_write_part (int regnum, unsigned int offset, unsigned int len,
 			  const gdb_byte *buf);

   void supply_regset (const struct regset *regset,
@@ -355,8 +357,9 @@ private:
 			int regnum, const void *in_buf,
 			void *out_buf, size_t size) const;

-  enum register_status write_part (int regnum, int offset, int len,
-				   const void *out, bool is_raw);
+  enum register_status write_part (int regnum, unsigned int offset,
+				   unsigned int len, const void *out,
+				   bool is_raw);


   /* The address space of this register cache (for registers where it
-- 
2.7.4

Re: [PATCH v2 1/3] Use unsigned ints in regcache_map_entry

[Alan Hayward]

> 
> On 21 Jun 2018, at 14:51, Simon Marchi <simon.marchi@ericsson.com> wrote:
> 
> On 2018-06-21 09:27 AM, Simon Marchi wrote:
>> On 2018-06-21 05:38 AM, Alan Hayward wrote:
>>> All current uses of regcache_map_entry use static hard coded values.
>>> Update transfer_regset which uses those values.
>> 
>> Can you explain what we gain from this patch?  In the previous discussion,
>> I mentioned that the parameters LEN and OFFSET in the regcache methods
>> (e.g. read_part) could be come unsigned, which would allow us to remove
>> the "offset >= 0 && len >= 0" assertions.  In turn, they won't be
>> needed in your raw_collect_part/raw_supply_part.  But I don't see
>> exactly what the current patch brings (though it's not incorrect).
>> 
>> Simon
>> 

This patch allows the len and offset in part 3 to be unsigned.
... However looking again at part 3, I’ve lost the unsigned parts!
It was meant to be:

+void
+reg_buffer::raw_collect_part (int regnum, unsigned int offset, unsigned int len,
+			      gdb_byte *out) const

I then didn’t need the asserts in raw_collect_part and transfer_regset used
unsigned ints.

When I repost part 3, I’ll make sure it has these additions.


> 
> This is what I would suggest:
> 
> 

I originally wrote this for just the _part functions and then I rejected
it. The problem as I see it with this is that, mostly all the code calling
these functions today are using ints.

So, to keep it safe we should really update all the callers too. For example,
one picked at random:

--- a/gdb/m32c-tdep.c
+++ b/gdb/m32c-tdep.c
@@ -443,9 +443,9 @@ m32c_find_part (struct m32c_reg *reg, int *offset_p, int *len_p)
    bits, read the value of the REG->n'th element.  */
 static enum register_status
 m32c_part_read (struct m32c_reg *reg, readable_regcache *cache, gdb_byte *buf)
 {
-  int offset, len;
+  unsigned int offset, len;

   memset (buf, 0, TYPE_LENGTH (reg->type));
   m32c_find_part (reg, &offset, &len);
   return cache->cooked_read_part (reg->rx->num, offset, len, buf);

And without checking, I’m not sure m32c_find_part can guarantee unsigned.

Without those changes all we are doing is losing some assert protection.


Alan.

> From 22032e0f89b74daaa8186223508eb9d788772962 Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@ericsson.com>
> Date: Thu, 21 Jun 2018 09:42:05 -0400
> Subject: [PATCH] Make some regcache method parameters unsigned
> 
> The parameters LEN and OFFSET in some of regcache's methods can only
> have values >= 0, so we can make them unsigned.  Doing so allows us to
> remove some asserts in read_part and write_part.
> 
> Also, when we have these two assertions ...
> 
>  offset <= m_descr->sizeof_register[regnum]
>  offset + len <= m_descr->sizeof_register[regnum]
> 
> ... if (offset + len) is smaller than the
> register size, then offset is necessarily smaller than the register
> size.  So we can remove the first one.
> 
> gdb/ChangeLog:
> 
> 	* common/common-regcache.h (struct reg_buffer_common)
> 	<raw_compare>: Make OFFSET unsigned.
> 	* regcache.h (class reg_buffer) <raw_compare>: Likewise.
> 	(class readable_regcache) <raw_read_part, cooked_read_part,
> 	read_part>: Make OFFSET and LEN unsigned.
> 	(class regcache) <raw_write_part, cooked_write_part,
> 	write_part>: Likewise.
> 	* regcache.c (readable_regcache::read_part): Make OFFSET and LEN
> 	unsigned, remove assertions.
> 	(regcache::write_part): Likewise.
> 	(readable_regcache::raw_read_part): Make OFFSET and LEN
> 	unsigned.
> 	(regcache::raw_write_part): Likewise.
> 	(readable_regcache::cooked_read_part): Likewise.
> 	(regcache::cooked_write_part): Likewise.
> 	(reg_buffer::raw_compare): Make OFFSET unsigned.
> 
> gdb/gdbserver/ChangeLog:
> 
> 	* regcache.h (struct regcache) <raw_compare>: Make OFFSET
> 	unsigned.
> 	* regcache.c (regcache::raw_compare): Likewise.
> ---
> gdb/common/common-regcache.h |  3 ++-
> gdb/gdbserver/regcache.c     |  2 +-
> gdb/gdbserver/regcache.h     |  3 ++-
> gdb/regcache.c               | 27 +++++++++++++--------------
> gdb/regcache.h               | 25 ++++++++++++++-----------
> 5 files changed, 32 insertions(+), 28 deletions(-)
> 
> diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h
> index 18080b2..7973254 100644
> --- a/gdb/common/common-regcache.h
> +++ b/gdb/common/common-regcache.h
> @@ -79,7 +79,8 @@ struct reg_buffer_common
>   /* Compare the contents of the register stored in the regcache (ignoring the
>      first OFFSET bytes) to the contents of BUF (without any offset).  Returns
>      true if the same.  */
> -  virtual bool raw_compare (int regnum, const void *buf, int offset) const = 0;
> +  virtual bool raw_compare (int regnum, const void *buf,
> +			    unsigned int offset) const = 0;
> };
> 
> #endif /* COMMON_REGCACHE_H */
> diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
> index 735ce7b..864333b 100644
> --- a/gdb/gdbserver/regcache.c
> +++ b/gdb/gdbserver/regcache.c
> @@ -506,7 +506,7 @@ regcache::get_register_status (int regnum) const
> /* See common/common-regcache.h.  */
> 
> bool
> -regcache::raw_compare (int regnum, const void *buf, int offset) const
> +regcache::raw_compare (int regnum, const void *buf, unsigned int offset) const
> {
>   gdb_assert (buf != NULL);
> 
> diff --git a/gdb/gdbserver/regcache.h b/gdb/gdbserver/regcache.h
> index b4c4c20..69cbeda 100644
> --- a/gdb/gdbserver/regcache.h
> +++ b/gdb/gdbserver/regcache.h
> @@ -56,7 +56,8 @@ struct regcache : public reg_buffer_common
>   void raw_collect (int regnum, void *buf) const override;
> 
>   /* See common/common-regcache.h.  */
> -  bool raw_compare (int regnum, const void *buf, int offset) const override;
> +  bool raw_compare (int regnum, const void *buf,
> +		    unsigned int offset) const override;
> };
> 
> struct regcache *init_register_cache (struct regcache *regcache,
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 25436bb..919f9a1 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -779,15 +779,14 @@ regcache::cooked_write (int regnum, const gdb_byte *buf)
>    operation.  */
> 
> enum register_status
> -readable_regcache::read_part (int regnum, int offset, int len, void *in,
> -			      bool is_raw)
> +readable_regcache::read_part (int regnum, unsigned int offset, unsigned int len,
> +			      void *in, bool is_raw)
> {
>   struct gdbarch *gdbarch = arch ();
>   gdb_byte *reg = (gdb_byte *) alloca (register_size (gdbarch, 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]);
> +  gdb_assert (offset + len <= m_descr->sizeof_register[regnum]);
>   /* Something to do?  */
>   if (offset + len == 0)
>     return REG_VALID;
> @@ -808,15 +807,14 @@ readable_regcache::read_part (int regnum, int offset, int len, void *in,
> }
> 
> enum register_status
> -regcache::write_part (int regnum, int offset, int len,
> -		     const void *out, bool is_raw)
> +regcache::write_part (int regnum, unsigned int offset, unsigned int len,
> +		      const void *out, bool is_raw)
> {
>   struct gdbarch *gdbarch = arch ();
>   gdb_byte *reg = (gdb_byte *) alloca (register_size (gdbarch, 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]);
> +  gdb_assert (offset + len <= m_descr->sizeof_register[regnum]);
>   /* Something to do?  */
>   if (offset + len == 0)
>     return REG_VALID;
> @@ -845,7 +843,8 @@ regcache::write_part (int regnum, int offset, int len,
> }
> 
> enum register_status
> -readable_regcache::raw_read_part (int regnum, int offset, int len, gdb_byte *buf)
> +readable_regcache::raw_read_part (int regnum, unsigned int offset,
> +				  unsigned int len, gdb_byte *buf)
> {
>   assert_regnum (regnum);
>   return read_part (regnum, offset, len, buf, true);
> @@ -854,7 +853,7 @@ readable_regcache::raw_read_part (int regnum, int offset, int len, gdb_byte *buf
> /* See regcache.h.  */
> 
> void
> -regcache::raw_write_part (int regnum, int offset, int len,
> +regcache::raw_write_part (int regnum, unsigned int offset, unsigned int len,
> 			  const gdb_byte *buf)
> {
>   assert_regnum (regnum);
> @@ -862,15 +861,15 @@ regcache::raw_write_part (int regnum, int offset, int len,
> }
> 
> enum register_status
> -readable_regcache::cooked_read_part (int regnum, int offset, int len,
> -				     gdb_byte *buf)
> +readable_regcache::cooked_read_part (int regnum, unsigned int offset,
> +				     unsigned int len, gdb_byte *buf)
> {
>   gdb_assert (regnum >= 0 && regnum < m_descr->nr_cooked_registers);
>   return read_part (regnum, offset, len, buf, false);
> }
> 
> void
> -regcache::cooked_write_part (int regnum, int offset, int len,
> +regcache::cooked_write_part (int regnum, unsigned int offset, unsigned int len,
> 			     const gdb_byte *buf)
> {
>   gdb_assert (regnum >= 0 && regnum < m_descr->nr_cooked_registers);
> @@ -1077,7 +1076,7 @@ regcache::collect_regset (const struct regset *regset,
> /* See common/common-regcache.h.  */
> 
> bool
> -reg_buffer::raw_compare (int regnum, const void *buf, int offset) const
> +reg_buffer::raw_compare (int regnum, const void *buf, unsigned int offset) const
> {
>   gdb_assert (buf != NULL);
>   assert_regnum (regnum);
> diff --git a/gdb/regcache.h b/gdb/regcache.h
> index 74ac858..0bf7f1b 100644
> --- a/gdb/regcache.h
> +++ b/gdb/regcache.h
> @@ -188,7 +188,8 @@ public:
>   virtual ~reg_buffer () = default;
> 
>   /* See common/common-regcache.h.  */
> -  bool raw_compare (int regnum, const void *buf, int offset) const override;
> +  bool raw_compare (int regnum, const void *buf,
> +		    unsigned int offset) const override;
> 
> protected:
>   /* Assert on the range of REGNUM.  */
> @@ -232,8 +233,8 @@ public:
>   enum register_status raw_read (int regnum, T *val);
> 
>   /* Partial transfer of raw registers.  Return the status of the register.  */
> -  enum register_status raw_read_part (int regnum, int offset, int len,
> -				      gdb_byte *buf);
> +  enum register_status raw_read_part (int regnum, unsigned int offset,
> +				      unsigned int len, gdb_byte *buf);
> 
>   /* Make certain that the register REGNUM is up-to-date.  */
>   virtual void raw_update (int regnum) = 0;
> @@ -245,16 +246,16 @@ public:
>   enum register_status cooked_read (int regnum, T *val);
> 
>   /* Partial transfer of a cooked register.  */
> -  enum register_status cooked_read_part (int regnum, int offset, int len,
> -					 gdb_byte *buf);
> +  enum register_status cooked_read_part (int regnum, unsigned int offset,
> +					 unsigned int len, gdb_byte *buf);
> 
>   /* Read register REGNUM from the regcache and return a new value.  This
>      will call mark_value_bytes_unavailable as appropriate.  */
>   struct value *cooked_read_value (int regnum);
> 
> protected:
> -  enum register_status read_part (int regnum, int offset, int len, void *in,
> -				  bool is_raw);
> +  enum register_status read_part (int regnum, unsigned int offset,
> +				  unsigned int len, void *in, bool is_raw);
> };
> 
> /* Buffer of registers, can be read and written.  */
> @@ -311,11 +312,12 @@ public:
> 
>   /* Partial transfer of raw registers.  Perform read, modify, write style
>      operations.  */
> -  void raw_write_part (int regnum, int offset, int len, const gdb_byte *buf);
> +  void raw_write_part (int regnum, unsigned int offset, unsigned int len,
> +		       const gdb_byte *buf);
> 
>   /* Partial transfer of a cooked register.  Perform read, modify, write style
>      operations.  */
> -  void cooked_write_part (int regnum, int offset, int len,
> +  void cooked_write_part (int regnum, unsigned int offset, unsigned int len,
> 			  const gdb_byte *buf);
> 
>   void supply_regset (const struct regset *regset,
> @@ -355,8 +357,9 @@ private:
> 			int regnum, const void *in_buf,
> 			void *out_buf, size_t size) const;
> 
> -  enum register_status write_part (int regnum, int offset, int len,
> -				   const void *out, bool is_raw);
> +  enum register_status write_part (int regnum, unsigned int offset,
> +				   unsigned int len, const void *out,
> +				   bool is_raw);
> 
> 
>   /* The address space of this register cache (for registers where it
> -- 
> 2.7.4
> 

Re: [PATCH v2 1/3] Use unsigned ints in regcache_map_entry

[Simon Marchi]

On 2018-06-21 11:19 AM, Alan Hayward wrote:
> I originally wrote this for just the _part functions and then I rejected
> it. The problem as I see it with this is that, mostly all the code calling
> these functions today are using ints.
> 
> So, to keep it safe we should really update all the callers too. For example,
> one picked at random:
> 
> --- a/gdb/m32c-tdep.c
> +++ b/gdb/m32c-tdep.c
> @@ -443,9 +443,9 @@ m32c_find_part (struct m32c_reg *reg, int *offset_p, int *len_p)
>     bits, read the value of the REG->n'th element.  */
>  static enum register_status
>  m32c_part_read (struct m32c_reg *reg, readable_regcache *cache, gdb_byte *buf)
>  {
> -  int offset, len;
> +  unsigned int offset, len;
> 
>    memset (buf, 0, TYPE_LENGTH (reg->type));
>    m32c_find_part (reg, &offset, &len);
>    return cache->cooked_read_part (reg->rx->num, offset, len, buf);
> 
> And without checking, I’m not sure m32c_find_part can guarantee unsigned.
> 
> Without those changes all we are doing is losing some assert protection.

Fair enough, I'm fine with keeping the ints and the >= 0 asserts.  It was just
a tiny itch :).

Simon

Re: [PATCH v2 1/3] Use unsigned ints in regcache_map_entry

[Simon Marchi]

On 2018-06-21 11:34 AM, Simon Marchi wrote:
> On 2018-06-21 11:19 AM, Alan Hayward wrote:
>> I originally wrote this for just the _part functions and then I rejected
>> it. The problem as I see it with this is that, mostly all the code calling
>> these functions today are using ints.
>>
>> So, to keep it safe we should really update all the callers too. For example,
>> one picked at random:
>>
>> --- a/gdb/m32c-tdep.c
>> +++ b/gdb/m32c-tdep.c
>> @@ -443,9 +443,9 @@ m32c_find_part (struct m32c_reg *reg, int *offset_p, int *len_p)
>>     bits, read the value of the REG->n'th element.  */
>>  static enum register_status
>>  m32c_part_read (struct m32c_reg *reg, readable_regcache *cache, gdb_byte *buf)
>>  {
>> -  int offset, len;
>> +  unsigned int offset, len;
>>
>>    memset (buf, 0, TYPE_LENGTH (reg->type));
>>    m32c_find_part (reg, &offset, &len);
>>    return cache->cooked_read_part (reg->rx->num, offset, len, buf);
>>
>> And without checking, I’m not sure m32c_find_part can guarantee unsigned.
>>
>> Without those changes all we are doing is losing some assert protection.
> 
> Fair enough, I'm fine with keeping the ints and the >= 0 asserts.  It was just
> a tiny itch :).
> 
> Simon
> 

I thought about it a bit more, and we indeed probably need as many assertions
with unsigned types as we do with signed types, I was wrong thinking it would
simplify things.

Let's say a caller miscalculate "offset" and it ends up being -2 (0xfffffffe as an
unsigned int) and length is 4.
The assertion

  gdb_assert (offset + len <= reg_size)

will not catch it, since (offset + len) will still be 2 (after the overflow).  So
we would need to check that offset and len are within reg_size individually, as well
as their sum:

  gdb_assert (offset <= reg_size);
  gdb_assert (len <= reg_size);
  gdb_assert (offset + len <= reg_size);

And that is equivalent to what we would need with signed types:

  gdb_assert (offset >= 0);
  gdb_assert (len >= 0);
  gdb_assert (offset + len <= reg_size);

So in the end, I think you can forget changing things to unsigned, since it
doesn't really add value... sorry for the noise.

Simon

Re: [PATCH v2 1/3] Use unsigned ints in regcache_map_entry

[Alan Hayward]

> On 21 Jun 2018, at 18:32, Simon Marchi <simon.marchi@ericsson.com> wrote:
> 
> On 2018-06-21 11:34 AM, Simon Marchi wrote:
>> On 2018-06-21 11:19 AM, Alan Hayward wrote:
>>> I originally wrote this for just the _part functions and then I rejected
>>> it. The problem as I see it with this is that, mostly all the code calling
>>> these functions today are using ints.
>>> 
>>> So, to keep it safe we should really update all the callers too. For example,
>>> one picked at random:
>>> 
>>> --- a/gdb/m32c-tdep.c
>>> +++ b/gdb/m32c-tdep.c
>>> @@ -443,9 +443,9 @@ m32c_find_part (struct m32c_reg *reg, int *offset_p, int *len_p)
>>>    bits, read the value of the REG->n'th element.  */
>>> static enum register_status
>>> m32c_part_read (struct m32c_reg *reg, readable_regcache *cache, gdb_byte *buf)
>>> {
>>> -  int offset, len;
>>> +  unsigned int offset, len;
>>> 
>>>   memset (buf, 0, TYPE_LENGTH (reg->type));
>>>   m32c_find_part (reg, &offset, &len);
>>>   return cache->cooked_read_part (reg->rx->num, offset, len, buf);
>>> 
>>> And without checking, I’m not sure m32c_find_part can guarantee unsigned.
>>> 
>>> Without those changes all we are doing is losing some assert protection.
>> 
>> Fair enough, I'm fine with keeping the ints and the >= 0 asserts.  It was just
>> a tiny itch :).
>> 
>> Simon
>> 
> 
> I thought about it a bit more, and we indeed probably need as many assertions
> with unsigned types as we do with signed types, I was wrong thinking it would
> simplify things.
> 
> Let's say a caller miscalculate "offset" and it ends up being -2 (0xfffffffe as an
> unsigned int) and length is 4.
> The assertion
> 
>  gdb_assert (offset + len <= reg_size)
> 
> will not catch it, since (offset + len) will still be 2 (after the overflow).  So
> we would need to check that offset and len are within reg_size individually, as well
> as their sum:
> 
>  gdb_assert (offset <= reg_size);
>  gdb_assert (len <= reg_size);
>  gdb_assert (offset + len <= reg_size);
> 
> And that is equivalent to what we would need with signed types:
> 
>  gdb_assert (offset >= 0);
>  gdb_assert (len >= 0);
>  gdb_assert (offset + len <= reg_size);
> 
> So in the end, I think you can forget changing things to unsigned, since it
> doesn't really add value... sorry for the noise.
> 

Agree with this. I’ll drop this patch from the series.


Alan.

[PATCH v2 3/3] Use partial register read/writes in transfer_regset

[Alan Hayward]

This avoids assert failures when the register is bigger than the
slot size. This happens on Aarch64 when truncating Z registers
into an fpsimd structure.

Also, when the register is smaller then the slot size, then
zero pad when writing to the slot, and truncate when writing
to the regcache. This happens on Aarch64 with the CPSR register.

Continue to ensure registers are invalidated when both buffers
are null.

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

	* regcache.c (reg_buffer::raw_collect_part): New function.
	(reg_buffer::raw_supply_part): Likewise.
	(regcache::transfer_regset): Call new functions.
	* regcache.h (reg_buffer::raw_collect_part): New declaration.
	(reg_buffer::raw_supply_part): Likewise.
---
 gdb/regcache.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 gdb/regcache.h | 10 +++++++
 2 files changed, 95 insertions(+), 8 deletions(-)

diff --git a/gdb/regcache.c b/gdb/regcache.c
index fe4742c2ee..4939150c66 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -807,6 +807,38 @@ readable_regcache::read_part (int regnum, int offset, int len,
 
 /* See regcache.h.  */
 
+void
+reg_buffer::raw_collect_part (int regnum, int offset, int len,
+			      gdb_byte *out) const
+{
+  int reg_size = register_size (arch (), regnum);
+
+  gdb_assert (out != nullptr);
+  gdb_assert (offset >= 0 && len >= 0 && offset <= reg_size);
+
+  if (offset == 0 && len == 0)
+    return;
+
+  if (offset == 0 && len == reg_size)
+    return raw_collect (regnum, out);
+
+  /* Read to buffer, then write out.  */
+  gdb_byte *reg = (gdb_byte *) alloca (reg_size);
+  raw_collect (regnum, reg);
+
+  if (offset + len <= reg_size)
+    memcpy (out, reg + offset, len);
+  else
+    {
+      /* Requested region runs off the end of the register.  Clear the
+	 additional space.  */
+      memcpy (out, reg + offset, reg_size - offset);
+      memset (out + reg_size, 0, offset + len - reg_size);
+    }
+}
+
+/* See regcache.h.  */
+
 enum register_status
 regcache::write_part (int regnum, int offset, int len,
 		      const gdb_byte *in, bool is_raw)
@@ -841,6 +873,38 @@ regcache::write_part (int regnum, int offset, int len,
 
 /* See regcache.h.  */
 
+void
+reg_buffer::raw_supply_part (int regnum, int offset, int len,
+			     const gdb_byte *in)
+{
+  int reg_size = register_size (arch (), regnum);
+
+  gdb_assert (in != nullptr);
+  gdb_assert (offset >= 0 && len >= 0 && offset <= reg_size);
+
+  if (offset == 0 && len == 0)
+    return;
+
+  if (offset + len > reg_size)
+    {
+      /* Truncate length to fit the size of the regcache register.  */
+      len = reg_size - offset;
+    }
+
+  if (offset == 0 && len == reg_size)
+    return raw_supply (regnum, in);
+
+  gdb_byte *reg = (gdb_byte *) alloca (reg_size);
+
+  /* Read when needed.  */
+  if (offset > 0 || offset + len < reg_size)
+    raw_collect (regnum, reg);
+
+  /* Write to buffer, then write out.  */
+  memcpy (reg + offset, in, len);
+  raw_supply (regnum, reg);
+}
+
 enum register_status
 readable_regcache::raw_read_part (int regnum, int offset, int len,
 				  gdb_byte *buf)
@@ -1013,12 +1077,18 @@ regcache::transfer_regset (const struct regset *regset,
 	    if (offs + slot_size > size)
 	      break;
 
+	    /* Use part versions to prevent possible overflow.  */
 	    if (out_buf)
-	      raw_collect (regno, (gdb_byte *) out_buf + offs);
+	      raw_collect_part (regno, 0, slot_size,
+				(gdb_byte *) out_buf + offs);
+	    else if (in_buf)
+	      out_regcache->raw_supply_part (regno, 0, slot_size,
+					     (const gdb_byte *) in_buf + offs);
 	    else
-	      out_regcache->raw_supply (regno, in_buf
-					? (const gdb_byte *) in_buf + offs
-					: NULL);
+	      {
+	      	/* Invalidate the register.  */
+		out_regcache->raw_supply (regno, nullptr);
+	      }
 	  }
       else
 	{
@@ -1027,12 +1097,19 @@ regcache::transfer_regset (const struct regset *regset,
 	  if (offs + slot_size > size)
 	    return;
 
+	  /* Use part versions to prevent possible overflow.  */
 	  if (out_buf)
-	    raw_collect (regnum, (gdb_byte *) out_buf + offs);
+	    raw_collect_part (regnum, 0, slot_size,
+			      (gdb_byte *) out_buf + offs);
+	  else if (in_buf)
+	    out_regcache->raw_supply_part (regnum, 0, slot_size,
+					   (const gdb_byte *) in_buf + offs);
 	  else
-	    out_regcache->raw_supply (regnum, in_buf
-				      ? (const gdb_byte *) in_buf + offs
-				      : NULL);
+	    {
+	      /* Invalidate the register.  */
+	      out_regcache->raw_supply (regnum, nullptr);
+	    }
+
 	  return;
 	}
     }
diff --git a/gdb/regcache.h b/gdb/regcache.h
index c17ce09dee..a69b67d513 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -162,6 +162,11 @@ public:
   void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
 			    bool is_signed) const;
 
+  /* Collect register REGNUM from REGCACHE, starting at offset in REGCACHE,
+     reading only LEN.  If this runs off the end of the register, then fill the
+     additional space with zeros.  */
+  void raw_collect_part (int regnum, int offset, int len, gdb_byte *out) const;
+
   /* See common/common-regcache.h.  */
   void raw_supply (int regnum, const void *buf) override;
 
@@ -183,6 +188,11 @@ public:
      unavailable).  */
   void raw_supply_zeroed (int regnum);
 
+  /* Supply register REGNUM to REGCACHE, starting at offset in REGCACHE, writing
+     only LEN, without editing the rest of the register.  If the length of the
+     supplied value would overflow the register, then truncate.  */
+  void raw_supply_part (int regnum, int offset, int len, const gdb_byte *in);
+
   void invalidate (int regnum);
 
   virtual ~reg_buffer () = default;
-- 
2.15.2 (Apple Git-101.1)

Re: [PATCH v2 3/3] Use partial register read/writes in transfer_regset

[Simon Marchi]

Hi Alan,

On 2018-06-21 05:38 AM, Alan Hayward wrote:
> @@ -1013,12 +1077,18 @@ regcache::transfer_regset (const struct regset *regset,
>  	    if (offs + slot_size > size)
>  	      break;
>  
> +	    /* Use part versions to prevent possible overflow.  */
>  	    if (out_buf)

Can you update the pointer comparisons in the code you touch to use != NULL or != nullptr?

> diff --git a/gdb/regcache.h b/gdb/regcache.h
> index c17ce09dee..a69b67d513 100644
> --- a/gdb/regcache.h
> +++ b/gdb/regcache.h
> @@ -162,6 +162,11 @@ public:
>    void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
>  			    bool is_signed) const;
>  
> +  /* Collect register REGNUM from REGCACHE, starting at offset in REGCACHE,
> +     reading only LEN.  If this runs off the end of the register, then fill the
> +     additional space with zeros.  */

To have a consistent interface,  I would be tempted to use the same behavior as
read_part and write_part for reads and writes that run off the end of the register
(not allow it).  It would be the responsibility of the caller to ensure that they
don't overflow.  I think it would just be a matter of

  std::min (reg_size, slot_size)

?

Simon

Re: [PATCH v2 3/3] Use partial register read/writes in transfer_regset

[Alan Hayward]

> On 21 Jun 2018, at 15:14, Simon Marchi <simon.marchi@ericsson.com> wrote:
> 
> Hi Alan,
> 
> On 2018-06-21 05:38 AM, Alan Hayward wrote:
>> @@ -1013,12 +1077,18 @@ regcache::transfer_regset (const struct regset *regset,
>> 	    if (offs + slot_size > size)
>> 	      break;
>> 
>> +	    /* Use part versions to prevent possible overflow.  */
>> 	    if (out_buf)
> 
> Can you update the pointer comparisons in the code you touch to use != NULL or != nullptr?
> 

Will do.

>> diff --git a/gdb/regcache.h b/gdb/regcache.h
>> index c17ce09dee..a69b67d513 100644
>> --- a/gdb/regcache.h
>> +++ b/gdb/regcache.h
>> @@ -162,6 +162,11 @@ public:
>>   void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
>> 			    bool is_signed) const;
>> 
>> +  /* Collect register REGNUM from REGCACHE, starting at offset in REGCACHE,
>> +     reading only LEN.  If this runs off the end of the register, then fill the
>> +     additional space with zeros.  */
> 
> To have a consistent interface,  I would be tempted to use the same behavior as
> read_part and write_part for reads and writes that run off the end of the register
> (not allow it).  It would be the responsibility of the caller to ensure that they
> don't overflow.  I think it would just be a matter of
> 
>  std::min (reg_size, slot_size)
> 
> ?

I went back and too between doing it this way and the way I posted.
I settled on this way because it made transfer_regset neater. I agree it makes
sense to keep the interface the same. In addition I’d then have to pull the
memset of 0s into transfer_regset. I think it’ll look better if I pull the two
almost identical blobs of code into a helper function.

I’ll switch it around and repost as single V3 tomorrow, with all the unsigned stuff
dropped.


Alan.

Re: [PATCH v2 3/3] Use partial register read/writes in transfer_regset

[Simon Marchi]

On 2018-06-21 05:38 AM, Alan Hayward wrote:
> This avoids assert failures when the register is bigger than the
> slot size. This happens on Aarch64 when truncating Z registers
> into an fpsimd structure.

Oh, and can you mention in the commit log that this happens when using
generate-core-file?  It could help if somebody wants to test it in the future.

Thanks,

Simon

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

[Alan Hayward]

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(-)

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).  */
-- 
2.15.2 (Apple Git-101.1)

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

[Simon Marchi]

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