[v2,02/13] regcache: Add functions suitable for regset_supply/collect.

  These functions are intended to suit all targets that don't require too
special logic in their regset supply/collect methods.  Having such
generic functions helps reducing target-specific complexity.

gdb/
	* regcache.c: Include "regset.h".
	(regcache_supply_regset, regcache_collect_regset): New functions.
	* regcache.h (struct regcache_map_entry): New structure.
	(REGCACHE_MAP_SKIP_BYTES): New enum value.
	(regcache_supply_regset, regcache_collect_regset): New prototypes.
---
 gdb/regcache.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/regcache.h | 44 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 110 insertions(+)
  

On 25 June 2014 21:48, Andreas Arnez <arnez@linux.vnet.ibm.com> wrote:
> These functions are intended to suit all targets that don't require too
> special logic in their regset supply/collect methods.  Having such
> generic functions helps reducing target-specific complexity.
>
> gdb/
>         * regcache.c: Include "regset.h".
>         (regcache_supply_regset, regcache_collect_regset): New functions.
>         * regcache.h (struct regcache_map_entry): New structure.
>         (REGCACHE_MAP_SKIP_BYTES): New enum value.
>         (regcache_supply_regset, regcache_collect_regset): New prototypes.
> ---
>  gdb/regcache.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  gdb/regcache.h | 44 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 110 insertions(+)
>
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 5ee90b0..78fd962 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -30,6 +30,7 @@
>  #include "exceptions.h"
>  #include "remote.h"
>  #include "valprint.h"
> +#include "regset.h"
>
>  /*
>   * DATA STRUCTURE
> @@ -1068,6 +1069,71 @@ regcache_raw_collect (const struct regcache *regcache, int regnum, void *buf)
>    memcpy (buf, regbuf, size);
>  }
>
> +/* Supply register REGNUM from BUF to REGCACHE, using the register map
> +   in REGSET.  If REGNUM is -1, do this for all registers in
> +   REGSET.  */
> +
> +void
> +regcache_supply_regset (const struct regset *regset,
> +                       struct regcache *regcache,
> +                       int regnum, const void *buf, size_t size)
> +{
> +  const struct regcache_map_entry *map = regset->regmap;
> +  int offs = 0;
> +  int i, count;
> +
> +  for (i = 0; (count = map[i].count) != 0; i++)
> +    {
> +      int regno = map[i].regno;
> +
> +      if (regno == REGCACHE_MAP_SKIP_BYTES)
> +       offs += count;
> +      else
> +       for (; count--; regno++)
> +         {
> +           int new_offs = offs + regcache->descr->sizeof_register[regno];
> +
> +           if (new_offs <= size && (regnum == -1 || regnum == regno))
> +             regcache_raw_supply (regcache, regno,
> +                                  buf ? (const gdb_byte *) buf + offs
> +                                  : NULL);
> +           offs = new_offs;
> +         }
> +    }
> +}
> +
> +/* Collect register REGNUM from REGCACHE to BUF, using the register
> +   map in REGSET.  If REGNUM is -1, do this for all registers in
> +   REGSET.  */
> +
> +void
> +regcache_collect_regset (const struct regset *regset,
> +                        const struct regcache *regcache,
> +                        int regnum, void *buf, size_t size)
> +{
> +  const struct regcache_map_entry *map = regset->regmap;
> +  int offs = 0;
> +  int i, count;
> +
> +  for (i = 0; (count = map[i].count) != 0; i++)
> +    {
> +      int regno = map[i].regno;
> +
> +      if (regno == REGCACHE_MAP_SKIP_BYTES)
> +       offs += count;
> +      else
> +       for (; count--; regno++)
> +         {
> +           int new_offs = offs + regcache->descr->sizeof_register[regno];
> +
> +           if (new_offs <= size && (regnum == -1 || regnum == regno))
> +             regcache_raw_collect (regcache, regno,
> +                                   (gdb_byte *) buf + offs);
> +           offs = new_offs;
> +         }
> +    }
> +}
> +
>
>  /* Special handling for register PC.  */
>
> diff --git a/gdb/regcache.h b/gdb/regcache.h
> index 8423f57..73ee043 100644
> --- a/gdb/regcache.h
> +++ b/gdb/regcache.h
> @@ -147,6 +147,50 @@ extern void regcache_raw_supply (struct regcache *regcache,
>  extern void regcache_raw_collect (const struct regcache *regcache,
>                                   int regnum, void *buf);
>
> +/* Mapping between register numbers and offsets in a buffer, for use
> +   in the '*regset' functions below.  In an array of
> +   'regcache_map_entry' each element is interpreted like follows:
> +
> +   - If 'regno' is a register number: Map register 'regno' to the
> +     current offset (starting with 0) and increase the current offset
> +     by the register's size.  Repeat this with consecutive register
> +     numbers up to 'regno+count-1'.
> +
> +   - If 'regno' is REGCACHE_MAP_SKIP_BYTES: Add 'count' to the current
> +     offset.
> +
> +   - If count=0: End of the map.  */
> +
> +struct regcache_map_entry
> +{
> +  int count;
> +  int regno;
> +};
> +
> +/* Special value for the 'regno' field in the struct above.  */
> +
> +enum
> +  {
> +    REGCACHE_MAP_SKIP_BYTES = -1,
> +  };
> +
> +/* Transfer a set of registers (as described by REGSET) between
> +   REGCACHE and BUF.  If REGNUM == -1, transfer all registers
> +   belonging to the regset, otherwise just the register numbered
> +   REGNUM.  The REGSET's 'regmap' field must point to an array of
> +   'struct regcache_map_entry'.
> +
> +   These functions are suitable for the 'regset_supply' and
> +   'regset_collect' fields in a regset structure.  */
> +
> +extern void regcache_supply_regset (const struct regset *regset,
> +                                   struct regcache *regcache,
> +                                   int regnum, const void *buf,
> +                                   size_t size);
> +extern void regcache_collect_regset (const struct regset *regset,
> +                                    const struct regcache *regcache,
> +                                    int regnum, void *buf, size_t size);
> +
>
>  /* The type of a register.  This function is slightly more efficient
>     then its gdbarch vector counterpart since it returns a precomputed
> --
> 1.8.4.2
>

Is there a way around avoiding the loop in supply/collect where regnum
!= -1? It should be more efficient in cases where we are looking for a
register with regnum > 0.
  

On Mon, Jul 07 2014, Omair Javaid wrote:

> Is there a way around avoiding the loop in supply/collect where regnum
> != -1? It should be more efficient in cases where we are looking for a
> register with regnum > 0.

Good question.  The most straightforward way would be a register map
format where regnum is used as an index into an array of offsets, like
this:

int s390_regmap_gregset[S390_NUM_REGS] =
{
  /* Program Status Word.  */
  0x00, 0x04,
  /* General Purpose Registers.  */
  0x08, 0x0c, 0x10, 0x14,
  0x18, 0x1c, 0x20, 0x24,
  0x28, 0x2c, 0x30, 0x34,
  0x38, 0x3c, 0x40, 0x44,
  /* Access Registers.  */
  0x48, 0x4c, 0x50, 0x54,
  0x58, 0x5c, 0x60, 0x64,
  0x68, 0x6c, 0x70, 0x74,
  0x78, 0x7c, 0x80, 0x84,
  /* Floating Point Control Word.  */
  -1,
  /* Floating Point Registers.  */
  -1, -1, -1, -1, -1, -1, -1, -1,
  -1, -1, -1, -1, -1, -1, -1, -1,
  /* GPR Uppper Halves.  */
  -1, -1, -1, -1, -1, -1, -1, -1,
  -1, -1, -1, -1, -1, -1, -1, -1,
  /* GNU/Linux-specific optional "registers".  */
  0x88, -1, -1,
};

