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

Simon Marchi
2018-06-21T13:52:16Z

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