diff mbox

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

Message ID C88DB226-B5CE-4D67-8D1D-B55F3C77CEAE@arm.com
State New, archived
Headers

Commit Message

Alan Hayward June 21, 2018, 3:19 p.m. UTC 
  > 
> 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:


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
>

Comments

Simon Marchi June 21, 2018, 3:34 p.m. UTC | #1 
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
Simon Marchi June 21, 2018, 5:32 p.m. UTC | #2 
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
Alan Hayward June 21, 2018, 7:52 p.m. UTC | #3 
> 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.
diff mbox

Patch

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