This is a real example.  For the full example refer to:

  https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/s390-tdep.c;h=72d55450225e89da4394079efac1fa33b36cb68c;hb=d91fab15e7eb04f6c9b7fee5859d8815b7aa84ee#l412

As you see, this is where the s390 implementation came from.  Then I
realized that regnum > 0 is a very rare case, and that the common case
was suboptimal with this format, because the supply/collect functions
had to iterate over *all* registers, not just those of a specific
regset.

Patch #3 in this series expresses the regmap from above like this:

static const struct regcache_map_entry s390_gregmap[] =
{
  { 1, S390_PSWM_REGNUM },
  { 1, S390_PSWA_REGNUM },
  { 16, S390_R0_REGNUM },
  { 16, S390_A0_REGNUM },
  { 1, S390_ORIG_R2_REGNUM },
  { 0 }
};

In addition to being more efficient in the common case, I also consider
this version much easier to read and maintain.

We could certainly spend more effort on supplying and collecting a
single register more efficiently.  For instance, we could offer
additional routines for that special case, perhaps in conjunction with a
preparation function that converts a regmap to an indexed-by-regnum
array.  However, I wouldn't focus on that too much before actually
making use of it.  Note that currently these functions are *always*
called with regnum == -1.

In fact, it may be more adequate to completely get rid of the parameter
regnum in the regset supply/collect functions.  Any reason why we
shouldn't?
  

