Patchwork [v2,04/10] Add regcache raw_compare method

login
register
mail settings
Submitter Alan Hayward
Date June 8, 2018, 3:16 p.m.
Message ID <B2E1E96D-A235-4CE2-9CF9-217FBE23C50A@arm.com>
Download mbox | patch
Permalink /patch/27718/
State New
Headers show

Comments

Alan Hayward - June 8, 2018, 3:16 p.m.
> On 7 Jun 2018, at 21:55, Simon Marchi <simon.marchi@ericsson.com> wrote:

> 

> On 2018-06-06 11:16 AM, Alan Hayward wrote:

>> raw_compare returns the same as a memcmp (0 for success,

>> the diff otherwise). Kept with tristate return as this

>> feels potentally more useful than a simple true/false return.

>> (Happy to change if not).

> 

> I would err on the bool side, but I don't really mind, the important is

> that it's documented properly.  We still use many ints as bool in GDB, so

> someone seeing an int return value could easily think it's 0 -> not equal,

> 1 -> equal.



Ok, updated.

> 

> Speaking of doc, I would suggest (as in the previous patch) to centralize

> the doc in the class/struct at the root, so we don't duplicate it.


Done.

> 

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

>> 

>> gdb/

>> 	* common/common-regcache.h (raw_compare): New function.

>> 	* regcache.c (regcache::raw_compare): Likewise.

>> 	* regcache.h (regcache::raw_compare): New declaration.

>> 

>> gdbserver/

>> 	* regcache.c (regcache::raw_compare): New function.

>> 	* regcache.h (regcache::raw_compare): New declaration.

>> ---

>> gdb/common/common-regcache.h |  1 +

>> gdb/gdbserver/regcache.c     | 10 ++++++++++

>> gdb/gdbserver/regcache.h     |  5 +++++

>> gdb/regcache.c               | 15 +++++++++++++++

>> gdb/regcache.h               | 11 +++++++++++

>> 5 files changed, 42 insertions(+)

>> 

>> diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h

>> index 487da0a7fb..e91439bec5 100644

>> --- a/gdb/common/common-regcache.h

>> +++ b/gdb/common/common-regcache.h

>> @@ -67,6 +67,7 @@ struct reg_buffer_common

>>   virtual ~reg_buffer_common () = default;

>>   virtual void raw_supply (int regnum, const void *buf) = 0;

>>   virtual void raw_collect (int regnum, void *buf) const = 0;

>> +  virtual int raw_compare (int regnum, const void *buf, int offset) const = 0;

>>   virtual register_status get_register_status (int regnum) const = 0;

>> };

>> 

>> diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c

>> index 857721ee3d..9648428349 100644

>> --- a/gdb/gdbserver/regcache.c

>> +++ b/gdb/gdbserver/regcache.c

>> @@ -501,3 +501,13 @@ regcache::get_register_status (int regnum) const

>>   return REG_VALID;

>> #endif

>> }

>> +

>> +/* See gdbserver/regcache.h.  */

>> +

>> +int

>> +regcache::raw_compare (int regnum, const void *buf, int offset) const

>> +{

>> +  gdb_assert (register_size (tdesc, regnum) > offset);

> 

> Should we check ">= offset"?  I think it could be useful for some edge cases

> where we could avoid an "if (offset < size)" in the caller, if we know offset

> could be equal to size.  memcmp would return 0 (equal), which is fine.

> 


Given that memcmp can cope with this, then ok.
(I need to make sure I update the patch that uses this function too!)

> Maybe assign "register_size (tdesc, regnum)" to a variable (e.g. reg_size)

> and use it in both places?


Done.

> 

>> +  return memcmp (buf, register_data (this, regnum, 1) + offset,

>> +		 register_size (tdesc, regnum) - offset);

>> +}

>> diff --git a/gdb/gdbserver/regcache.h b/gdb/gdbserver/regcache.h

>> index 1842f1d9cf..b26f39a8ad 100644

>> --- a/gdb/gdbserver/regcache.h

>> +++ b/gdb/gdbserver/regcache.h

>> @@ -50,6 +50,11 @@ struct regcache : public reg_buffer_common

>> 

>>   void raw_collect (int regnum, void *buf) const override;

>> 

>> +  /* Compare the contents of the register stored in the regcache (ignoring the

>> +     first OFFSET bytes) to the contents of BUF (without any offset). Returns

>> +     the same result as memcmp.  */

>> +  int raw_compare (int regnum, const void *buf, int offset) const override;

>> +

>>   enum register_status get_register_status (int regnum) const override;

>> };

>> 

>> diff --git a/gdb/regcache.c b/gdb/regcache.c

>> index a914b548ca..383e355e9f 100644

>> --- a/gdb/regcache.c

>> +++ b/gdb/regcache.c

