Patchwork [v2,03/10] Add reg_buffer_common

login
register
mail settings
Submitter Alan Hayward
Date June 8, 2018, 2:14 p.m.
Message ID <CEE89B18-4FE7-4171-9BBF-0FD17AECFBED@arm.com>
Download mbox | patch
Permalink /patch/27717/
State New
Headers show

Comments

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

> 

> Hi Alan,

> 

> Just some quick comments.

> 


<snip - moved to 02 thread>

>> ---

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

>> gdb/gdbserver/regcache.c     | 47 ++++++++++++++++++++++++++++++++------------

>> gdb/gdbserver/regcache.h     | 18 +++++++++++------

>> gdb/regcache.h               | 19 ++++++++++++++----

>> 4 files changed, 69 insertions(+), 23 deletions(-)

>> 

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

>> index 696ba00955..487da0a7fb 100644

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

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

>> @@ -62,4 +62,12 @@ extern enum register_status regcache_raw_read_unsigned

>> 

>> ULONGEST regcache_raw_get_unsigned (struct regcache *regcache, int regnum);

>> 

>> +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 register_status get_register_status (int regnum) const = 0;

>> +};

> 

> Ideally, we would gather the documentation for these methods here.  Where they

> are implemented/overriden, we can maybe add a reference such as

> 

>  /* See struct reg_buffer_common.  */

> 

> ?


Agreed. Updated all the comments for all the moved functions.

I noticed the comment for transfer_regset had become detached from the function,
so I move it back to the right place.

> 

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

>> index 3edddf47e1..b559a10752 100644

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

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

>> @@ -139,7 +139,7 @@ typedef struct cached_reg

>> 

>> /* Buffer of registers.  */

>> 

>> -class reg_buffer

>> +class reg_buffer : public reg_buffer_common