On 8 July 2014 16:31, Andreas Arnez <arnez@linux.vnet.ibm.com> wrote:
> On Mon, Jul 07 2014, Omair Javaid wrote:
>
>> Is there a way around avoiding the loop in supply/collect where regnum
>> != -1? It should be more efficient in cases where we are looking for a
>> register with regnum > 0.
>
> Good question.  The most straightforward way would be a register map
> format where regnum is used as an index into an array of offsets, like
> this:
>
> int s390_regmap_gregset[S390_NUM_REGS] =
> {
>   /* Program Status Word.  */
>   0x00, 0x04,
>   /* General Purpose Registers.  */
>   0x08, 0x0c, 0x10, 0x14,
>   0x18, 0x1c, 0x20, 0x24,
>   0x28, 0x2c, 0x30, 0x34,
>   0x38, 0x3c, 0x40, 0x44,
>   /* Access Registers.  */
>   0x48, 0x4c, 0x50, 0x54,
>   0x58, 0x5c, 0x60, 0x64,
>   0x68, 0x6c, 0x70, 0x74,
>   0x78, 0x7c, 0x80, 0x84,
>   /* Floating Point Control Word.  */
>   -1,
>   /* Floating Point Registers.  */
>   -1, -1, -1, -1, -1, -1, -1, -1,
>   -1, -1, -1, -1, -1, -1, -1, -1,
>   /* GPR Uppper Halves.  */
>   -1, -1, -1, -1, -1, -1, -1, -1,
>   -1, -1, -1, -1, -1, -1, -1, -1,
>   /* GNU/Linux-specific optional "registers".  */
>   0x88, -1, -1,
> };
>
> This is a real example.  For the full example refer to:
>
>   https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/s390-tdep.c;h=72d55450225e89da4394079efac1fa33b36cb68c;hb=d91fab15e7eb04f6c9b7fee5859d8815b7aa84ee#l412
>
> As you see, this is where the s390 implementation came from.  Then I
> realized that regnum > 0 is a very rare case, and that the common case
> was suboptimal with this format, because the supply/collect functions
> had to iterate over *all* registers, not just those of a specific
> regset.
>
> Patch #3 in this series expresses the regmap from above like this:
>
> static const struct regcache_map_entry s390_gregmap[] =
> {
>   { 1, S390_PSWM_REGNUM },
>   { 1, S390_PSWA_REGNUM },
>   { 16, S390_R0_REGNUM },
>   { 16, S390_A0_REGNUM },
>   { 1, S390_ORIG_R2_REGNUM },
>   { 0 }
> };
>
> In addition to being more efficient in the common case, I also consider
> this version much easier to read and maintain.
>
> We could certainly spend more effort on supplying and collecting a
> single register more efficiently.  For instance, we could offer
> additional routines for that special case, perhaps in conjunction with a
> preparation function that converts a regmap to an indexed-by-regnum
> array.  However, I wouldn't focus on that too much before actually
> making use of it.  Note that currently these functions are *always*
> called with regnum == -1.
>
> In fact, it may be more adequate to completely get rid of the parameter
> regnum in the regset supply/collect functions.  Any reason why we
> shouldn't?
I think you are right its better off if we leave the single register
variants to target specific *-tdep where they can be retrieved using
regcache_raw_ supply/collect functions. All other options to get
around the loops wont be trivial.
  

On Tue, Jul 08 2014, Omair Javaid wrote:

> I think you are right its better off if we leave the single register
> variants to target specific *-tdep where they can be retrieved using
> regcache_raw_ supply/collect functions. All other options to get
> around the loops wont be trivial.

Hm, I'd actually prefer if the new functions could be used for any
case where registers are supplied to the regcache from a buffer, or
collected from the regcache to a buffer.

Which variants do you mean?  Do you have examples where they are used?
  

On 10 July 2014 12:54, Andreas Arnez <arnez@linux.vnet.ibm.com> wrote:
> On Tue, Jul 08 2014, Omair Javaid wrote:
>
>> I think you are right its better off if we leave the single register
>> variants to target specific *-tdep where they can be retrieved using
>> regcache_raw_ supply/collect functions. All other options to get
>> around the loops wont be trivial.
>
> Hm, I'd actually prefer if the new functions could be used for any
> case where registers are supplied to the regcache from a buffer, or
> collected from the regcache to a buffer.
>
> Which variants do you mean?  Do you have examples where they are used?
>

Here's one implementation I saw for PPC, this is done for rs6000-tdep.c

ppc_supply_reg (struct regcache *regcache, int regnum,
const gdb_byte *regs, size_t offset, int regsize)
{
  if (regnum != -1 && offset != -1)
    {
      if (regsize > 4)
{
 struct gdbarch *gdbarch = get_regcache_arch (regcache);
 int gdb_regsize = register_size (gdbarch, regnum);
 if (gdb_regsize < regsize
     && gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
   offset += regsize - gdb_regsize;
}
      regcache_raw_supply (regcache, regnum, regs + offset);
    }
}

But I agree that its better the way its been done already.