>> @@ -1082,6 +1082,21 @@ regcache::collect_regset (const struct regset *regset,

>>   transfer_regset (regset, NULL, regnum, NULL, buf, size);

>> }

>> 

>> +/* See regcache.h.  */

>> +

>> +int

>> +regcache::raw_compare (int regnum, const void *buf, int offset) const

>> +{

>> +  const char *regbuf;

>> +  size_t size;

>> +

>> +  gdb_assert (buf != NULL);

>> +  assert_regnum (regnum);

> 

> Should we assert that offset is < or <= size here too?

> 


Not sure why I didn’t add this.


Patch update with changes above. Are you ok with this version?
Simon Marchi - June 10, 2018, 10:26 p.m.
On 2018-06-08 11:16 AM, Alan Hayward wrote:
> diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h
> index 29d9a81182ad1a5797b080e136b682fe59ecef37..fe3ece7ac52d60d07d718ad617f30be2f7133b5f 100644
> --- a/gdb/common/common-regcache.h
> +++ b/gdb/common/common-regcache.h
> @@ -75,6 +75,11 @@ struct reg_buffer_common
> 
>    /* Collect register REGNUM from REGCACHE and store its contents in BUF.  */
>    virtual void raw_collect (int regnum, void *buf) const = 0;
> +
> +  /* 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.  */

True -> true (since it's written this way in C++, unlike Python for example).

LGTM with that fixed.

Simon

Patch

diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h

index 29d9a81182ad1a5797b080e136b682fe59ecef37..fe3ece7ac52d60d07d718ad617f30be2f7133b5f 100644

--- a/gdb/common/common-regcache.h

+++ b/gdb/common/common-regcache.h

@@ -75,6 +75,11 @@  struct reg_buffer_common


   /* Collect register REGNUM from REGCACHE and store its contents in BUF.  */
   virtual void raw_collect (int regnum, void *buf) const = 0;
+

+  /* 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;

 };

 #endif /* COMMON_REGCACHE_H */
diff --git a/gdb/gdbserver/regcache.h b/gdb/gdbserver/regcache.h

index 352c1df3f9eeacd622a3d5d668ff4ea98aea9c6f..b4c4c20ebd368f56f14af758301ff7d55fd16dae 100644

--- a/gdb/gdbserver/regcache.h

+++ b/gdb/gdbserver/regcache.h

@@ -54,6 +54,9 @@  struct regcache : public reg_buffer_common


   /* See common/common-regcache.h.  */
   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;

 };

 struct regcache *init_register_cache (struct regcache *regcache,
diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c

index 83837525a18c1dc96b6e9d24397064e247ced459..735ce7bccfc7a058f8604620928f3894d4655887 100644

--- a/gdb/gdbserver/regcache.c

+++ b/gdb/gdbserver/regcache.c

@@ -502,3 +502,17 @@  regcache::get_register_status (int regnum) const

   return REG_VALID;
 #endif
 }
+

+/* See common/common-regcache.h.  */

+

+bool

+regcache::raw_compare (int regnum, const void *buf, int offset) const

+{

+  gdb_assert (buf != NULL);

+

+  const unsigned char *regbuf = register_data (this, regnum, 1);

+  int size = register_size (tdesc, regnum);

+  gdb_assert (size >= offset);

+

+  return (memcmp (buf, regbuf + offset, size - offset) == 0);

+}

diff --git a/gdb/regcache.h b/gdb/regcache.h

index 4e5659b25bc530a4ced32520825136534be2e642..511864858d24a7fa53047fdc247846a551b121af 100644

--- a/gdb/regcache.h

+++ b/gdb/regcache.h

@@ -191,6 +191,10 @@  public:

     xfree (m_registers);
     xfree (m_register_status);
   }
+

+  /* See common/common-regcache.h.  */

+  bool raw_compare (int regnum, const void *buf, int offset) const override;

+

 protected:
   /* Assert on the range of REGNUM.  */
   void assert_regnum (int regnum) const;
diff --git a/gdb/regcache.c b/gdb/regcache.c

index 032fef0d34ac635c96dd6c39426f5c6d04f28095..fd0fd7318c8cca0083c897d2bd6ba1361dd627c0 100644

--- a/gdb/regcache.c

+++ b/gdb/regcache.c

@@ -1078,6 +1078,20 @@  regcache::collect_regset (const struct regset *regset,

   transfer_regset (regset, NULL, regnum, NULL, buf, size);
 }

+/* See common/common-regcache.h.  */

+

+bool

+reg_buffer::raw_compare (int regnum, const void *buf, int offset) const

+{

+  gdb_assert (buf != NULL);

+  assert_regnum (regnum);

+

+  const char *regbuf = (const char *) register_buffer (regnum);

+  size_t size = m_descr->sizeof_register[regnum];

+  gdb_assert (size >= offset);

+

+  return (memcmp (buf, regbuf + offset, size - offset) == 0);

+}


 /* Special handling for register PC.  */