>> {

>> public:

>>   reg_buffer (gdbarch *gdbarch, bool has_pseudo);

>> @@ -151,13 +151,24 @@ public:

>> 

>>   /* Get the availability status of the value of register REGNUM in this

>>      buffer.  */

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

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

>> 

>>   virtual ~reg_buffer ()

>>   {

>>     xfree (m_registers);

>>     xfree (m_register_status);

>>   }

>> +

>> +  virtual void raw_supply (int regnum, const void *buf) override

>> +  {

>> +    gdb_assert (false);

>> +  }

>> +

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

>> +  {

>> +    gdb_assert (false);

>> +  }

> 

> Hmm, I understand why you need to do this right now.  But what do you think of the

> idea of moving the supply and collect implementations up to reg_buffer?  I think

> that the supply/collect operations are a good fit to go in reg_buffer. Essentially

> they just peek/poke in the buffer.  The regcache layer's responsibility is then to

> use that register buffer to implement a cache in front of the target registers,

> and offer the API to properly read/write registers (including pseudo ones).

> 

> For reference here's the patch in the regcache-for-alan branch that did this:

> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=0e04bb35765d2717818dddd88328cb975f417b2c;hp=ca9f66e37913be55abaea44813a768b40673a39a

> 


I’m happy with that. I hadn’t that there was no methods for copying in and out
of reg_buffer. Your change improves that.

Ok, so updated with changes as above. Are you ok with this version?
Simon Marchi - June 10, 2018, 10:21 p.m.
On 2018-06-08 10:14 AM, Alan Hayward wrote:
> 
>> On 7 Jun 2018, at 21:18, Simon Marchi <simon.marchi@ericsson.com> wrote:
>>
>> Hi Alan,
>>
>> Just some quick comments.
>>
> 
> <snip - moved to 02 thread>
> 
>>> ---
>>> gdb/common/common-regcache.h |  8 ++++++++
>>> gdb/gdbserver/regcache.c     | 47 ++++++++++++++++++++++++++++++++------------
>>> gdb/gdbserver/regcache.h     | 18 +++++++++++------
>>> gdb/regcache.h               | 19 ++++++++++++++----
>>> 4 files changed, 69 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h
>>> index 696ba00955..487da0a7fb 100644
>>> --- a/gdb/common/common-regcache.h
>>> +++ b/gdb/common/common-regcache.h
>>> @@ -62,4 +62,12 @@ extern enum register_status regcache_raw_read_unsigned
>>>
>>> ULONGEST regcache_raw_get_unsigned (struct regcache *regcache, int regnum);
>>>
>>> +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 register_status get_register_status (int regnum) const = 0;
>>> +};
>>
>> Ideally, we would gather the documentation for these methods here.  Where they
>> are implemented/overriden, we can maybe add a reference such as
>>
>>  /* See struct reg_buffer_common.  */
>>
>> ?
> 
> Agreed. Updated all the comments for all the moved functions.
> 
> I noticed the comment for transfer_regset had become detached from the function,
> so I move it back to the right place.
> 
>>
>>> diff --git a/gdb/regcache.h b/gdb/regcache.h
>>> index 3edddf47e1..b559a10752 100644
>>> --- a/gdb/regcache.h
>>> +++ b/gdb/regcache.h
>>> @@ -139,7 +139,7 @@ typedef struct cached_reg
>>>
>>> /* Buffer of registers.  */
>>>
>>> -class reg_buffer
>>> +class reg_buffer : public reg_buffer_common
>>> {
>>> public:
>>>   reg_buffer (gdbarch *gdbarch, bool has_pseudo);
>>> @@ -151,13 +151,24 @@ public:
>>>
>>>   /* Get the availability status of the value of register REGNUM in this
>>>      buffer.  */
>>> -  enum register_status get_register_status (int regnum) const;
>>> +  enum register_status get_register_status (int regnum) const override;
>>>
>>>   virtual ~reg_buffer ()
>>>   {
>>>     xfree (m_registers);
>>>     xfree (m_register_status);
>>>   }
>>> +
>>> +  virtual void raw_supply (int regnum, const void *buf) override
>>> +  {
>>> +    gdb_assert (false);
>>> +  }
>>> +
>>> +  virtual void raw_collect (int regnum, void *buf) const override
>>> +  {
>>> +    gdb_assert (false);
>>> +  }
>>
>> Hmm, I understand why you need to do this right now.  But what do you think of the
>> idea of moving the supply and collect implementations up to reg_buffer?  I think
>> that the supply/collect operations are a good fit to go in reg_buffer. Essentially
>> they just peek/poke in the buffer.  The regcache layer's responsibility is then to
>> use that register buffer to implement a cache in front of the target registers,
>> and offer the API to properly read/write registers (including pseudo ones).
>>
>> For reference here's the patch in the regcache-for-alan branch that did this:
>> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commitdiff;h=0e04bb35765d2717818dddd88328cb975f417b2c;hp=ca9f66e37913be55abaea44813a768b40673a39a
>>
> 
> I’m happy with that. I hadn’t that there was no methods for copying in and out
> of reg_buffer. Your change improves that.
> 
> Ok, so updated with changes as above. Are you ok with this version?

Yes, that LGTM, thanks.

Simon

Patch

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

index 696ba00955bcdec85ab2a1abd7e030cd10418524..29d9a81182ad1a5797b080e136b682fe59ecef37 100644

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

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

@@ -62,4 +62,19 @@  extern enum register_status regcache_raw_read_unsigned


 ULONGEST regcache_raw_get_unsigned (struct regcache *regcache, int regnum);

+struct reg_buffer_common

+{

+  virtual ~reg_buffer_common () = default;

+

+  /* Get the availability status of the value of register REGNUM in this

+     buffer.  */

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

+

+  /* Supply register REGNUM, whose contents are stored in BUF, to REGCACHE.  */

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

+

+  /* Collect register REGNUM from REGCACHE and store its contents in BUF.  */

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

+};

+

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

index 2c0df648f6d439517b56844d00b1bf0a1c9eafd1..352c1df3f9eeacd622a3d5d668ff4ea98aea9c6f 100644

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

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

@@ -28,23 +28,32 @@  struct target_desc;

    inferior; this is primarily for simplicity, as the performance
    benefit is minimal.  */

-struct regcache

+struct regcache : public reg_buffer_common

 {
   /* The regcache's target description.  */
-  const struct target_desc *tdesc;

+  const struct target_desc *tdesc = nullptr;


   /* Whether the REGISTERS buffer's contents are valid.  If false, we
      haven't fetched the registers from the target yet.  Not that this
      register cache is _not_ pass-through, unlike GDB's.  Note that
      "valid" here is unrelated to whether the registers are available
      in a traceframe.  For that, check REGISTER_STATUS below.  */
-  int registers_valid;

-  int registers_owned;

-  unsigned char *registers;

+  int registers_valid = 0;

+  int registers_owned = 0;

+  unsigned char *registers = nullptr;

 #ifndef IN_PROCESS_AGENT
   /* One of REG_UNAVAILBLE or REG_VALID.  */
-  unsigned char *register_status;

+  unsigned char *register_status = nullptr;

 #endif
+

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

+  enum register_status get_register_status (int regnum) const override;

+

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

+  void raw_supply (int regnum, const void *buf) override;

+

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

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

 };

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

index 0718b9f9a04d58577f71ab14887e18dc05fee018..83837525a18c1dc96b6e9d24397064e247ced459 100644

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

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

@@ -159,7 +159,7 @@  init_register_cache (struct regcache *regcache,

 struct regcache *
 new_register_cache (const struct target_desc *tdesc)
 {
-  struct regcache *regcache = XCNEW (struct regcache);

+  struct regcache *regcache = new struct regcache;


   gdb_assert (tdesc->registers_size != 0);

@@ -174,7 +174,7 @@  free_register_cache (struct regcache *regcache)

       if (regcache->registers_owned)
 	free (regcache->registers);
       free (regcache->register_status);
-      free (regcache);

+      delete regcache;

     }
 }

@@ -300,35 +300,37 @@  regcache_register_size (const struct regcache *regcache, int n)

 }

 static unsigned char *
