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

[Simon Marchi <simon.marchi@ericsson.com>]

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