[v2,02/13] regcache: Add functions suitable for regset_supply/collect.
Commit Message
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(+)
Comments
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.
@@ -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. */
@@ -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