-register_data (struct regcache *regcache, int n, int fetch)

+register_data (const struct regcache *regcache, int n, int fetch)

 {
   return (regcache->registers
 	  + find_register_by_number (regcache->tdesc, n).offset / 8);
 }

-/* Supply register N, whose contents are stored in BUF, to REGCACHE.

-   If BUF is NULL, the register's value is recorded as

-   unavailable.  */

-

 void
 supply_register (struct regcache *regcache, int n, const void *buf)
+{

+  return regcache->raw_supply (n, buf);

+}

+

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

+

+void

+regcache::raw_supply (int n, const void *buf)

 {
   if (buf)
     {
-      memcpy (register_data (regcache, n, 0), buf,

-	      register_size (regcache->tdesc, n));

+      memcpy (register_data (this, n, 0), buf, register_size (tdesc, n));

 #ifndef IN_PROCESS_AGENT
-      if (regcache->register_status != NULL)

-	regcache->register_status[n] = REG_VALID;

+      if (register_status != NULL)

+	register_status[n] = REG_VALID;

 #endif
     }
   else
     {
-      memset (register_data (regcache, n, 0), 0,

-	      register_size (regcache->tdesc, n));

+      memset (register_data (this, n, 0), 0, register_size (tdesc, n));

 #ifndef IN_PROCESS_AGENT
-      if (regcache->register_status != NULL)

-	regcache->register_status[n] = REG_UNAVAILABLE;

+      if (register_status != NULL)

+	register_status[n] = REG_UNAVAILABLE;

 #endif
     }
 }
@@ -410,8 +412,15 @@  supply_register_by_name (struct regcache *regcache,

 void
 collect_register (struct regcache *regcache, int n, void *buf)
 {
-  memcpy (buf, register_data (regcache, n, 1),

-	  register_size (regcache->tdesc, n));

+  regcache->raw_collect (n, buf);

+}

+

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

+

+void

+regcache::raw_collect (int n, void *buf) const

+{

+  memcpy (buf, register_data (this, n, 1), register_size (tdesc, n));

 }

 enum register_status
@@ -480,3 +489,16 @@  regcache_write_pc (struct regcache *regcache, CORE_ADDR pc)

 }

 #endif
+

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

+

+enum register_status

+regcache::get_register_status (int regnum) const

+{

+#ifndef IN_PROCESS_AGENT

+  gdb_assert (regnum >= 0 && regnum < tdesc->reg_defs.size ());

+  return (enum register_status) (register_status[regnum]);

+#else

+  return REG_VALID;

+#endif

+}

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

index 3edddf47e12f8e9ecb6528bdac061d4135df2d5b..4e5659b25bc530a4ced32520825136534be2e642 100644

--- a/gdb/regcache.h

+++ b/gdb/regcache.h

@@ -139,7 +139,7 @@  typedef struct cached_reg


 /* Buffer of registers.  */

-class reg_buffer

+class reg_buffer : public reg_buffer_common

 {
 public:
   reg_buffer (gdbarch *gdbarch, bool has_pseudo);
@@ -149,9 +149,42 @@  public:

   /* Return regcache's architecture.  */
   gdbarch *arch () const;

-  /* Get the availability status of the value of register REGNUM in this

-     buffer.  */

-  enum register_status get_register_status (int regnum) const;

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

+  enum register_status get_register_status (int regnum) const override;

+

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

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

+

+  /* Collect register REGNUM from REGCACHE.  Store collected value as an integer

+     at address ADDR, in target endian, with length ADDR_LEN and sign IS_SIGNED.

+     If ADDR_LEN is greater than the register size, then the integer will be

+     sign or zero extended.  If ADDR_LEN is smaller than the register size, then

+     the most significant bytes of the integer will be truncated.  */

+  void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,

+			    bool is_signed) const;

+

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

+  void raw_supply (int regnum, const void *buf) override;

+

+  void raw_supply (int regnum, const reg_buffer &src)

+  {

+    raw_supply (regnum, src.register_buffer (regnum));

+  }

+

+  /* Supply register REGNUM to REGCACHE.  Value to supply is an integer stored

+     at address ADDR, in target endian, with length ADDR_LEN and sign IS_SIGNED.

+     If the register size is greater than ADDR_LEN, then the integer will be

+     sign or zero extended.  If the register size is smaller than the integer,

+     then the most significant bytes of the integer will be truncated.  */

+  void raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len,

+			   bool is_signed);

+

+  /* Supply register REGNUM with zeroed value to REGCACHE.  This is not the same

+     as calling raw_supply with NULL (which will set the state to

+     unavailable).  */

+  void raw_supply_zeroed (int regnum);

+

+  void invalidate (int regnum);


   virtual ~reg_buffer ()
   {
@@ -234,24 +267,9 @@  public:

     : readable_regcache (gdbarch, has_pseudo)
   {}

-  /* Supply register REGNUM, whose contents are stored in BUF, to REGCACHE.  */

-  void raw_supply (int regnum, const void *buf);

-

-  void raw_supply (int regnum, const reg_buffer &src)

-  {

-    raw_supply (regnum, src.register_buffer (regnum));

-  }

-

   void raw_update (int regnum) override
   {}

-  void raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len,

-			   bool is_signed);

-

-  void raw_supply_zeroed (int regnum);

-

-  void invalidate (int regnum);

-

   DISABLE_COPY_AND_ASSIGN (detached_regcache);
 };

@@ -292,12 +310,6 @@  public:


   void raw_update (int regnum) override;

-  /* Collect register REGNUM from REGCACHE and store its contents in BUF.  */

-  void raw_collect (int regnum, void *buf) const;

-

-  void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,

-			    bool is_signed) const;

-

   /* 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);
diff --git a/gdb/regcache.c b/gdb/regcache.c

index a914b548cadce1d375f5d72b0936b2258b90e08f..032fef0d34ac635c96dd6c39426f5c6d04f28095 100644

--- a/gdb/regcache.c

+++ b/gdb/regcache.c

@@ -320,6 +320,8 @@  regcache::restore (readonly_detached_regcache *src)

     }
 }

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

+

 enum register_status
 reg_buffer::get_register_status (int regnum) const
 {
@@ -329,7 +331,7 @@  reg_buffer::get_register_status (int regnum) const

 }

 void
-detached_regcache::invalidate (int regnum)

+reg_buffer::invalidate (int regnum)

 {
   assert_regnum (regnum);
   m_register_status[regnum] = REG_UNKNOWN;
@@ -879,8 +881,10 @@  regcache::cooked_write_part (int regnum, int offset, int len,

   write_part (regnum, offset, len, buf, false);
 }

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

+

 void
-detached_regcache::raw_supply (int regnum, const void *buf)

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

 {
   void *regbuf;
   size_t size;
@@ -905,15 +909,11 @@  detached_regcache::raw_supply (int regnum, const void *buf)

     }
 }

-/* Supply register REGNUM to REGCACHE.  Value to supply is an integer stored at

-   address ADDR, in target endian, with length ADDR_LEN and sign IS_SIGNED.  If

-   the register size is greater than ADDR_LEN, then the integer will be sign or

-   zero extended.  If the register size is smaller than the integer, then the

-   most significant bytes of the integer will be truncated.  */

+/* See regcache.h.  */


 void
-detached_regcache::raw_supply_integer (int regnum, const gdb_byte *addr,

-				   int addr_len, bool is_signed)

+reg_buffer::raw_supply_integer (int regnum, const gdb_byte *addr,

+				int addr_len, bool is_signed)

 {
   enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
   gdb_byte *regbuf;
@@ -929,12 +929,10 @@  detached_regcache::raw_supply_integer (int regnum, const gdb_byte *addr,

   m_register_status[regnum] = REG_VALID;
 }

-/* Supply register REGNUM with zeroed value to REGCACHE.  This is not the same

-   as calling raw_supply with NULL (which will set the state to

-   unavailable).  */

+/* See regcache.h.  */


 void
-detached_regcache::raw_supply_zeroed (int regnum)

+reg_buffer::raw_supply_zeroed (int regnum)

 {
   void *regbuf;
   size_t size;
@@ -948,8 +946,10 @@  detached_regcache::raw_supply_zeroed (int regnum)

   m_register_status[regnum] = REG_VALID;
 }

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

+

 void
-regcache::raw_collect (int regnum, void *buf) const

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

 {
   const void *regbuf;
   size_t size;
@@ -962,19 +962,11 @@  regcache::raw_collect (int regnum, void *buf) const

   memcpy (buf, regbuf, size);
 }

-/* Transfer a single or all registers belonging to a certain register

-   set to or from a buffer.  This is the main worker function for

-   regcache_supply_regset and regcache_collect_regset.  */

-

-/* Collect register REGNUM from REGCACHE.  Store collected value as an integer

-   at address ADDR, in target endian, with length ADDR_LEN and sign IS_SIGNED.

-   If ADDR_LEN is greater than the register size, then the integer will be sign

-   or zero extended.  If ADDR_LEN is smaller than the register size, then the

-   most significant bytes of the integer will be truncated.  */

+/* See regcache.h.  */


 void
-regcache::raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,

-			       bool is_signed) const

+reg_buffer::raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,

+				 bool is_signed) const

 {
   enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
   const gdb_byte *regbuf;
@@ -989,6 +981,10 @@  regcache::raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,

 			byte_order);
 }

+/* Transfer a single or all registers belonging to a certain register

+   set to or from a buffer.  This is the main worker function for

+   regcache_supply_regset and regcache_collect_regset.  */

+

 void
 regcache::transfer_regset (const struct regset *regset,
 			   struct regcache *out_regcache,