[PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE

[PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE

[Alan Hayward]

Max size set to 64bits, which I determined using the files regformats/mips*.dat

Tested on a --enable-targets=all build using make check with board files
unix and native-gdbserver.

I do not have a MIPS machine to test on.

Ok to commit?

Alan.

2017-04-04  Alan Hayward  <alan.hayward@arm.com>

	* mips-fbsd-tdep.c (mips_fbsd_supply_reg): Use MIPS_MAX_REGISTER_SIZE.
	(mips_fbsd_collect_reg): Likewise.
	* mips-linux-tdep.c (supply_32bit_reg): Likewise.
	(mips_supply_gregset): Likewise.
	(mips_supply_fpregset): Likewise.
	(mips64_supply_gregset): Likewise.
	(mips64_fill_gregset): Likewise.
	(mips64_fill_fpregset): Likewise.
	* mips-tdep.c (mips_eabi_push_dummy_call): Likewise.
	(mips_o32_return_value): Likewise.
	(print_gp_register_row): Likewise.
	* mips-tdep.h (MIPS_MAX_REGISTER_SIZE): Add



diff --git a/gdb/mips-fbsd-tdep.c b/gdb/mips-fbsd-tdep.c
index 00fae0ec60ddc9e645d3236efe29f2f9e9ceab5c..cb696f7318a9da176fee2693e484ecf48346712c 100644
--- a/gdb/mips-fbsd-tdep.c
+++ b/gdb/mips-fbsd-tdep.c
@@ -63,7 +63,7 @@ mips_fbsd_supply_reg (struct regcache *regcache, int regnum, const void *addr,
   else
     {
       enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      gdb_byte buf[MAX_REGISTER_SIZE];
+      gdb_byte buf[MIPS_MAX_REGISTER_SIZE];
       LONGEST val;

       val = extract_signed_integer ((const gdb_byte *) addr, len, byte_order);
@@ -90,7 +90,7 @@ mips_fbsd_collect_reg (const struct regcache *regcache, int regnum, void *addr,
   else
     {
       enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      gdb_byte buf[MAX_REGISTER_SIZE];
+      gdb_byte buf[MIPS_MAX_REGISTER_SIZE];
       LONGEST val;

       regcache_raw_collect (regcache, regnum, buf);
diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
index 57e75b5343e1b927e9fe28dea16759f769cf4506..ce2f378854f4b66c426fd9d6683831e8795c58a6 100644
--- a/gdb/mips-linux-tdep.c
+++ b/gdb/mips-linux-tdep.c
@@ -118,7 +118,7 @@ supply_32bit_reg (struct regcache *regcache, int regnum, const void *addr)
 {
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-  gdb_byte buf[MAX_REGISTER_SIZE];
+  gdb_byte buf[MIPS_MAX_REGISTER_SIZE];
   store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
 			extract_signed_integer ((const gdb_byte *) addr, 4,
 						byte_order));
@@ -133,10 +133,10 @@ mips_supply_gregset (struct regcache *regcache,
 {
   int regi;
   const mips_elf_greg_t *regp = *gregsetp;
-  char zerobuf[MAX_REGISTER_SIZE];
+  char zerobuf[MIPS_MAX_REGISTER_SIZE];
   struct gdbarch *gdbarch = get_regcache_arch (regcache);

-  memset (zerobuf, 0, MAX_REGISTER_SIZE);
+  memset (zerobuf, 0, MIPS_MAX_REGISTER_SIZE);

   for (regi = EF_REG0 + 1; regi <= EF_REG31; regi++)
     supply_32bit_reg (regcache, regi - EF_REG0, regp + regi);
@@ -245,9 +245,9 @@ mips_supply_fpregset (struct regcache *regcache,
 {
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   int regi;
-  char zerobuf[MAX_REGISTER_SIZE];
+  char zerobuf[MIPS_MAX_REGISTER_SIZE];

-  memset (zerobuf, 0, MAX_REGISTER_SIZE);
+  memset (zerobuf, 0, MIPS_MAX_REGISTER_SIZE);

   for (regi = 0; regi < 32; regi++)
     regcache_raw_supply (regcache,
@@ -379,10 +379,10 @@ mips64_supply_gregset (struct regcache *regcache,
 {
   int regi;
   const mips64_elf_greg_t *regp = *gregsetp;
-  gdb_byte zerobuf[MAX_REGISTER_SIZE];
+  gdb_byte zerobuf[MIPS_MAX_REGISTER_SIZE];
   struct gdbarch *gdbarch = get_regcache_arch (regcache);

-  memset (zerobuf, 0, MAX_REGISTER_SIZE);
+  memset (zerobuf, 0, MIPS_MAX_REGISTER_SIZE);

   for (regi = MIPS64_EF_REG0 + 1; regi <= MIPS64_EF_REG31; regi++)
     supply_64bit_reg (regcache, regi - MIPS64_EF_REG0,
@@ -470,7 +470,7 @@ mips64_fill_gregset (const struct regcache *regcache,

   if (regaddr != -1)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
+      gdb_byte buf[MIPS_MAX_REGISTER_SIZE];
       LONGEST val;

       regcache_raw_collect (regcache, regno, buf);
@@ -574,7 +574,7 @@ mips64_fill_fpregset (const struct regcache *regcache,
     }
   else if (regno == mips_regnum (gdbarch)->fp_control_status)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
+      gdb_byte buf[MIPS_MAX_REGISTER_SIZE];
       LONGEST val;

       regcache_raw_collect (regcache, regno, buf);
@@ -585,7 +585,7 @@ mips64_fill_fpregset (const struct regcache *regcache,
     }
   else if (regno == mips_regnum (gdbarch)->fp_implementation_revision)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
+      gdb_byte buf[MIPS_MAX_REGISTER_SIZE];
       LONGEST val;

       regcache_raw_collect (regcache, regno, buf);
diff --git a/gdb/mips-tdep.h b/gdb/mips-tdep.h
index ce168cc49cd98b05136034cdafb2154b23264d7f..de443a0586c1ac5cfd55dfe02bf0a78cfd0f7de6 100644
--- a/gdb/mips-tdep.h
+++ b/gdb/mips-tdep.h
@@ -150,7 +150,9 @@ enum
   MIPS_INSN16_SIZE = 2,
   MIPS_INSN32_SIZE = 4,
   /* The number of floating-point or integer registers.  */
-  MIPS_NUMREGS = 32
+  MIPS_NUMREGS = 32,
+  /* Big enough to hold the size of the largest register in bytes.  */
+  MIPS_MAX_REGISTER_SIZE = 8
 };

 /* Single step based on where the current instruction will take us.  */
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 41cb9d82c6ef473c1fbbf86601914f9a4f462411..51a22ba29a520639bdeb95c235c00c74ad40435b 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -4527,7 +4527,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
   for (argnum = 0; argnum < nargs; argnum++)
     {
       const gdb_byte *val;
-      gdb_byte valbuf[MAX_REGISTER_SIZE];
+      gdb_byte valbuf[MIPS_MAX_REGISTER_SIZE];
       struct value *arg = args[argnum];
       struct type *arg_type = check_typedef (value_type (arg));
       int len = TYPE_LENGTH (arg_type);
@@ -5758,7 +5758,7 @@ mips_o32_return_value (struct gdbarch *gdbarch, struct value *function,
       /* A struct that contains one or two floats.  Each value is part
          in the least significant part of their floating point
          register..  */
-      gdb_byte reg[MAX_REGISTER_SIZE];
+      gdb_byte reg[MIPS_MAX_REGISTER_SIZE];
       int regnum;
       int field;
       for (field = 0, regnum = mips_regnum (gdbarch)->fp0;
@@ -6473,7 +6473,7 @@ print_gp_register_row (struct ui_file *file, struct frame_info *frame,
 {
   struct gdbarch *gdbarch = get_frame_arch (frame);
   /* Do values for GP (int) regs.  */
-  gdb_byte raw_buffer[MAX_REGISTER_SIZE];
+  gdb_byte raw_buffer[MIPS_MAX_REGISTER_SIZE];
   int ncols = (mips_abi_regsize (gdbarch) == 8 ? 4 : 8);    /* display cols
 							       per row.  */
   int col, byte;

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE

[John Baldwin]

On Tuesday, April 04, 2017 10:12:39 AM Alan Hayward wrote:
> Max size set to 64bits, which I determined using the files regformats/mips*.dat
> 
> Tested on a --enable-targets=all build using make check with board files
> unix and native-gdbserver.
> 
> I do not have a MIPS machine to test on.
> 
> Ok to commit?

I don't know how much we (GDB) care, but keep in mind that this does mean that
these constants have to be updated as architectures change.  In my case I'm
working on a research CPU that extends MIPS with some 128 and 256-bit registers.
This means that in my GDB patches for this processor I will need to bump this
constant explicitly.  That's probably not the end of the world, but this approach
of per-arch constants vs a global constant does make that sort of thing slightly
more complex to handle.

> Alan.
> 
> 2017-04-04  Alan Hayward  <alan.hayward@arm.com>
> 
> 	* mips-fbsd-tdep.c (mips_fbsd_supply_reg): Use MIPS_MAX_REGISTER_SIZE.
> 	(mips_fbsd_collect_reg): Likewise.
> 	* mips-linux-tdep.c (supply_32bit_reg): Likewise.
> 	(mips_supply_gregset): Likewise.
> 	(mips_supply_fpregset): Likewise.
> 	(mips64_supply_gregset): Likewise.
> 	(mips64_fill_gregset): Likewise.
> 	(mips64_fill_fpregset): Likewise.
> 	* mips-tdep.c (mips_eabi_push_dummy_call): Likewise.
> 	(mips_o32_return_value): Likewise.
> 	(print_gp_register_row): Likewise.
> 	* mips-tdep.h (MIPS_MAX_REGISTER_SIZE): Add
> 
> 
> 
> diff --git a/gdb/mips-fbsd-tdep.c b/gdb/mips-fbsd-tdep.c
> index 00fae0ec60ddc9e645d3236efe29f2f9e9ceab5c..cb696f7318a9da176fee2693e484ecf48346712c 100644
> --- a/gdb/mips-fbsd-tdep.c
> +++ b/gdb/mips-fbsd-tdep.c
> @@ -63,7 +63,7 @@ mips_fbsd_supply_reg (struct regcache *regcache, int regnum, const void *addr,
>    else
>      {
>        enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> -      gdb_byte buf[MAX_REGISTER_SIZE];
> +      gdb_byte buf[MIPS_MAX_REGISTER_SIZE];
>        LONGEST val;
> 
>        val = extract_signed_integer ((const gdb_byte *) addr, len, byte_order);
> @@ -90,7 +90,7 @@ mips_fbsd_collect_reg (const struct regcache *regcache, int regnum, void *addr,
>    else
>      {
>        enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> -      gdb_byte buf[MAX_REGISTER_SIZE];
> +      gdb_byte buf[MIPS_MAX_REGISTER_SIZE];
>        LONGEST val;
> 
>        regcache_raw_collect (regcache, regnum, buf);

This part is ok with me, but I can't approve changes so you will need
additional review from an approver.

-- 
John Baldwin

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE

[Alan Hayward]

> On 4 Apr 2017, at 16:59, John Baldwin <jhb@freebsd.org> wrote:
> 
> On Tuesday, April 04, 2017 10:12:39 AM Alan Hayward wrote:
>> Max size set to 64bits, which I determined using the files regformats/mips*.dat
>> 
>> Tested on a --enable-targets=all build using make check with board files
>> unix and native-gdbserver.
>> 
>> I do not have a MIPS machine to test on.
>> 
>> Ok to commit?
> 
> I don't know how much we (GDB) care, but keep in mind that this does mean that
> these constants have to be updated as architectures change.  In my case I'm
> working on a research CPU that extends MIPS with some 128 and 256-bit registers.
> This means that in my GDB patches for this processor I will need to bump this
> constant explicitly.  That's probably not the end of the world, but this approach
> of per-arch constants vs a global constant does make that sort of thing slightly
> more complex to handle.

Understood.
The problem I’m trying to solve here is that the SVE extension for Aarch64 will
have a maximum register size of 2048 bits, which could increase in the future too.
We didn’t want to have to make MAX_REGISTER_SIZE that big!

> 
>> Alan.
>> 
>> 2017-04-04  Alan Hayward  <alan.hayward@arm.com>
>> 
>> 	* mips-fbsd-tdep.c (mips_fbsd_supply_reg): Use MIPS_MAX_REGISTER_SIZE.
>> 	(mips_fbsd_collect_reg): Likewise.
>> 	* mips-linux-tdep.c (supply_32bit_reg): Likewise.
>> 	(mips_supply_gregset): Likewise.
>> 	(mips_supply_fpregset): Likewise.
>> 	(mips64_supply_gregset): Likewise.
>> 	(mips64_fill_gregset): Likewise.
>> 	(mips64_fill_fpregset): Likewise.
>> 	* mips-tdep.c (mips_eabi_push_dummy_call): Likewise.
>> 	(mips_o32_return_value): Likewise.
>> 	(print_gp_register_row): Likewise.
>> 	* mips-tdep.h (MIPS_MAX_REGISTER_SIZE): Add
>> 
>> 
>> 
>> diff --git a/gdb/mips-fbsd-tdep.c b/gdb/mips-fbsd-tdep.c
>> index 00fae0ec60ddc9e645d3236efe29f2f9e9ceab5c..cb696f7318a9da176fee2693e484ecf48346712c 100644
>> --- a/gdb/mips-fbsd-tdep.c
>> +++ b/gdb/mips-fbsd-tdep.c
>> @@ -63,7 +63,7 @@ mips_fbsd_supply_reg (struct regcache *regcache, int regnum, const void *addr,
>>   else
>>     {
>>       enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>> -      gdb_byte buf[MAX_REGISTER_SIZE];
>> +      gdb_byte buf[MIPS_MAX_REGISTER_SIZE];
>>       LONGEST val;
>> 
>>       val = extract_signed_integer ((const gdb_byte *) addr, len, byte_order);
>> @@ -90,7 +90,7 @@ mips_fbsd_collect_reg (const struct regcache *regcache, int regnum, void *addr,
>>   else
>>     {
>>       enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>> -      gdb_byte buf[MAX_REGISTER_SIZE];
>> +      gdb_byte buf[MIPS_MAX_REGISTER_SIZE];
>>       LONGEST val;
>> 
>>       regcache_raw_collect (regcache, regnum, buf);
> 
> This part is ok with me, but I can't approve changes so you will need
> additional review from an approver.
> 

Ok, thanks for looking.

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE

[Yao Qi]

Alan Hayward <Alan.Hayward@arm.com> writes:

Hi Alan,
There are different ways of getting rid of MAX_REGISTER_SIZE, let us try
some simple approaches first.  Some uses of MAX_REGISTER_SIZE still
can't be removed, but let us start from easy part.

> diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
> index 57e75b5343e1b927e9fe28dea16759f769cf4506..ce2f378854f4b66c426fd9d6683831e8795c58a6 100644
> --- a/gdb/mips-linux-tdep.c
> +++ b/gdb/mips-linux-tdep.c
> @@ -118,7 +118,7 @@ supply_32bit_reg (struct regcache *regcache, int regnum, const void *addr)
>  {
>    struct gdbarch *gdbarch = get_regcache_arch (regcache);
>    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> -  gdb_byte buf[MAX_REGISTER_SIZE];
> +  gdb_byte buf[MIPS_MAX_REGISTER_SIZE];

Given the function name supply_32bit_reg, all registers are 32-bit here,
so we can do "buf[4]"?

> @@ -470,7 +470,7 @@ mips64_fill_gregset (const struct regcache *regcache,
>
>    if (regaddr != -1)
>      {
> -      gdb_byte buf[MAX_REGISTER_SIZE];
> +      gdb_byte buf[MIPS_MAX_REGISTER_SIZE];
>        LONGEST val;
>
>        regcache_raw_collect (regcache, regno, buf);

The "buf" is used by regcache_raw_collect + extract_signed_integer,

      regcache_raw_collect (regcache, regno, buf);
      val = extract_signed_integer (buf, register_size (gdbarch, regno),
				    byte_order);

this pattern exists in some places, so can we add a new regcache api,

LONGEST
regcache_raw_collect_signed (const struct regcache *regcache, int regnum)
{
  struct gdbarch *gdbarch = get_regcache_arch (regcache);
  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);

  return extract_signed_integer (register_buffer (regcache, regnum),
                                 register_size (gdbarch, regnum),
                                 byte_order);
}

this will remove one memory copy, as well as the local buffer "buf".

> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
> index 41cb9d82c6ef473c1fbbf86601914f9a4f462411..51a22ba29a520639bdeb95c235c00c74ad40435b 100644
> --- a/gdb/mips-tdep.c
> +++ b/gdb/mips-tdep.c
> @@ -4527,7 +4527,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>    for (argnum = 0; argnum < nargs; argnum++)
>      {
>        const gdb_byte *val;
> -      gdb_byte valbuf[MAX_REGISTER_SIZE];
> +      gdb_byte valbuf[MIPS_MAX_REGISTER_SIZE];
>        struct value *arg = args[argnum];
>        struct type *arg_type = check_typedef (value_type (arg));
>        int len = TYPE_LENGTH (arg_type);
> @@ -5758,7 +5758,7 @@ mips_o32_return_value (struct gdbarch *gdbarch, struct value *function,
>        /* A struct that contains one or two floats.  Each value is part
>           in the least significant part of their floating point
>           register..  */
> -      gdb_byte reg[MAX_REGISTER_SIZE];
> +      gdb_byte reg[MIPS_MAX_REGISTER_SIZE];

"reg" is not used at all, we can remove it.

>        int regnum;
>        int field;
>        for (field = 0, regnum = mips_regnum (gdbarch)->fp0;
> @@ -6473,7 +6473,7 @@ print_gp_register_row (struct ui_file *file, struct frame_info *frame,
>  {
>    struct gdbarch *gdbarch = get_frame_arch (frame);
>    /* Do values for GP (int) regs.  */
> -  gdb_byte raw_buffer[MAX_REGISTER_SIZE];
> +  gdb_byte raw_buffer[MIPS_MAX_REGISTER_SIZE];
>    int ncols = (mips_abi_regsize (gdbarch) == 8 ? 4 : 8);    /* display cols
>  							       per row.  */

"raw_buffer" is used in deprecated_frame_register_read, so can we use
get_frame_register_value instead? so that "raw_buffer" is not needed.

-- 
Yao (齐尧)

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (1/4)

[Alan Hayward]

> On 11 Apr 2017, at 16:37, Yao Qi <qiyaoltc@gmail.com> wrote:
> 
> Alan Hayward <Alan.Hayward@arm.com> writes:
> 
> Hi Alan,
> There are different ways of getting rid of MAX_REGISTER_SIZE, let us try
> some simple approaches first.  Some uses of MAX_REGISTER_SIZE still
> can't be removed, but let us start from easy part.
> 

Ok, patch split into multiple parts.

This part uses the new raw_supply_zeroed.

Tested on a --enable-targets=all build using make check with board files
unix and native-gdbserver.
I do not have a MIPS machine to test on.

Ok to commit?

2017-05-05  Alan Hayward  <alan.hayward@arm.com>

	* mips-linux-tdep.c (mips_supply_gregset): Use raw_supply_zeroed.
	(mips_supply_fpregset): Likewise.
	(mips64_supply_gregset): Likewise.

diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
index 57e75b5343e1b927e9fe28dea16759f769cf4506..48a582a16c934abe6e8f87c46a6009649c606d49 100644
--- a/gdb/mips-linux-tdep.c
+++ b/gdb/mips-linux-tdep.c
@@ -133,11 +133,8 @@ mips_supply_gregset (struct regcache *regcache,
 {
   int regi;
   const mips_elf_greg_t *regp = *gregsetp;
-  char zerobuf[MAX_REGISTER_SIZE];
   struct gdbarch *gdbarch = get_regcache_arch (regcache);

-  memset (zerobuf, 0, MAX_REGISTER_SIZE);
-
   for (regi = EF_REG0 + 1; regi <= EF_REG31; regi++)
     supply_32bit_reg (regcache, regi - EF_REG0, regp + regi);

@@ -156,7 +153,7 @@ mips_supply_gregset (struct regcache *regcache,
 		    regp + EF_CP0_CAUSE);

   /* Fill the inaccessible zero register with zero.  */
-  regcache_raw_supply (regcache, MIPS_ZERO_REGNUM, zerobuf);
+  regcache->raw_supply_zeroed (MIPS_ZERO_REGNUM);
 }

 static void
@@ -245,9 +242,6 @@ mips_supply_fpregset (struct regcache *regcache,
 {
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   int regi;
-  char zerobuf[MAX_REGISTER_SIZE];
-
-  memset (zerobuf, 0, MAX_REGISTER_SIZE);

   for (regi = 0; regi < 32; regi++)
     regcache_raw_supply (regcache,
@@ -259,9 +253,8 @@ mips_supply_fpregset (struct regcache *regcache,
 		       *fpregsetp + 32);

   /* FIXME: how can we supply FCRIR?  The ABI doesn't tell us.  */
-  regcache_raw_supply (regcache,
-		       mips_regnum (gdbarch)->fp_implementation_revision,
-		       zerobuf);
+  regcache->raw_supply_zeroed
+    (mips_regnum (gdbarch)->fp_implementation_revision);
 }

 static void
@@ -379,11 +372,8 @@ mips64_supply_gregset (struct regcache *regcache,
 {
   int regi;
   const mips64_elf_greg_t *regp = *gregsetp;
-  gdb_byte zerobuf[MAX_REGISTER_SIZE];
   struct gdbarch *gdbarch = get_regcache_arch (regcache);

-  memset (zerobuf, 0, MAX_REGISTER_SIZE);
-
   for (regi = MIPS64_EF_REG0 + 1; regi <= MIPS64_EF_REG31; regi++)
     supply_64bit_reg (regcache, regi - MIPS64_EF_REG0,
 		      (const gdb_byte *) (regp + regi));
@@ -407,7 +397,7 @@ mips64_supply_gregset (struct regcache *regcache,
 		    (const gdb_byte *) (regp + MIPS64_EF_CP0_CAUSE));

   /* Fill the inaccessible zero register with zero.  */
-  regcache_raw_supply (regcache, MIPS_ZERO_REGNUM, zerobuf);
+  regcache->raw_supply_zeroed (MIPS_ZERO_REGNUM);
 }

 static void

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (4/4)

[Alan Hayward]

> On 11 Apr 2017, at 16:37, Yao Qi <qiyaoltc@gmail.com> wrote:
> 
> Alan Hayward <Alan.Hayward@arm.com> writes:
> 
> Hi Alan,
> There are different ways of getting rid of MAX_REGISTER_SIZE, let us try
> some simple approaches first.  Some uses of MAX_REGISTER_SIZE still
> can't be removed, but let us start from easy part.


Patch split into multiple parts.

In this part mips_eabi_push_dummy_call is extracting an address from the
register. I've hardcoded the buffer to the max address size and added an
assert. I've used the value 8, let me know if there is a #define to use.

Tested on a --enable-targets=all build using make check with board files
unix and native-gdbserver.
I do not have a MIPS machine to test on.

Ok to commit?

2017-05-05  Alan Hayward  <alan.hayward@arm.com>

	* mips-tdep.c (mips_eabi_push_dummy_call): Hard code buffer size.


diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 7b247eac40e26599b80cd45187417282d03dcc85..145ac7fdc9b5335ff527c5c5c0ede8dc981c5880 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -4528,7 +4528,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
   for (argnum = 0; argnum < nargs; argnum++)
     {
       const gdb_byte *val;
-      gdb_byte valbuf[MAX_REGISTER_SIZE];
+      gdb_byte valbuf[8];
       struct value *arg = args[argnum];
       struct type *arg_type = check_typedef (value_type (arg));
       int len = TYPE_LENGTH (arg_type);
@@ -4544,6 +4544,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
       if (len > regsize
 	  && (typecode == TYPE_CODE_STRUCT || typecode == TYPE_CODE_UNION))
 	{
+	  gdb_assert (regsize <= 8);
 	  store_unsigned_integer (valbuf, regsize, byte_order,
 				  value_address (arg));
 	  typecode = TYPE_CODE_PTR;

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (3/4)

[Alan Hayward]

> On 11 Apr 2017, at 16:37, Yao Qi <qiyaoltc@gmail.com> wrote:
> 
> Alan Hayward <Alan.Hayward@arm.com> writes:
> 
> Hi Alan,
> There are different ways of getting rid of MAX_REGISTER_SIZE, let us try
> some simple approaches first.  Some uses of MAX_REGISTER_SIZE still
> can't be removed, but let us start from easy part.
> 

Patch split into multiple parts.

This part removes an unused and commented out buffer.
This part also replaces a use of deprecated_frame_register_read with
get_frame_register_value.

Tested on a --enable-targets=all build using make check with board files
unix and native-gdbserver.
I do not have a MIPS machine to test on.

Ok to commit?

2017-05-05  Alan Hayward  <alan.hayward@arm.com>

	* mips-tdep.c (mips_o32_return_value): Remove unused buffer.
	(print_gp_register_row): Use get_frame_register_value.


diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index f8e68090cfafbe4339b691ceda085a7ee7ffbc14..2f5a90be46b568826dc44cad7d8935afa1794cd1 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -5759,7 +5759,6 @@ mips_o32_return_value (struct gdbarch *gdbarch, struct value *function,
       /* A struct that contains one or two floats.  Each value is part
          in the least significant part of their floating point
          register..  */
-      gdb_byte reg[MAX_REGISTER_SIZE];
       int regnum;
       int field;
       for (field = 0, regnum = mips_regnum (gdbarch)->fp0;
@@ -6474,7 +6473,8 @@ print_gp_register_row (struct ui_file *file, struct frame_info *frame,
 {
   struct gdbarch *gdbarch = get_frame_arch (frame);
   /* Do values for GP (int) regs.  */
-  gdb_byte raw_buffer[MAX_REGISTER_SIZE];
+  const gdb_byte *raw_buffer;
+  struct value *value;
   int ncols = (mips_abi_regsize (gdbarch) == 8 ? 4 : 8);    /* display cols
 							       per row.  */
   int col, byte;
@@ -6533,9 +6533,12 @@ print_gp_register_row (struct ui_file *file, struct frame_info *frame,
 	break;			/* End row: large register.  */

       /* OK: get the data in raw format.  */
-      if (!deprecated_frame_register_read (frame, regnum, raw_buffer))
+      value = get_frame_register_value (frame, regnum);
+      if (value_optimized_out (value)
+	|| !value_entirely_available (value))
 	error (_("can't read register %d (%s)"),
 	       regnum, gdbarch_register_name (gdbarch, regnum));
+      raw_buffer = value_contents_all (value);
       /* pad small registers */
       for (byte = 0;
 	   byte < (mips_abi_regsize (gdbarch)

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Alan Hayward]

> On 11 Apr 2017, at 16:37, Yao Qi <qiyaoltc@gmail.com> wrote:
> 
> Alan Hayward <Alan.Hayward@arm.com> writes:
> 
> Hi Alan,
> There are different ways of getting rid of MAX_REGISTER_SIZE, let us try
> some simple approaches first.  Some uses of MAX_REGISTER_SIZE still
> can't be removed, but let us start from easy part.
> 

Patch split into multiple parts.

This part adds two new regcache functions: raw_supply_signed and 
raw_collect_signed.

Tested on a --enable-targets=all build using make check with board files
unix and native-gdbserver.
I do not have a MIPS machine to test on.

Ok to commit?

2017-05-05  Alan Hayward  <alan.hayward@arm.com>

	* mips-fbsd-tdep.c (mips_fbsd_supply_reg): Use raw_supply_signed.
	(mips_fbsd_collect_reg): Use raw_collect_signed.
	* mips-linux-tdep.c (supply_32bit_reg): Use raw_supply_signed.
	(mips64_fill_gregset): Use raw_collect_signed.
	(mips64_fill_fpregset): Use raw_supply_signed.
	* regcache.c (regcache::raw_supply_signed): New function.
	(regcache::raw_collect_signed): New function.
	* regcache.h (regcache::raw_supply_signed): New declaration.
	(regcache::raw_collect_signed): New declaration.


Last login: Fri May  5 08:55:03 on ttys000
alahay01@C02N40NHFH04:~
[08:56:55] $ cat Downloads/rb7439.patch
diff --git a/gdb/mips-fbsd-tdep.c b/gdb/mips-fbsd-tdep.c
index 00fae0ec60ddc9e645d3236efe29f2f9e9ceab5c..4785bf9a115fa74fb7f48eac388a7e3df2f0304d 100644
--- a/gdb/mips-fbsd-tdep.c
+++ b/gdb/mips-fbsd-tdep.c
@@ -63,13 +63,10 @@ mips_fbsd_supply_reg (struct regcache *regcache, int regnum, const void *addr,
   else
     {
       enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      gdb_byte buf[MAX_REGISTER_SIZE];
       LONGEST val;

       val = extract_signed_integer ((const gdb_byte *) addr, len, byte_order);
-      store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
-			    val);
-      regcache_raw_supply (regcache, regnum, buf);
+      regcache->raw_supply_signed (regnum, val);
     }
 }

@@ -90,12 +87,7 @@ mips_fbsd_collect_reg (const struct regcache *regcache, int regnum, void *addr,
   else
     {
       enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regnum, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regnum),
-				    byte_order);
+      LONGEST val = regcache->raw_collect_signed (regnum);
       store_signed_integer ((gdb_byte *) addr, len, byte_order, val);
     }
 }
diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
index 48a582a16c934abe6e8f87c46a6009649c606d49..5325d75420a71451b63cbadbf4e33d4fcf233fa4 100644
--- a/gdb/mips-linux-tdep.c
+++ b/gdb/mips-linux-tdep.c
@@ -118,11 +118,10 @@ supply_32bit_reg (struct regcache *regcache, int regnum, const void *addr)
 {
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-  gdb_byte buf[MAX_REGISTER_SIZE];
-  store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
-			extract_signed_integer ((const gdb_byte *) addr, 4,
-						byte_order));
-  regcache_raw_supply (regcache, regnum, buf);
+  LONGEST val;
+
+  val = extract_signed_integer ((const gdb_byte *) addr, 4, byte_order);
+  regcache->raw_supply_signed (regnum, val);
 }

 /* Unpack an elf_gregset_t into GDB's register cache.  */
@@ -460,12 +459,7 @@ mips64_fill_gregset (const struct regcache *regcache,

   if (regaddr != -1)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
+      LONGEST val = regcache->raw_collect_signed (regno);
       dst = regp + regaddr;
       store_signed_integer ((gdb_byte *) dst, 8, byte_order, val);
     }
@@ -564,23 +558,13 @@ mips64_fill_fpregset (const struct regcache *regcache,
     }
   else if (regno == mips_regnum (gdbarch)->fp_control_status)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
+      LONGEST val = regcache->raw_collect_signed (regno);
       to = (gdb_byte *) (*fpregsetp + 32);
       store_signed_integer (to, 4, byte_order, val);
     }
   else if (regno == mips_regnum (gdbarch)->fp_implementation_revision)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
+      LONGEST val = regcache->raw_collect_signed (regno);
       to = (gdb_byte *) (*fpregsetp + 32) + 4;
       store_signed_integer (to, 4, byte_order, val);
     }
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 4dcfccbac70f0f962bf5e5596d035fda42322795..76f5bdfdf6cefd6d963be950aefcc275de7a820b 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -294,10 +294,14 @@ public:

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

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

   void raw_supply_zeroed (int regnum);

+  void raw_supply_signed (int regnum, LONGEST val);
+
   void raw_copy (int regnum, struct regcache *src_regcache);

   enum register_status get_register_status (int regnum) const;
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 957b265c28d929376c3b7d8c100ea355d8292b94..c9fbc997a3fa33fb576db7f57da4501043ff4e9d 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1228,6 +1228,26 @@ regcache::raw_supply_zeroed (int regnum)
   m_register_status[regnum] = REG_VALID;
 }

+/* Supply register REGNUM, whose contents are stored in signed VAL, to
+   REGCACHE.  */
+
+void
+regcache::raw_supply_signed (int regnum, LONGEST val)
+{
+  enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
+  gdb_byte *regbuf;
+  size_t size;
+
+  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
+  gdb_assert (!m_readonly_p);
+
+  regbuf = register_buffer (regnum);
+  size = m_descr->sizeof_register[regnum];
+
+  store_signed_integer (regbuf, size, byte_order, val);
+  m_register_status[regnum] = REG_VALID;
+}
+
 /* Collect register REGNUM from REGCACHE and store its contents in BUF.  */

 void
@@ -1251,6 +1271,23 @@ regcache::raw_collect (int regnum, void *buf) const
   memcpy (buf, regbuf, size);
 }

+/* Collect register REGNUM from REGCACHE and extract its contents into a signed
+   LONGEST.  */
+
+LONGEST
+regcache::raw_collect_signed (int regnum) const
+{
+  enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
+  const gdb_byte *regbuf;
+  size_t size;
+
+  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
+
+  regbuf = register_buffer (regnum);
+  size = m_descr->sizeof_register[regnum];
+  return extract_signed_integer (regbuf, size, byte_order);
+}
+
 void
 regcache::raw_copy (int regnum, struct regcache *src_regcache)
 {

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[John Baldwin]

On Friday, May 05, 2017 08:04:04 AM Alan Hayward wrote:
> 
> > On 11 Apr 2017, at 16:37, Yao Qi <qiyaoltc@gmail.com> wrote:
> > 
> > Alan Hayward <Alan.Hayward@arm.com> writes:
> > 
> > Hi Alan,
> > There are different ways of getting rid of MAX_REGISTER_SIZE, let us try
> > some simple approaches first.  Some uses of MAX_REGISTER_SIZE still
> > can't be removed, but let us start from easy part.
> > 
> 
> Patch split into multiple parts.
> 
> This part adds two new regcache functions: raw_supply_signed and 
> raw_collect_signed.
> 
> Tested on a --enable-targets=all build using make check with board files
> unix and native-gdbserver.
> I do not have a MIPS machine to test on.
> 
> Ok to commit?

mips-fbsd-tdep.c changes look ok to me.

-- 
John Baldwin

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (1/4)

[Yao Qi]

Alan Hayward <Alan.Hayward@arm.com> writes:

> Ok to commit?

Yes, it is OK.

-- 
Yao (齐尧)

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (3/4)

[Yao Qi]

Alan Hayward <Alan.Hayward@arm.com> writes:

> Ok to commit?

Yes, it is OK.

-- 
Yao (齐尧)

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Yao Qi]

Alan Hayward <Alan.Hayward@arm.com> writes:

>  }
>
> +/* Supply register REGNUM, whose contents are stored in signed VAL, to
> +   REGCACHE.  */
> +
> +void
> +regcache::raw_supply_signed (int regnum, LONGEST val)

The unsigned version of this function is also needed, because I see such
pattern also exists,

      store_unsigned_integer (buf, 8, byte_order, sp + offset);
      regcache_raw_supply (regcache, AMD64_RSP_REGNUM, buf);

this leads me thinking we need to use function template to define
functions for both LONGEST and ULONGEST.

Secondly, this method can be named as raw_supply.

> +{
> +  enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
> +  gdb_byte *regbuf;
> +  size_t size;
> +
> +  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
> +  gdb_assert (!m_readonly_p);
> +
> +  regbuf = register_buffer (regnum);
> +  size = m_descr->sizeof_register[regnum];
> +
> +  store_signed_integer (regbuf, size, byte_order, val);
> +  m_register_status[regnum] = REG_VALID;
> +}
> +
>  /* Collect register REGNUM from REGCACHE and store its contents in BUF.  */
>
>  void
> @@ -1251,6 +1271,23 @@ regcache::raw_collect (int regnum, void *buf) const
>    memcpy (buf, regbuf, size);
>  }
>
> +/* Collect register REGNUM from REGCACHE and extract its contents into a signed

This line is too long.

> +   LONGEST.  */
> +
> +LONGEST
> +regcache::raw_collect_signed (int regnum) const

We can define this method like this,

template<typename T>
using LongType = typename std::enable_if<(std::is_same<T, LONGEST>::value
					  || std::is_same<T, ULONGEST>::value),
					 T>::type;

  template<typename T>
  LongType<T> raw_collect (int regnum) const
  {
    ....
    if (std::is_signed<T>::value)
      return extract_signed_integer (regbuf, size, byte_order);
    else
      return extract_unsigned_integer (regbuf, size, byte_order);
  }

> +{
> +  enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
> +  const gdb_byte *regbuf;
> +  size_t size;
> +
> +  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
> +
> +  regbuf = register_buffer (regnum);
> +  size = m_descr->sizeof_register[regnum];
> +  return extract_signed_integer (regbuf, size, byte_order);
> +}

If you want, we can add a function template for extract_signed_integer
and extract_unsigned_integer.

template<typename T>
LongType<T>
extract_integer (const gdb_byte *addr, int len, enum bfd_endian byte_order)
{
  T retval;

  ...
  if (std::is_signed<T>::value)
    {
      ...
    }
  else
    {
      ...
    }
  return retval;
}

so, the raw_collect above becomes, 

  template<typename T>
  LongType<T> raw_collect (int regnum) const
  {
    ....
    return extract_integer<T> (regbuf, size, byte_order);
  }

and raw_read_{signed,unsigned}, cooked_read_{signed,unsigned} can be
merged as function template too.

-- 
Yao (齐尧)

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Alan Hayward]

> On 12 May 2017, at 09:53, Yao Qi <qiyaoltc@gmail.com> wrote:
> 
> Alan Hayward <Alan.Hayward@arm.com> writes:
> 
>> }
>> 
>> +/* Supply register REGNUM, whose contents are stored in signed VAL, to
>> +   REGCACHE.  */
>> +
>> +void
>> +regcache::raw_supply_signed (int regnum, LONGEST val)
> 
> The unsigned version of this function is also needed, because I see such
> pattern also exists,
> 
>      store_unsigned_integer (buf, 8, byte_order, sp + offset);
>      regcache_raw_supply (regcache, AMD64_RSP_REGNUM, buf);
> 
> this leads me thinking we need to use function template to define
> functions for both LONGEST and ULONGEST.
> 
> Secondly, this method can be named as raw_supply.
> 
>> +{
>> +  enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
>> +  gdb_byte *regbuf;
>> +  size_t size;
>> +
>> +  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
>> +  gdb_assert (!m_readonly_p);
>> +
>> +  regbuf = register_buffer (regnum);
>> +  size = m_descr->sizeof_register[regnum];
>> +
>> +  store_signed_integer (regbuf, size, byte_order, val);
>> +  m_register_status[regnum] = REG_VALID;
>> +}
>> +
>> /* Collect register REGNUM from REGCACHE and store its contents in BUF.  */
>> 
>> void
>> @@ -1251,6 +1271,23 @@ regcache::raw_collect (int regnum, void *buf) const
>>   memcpy (buf, regbuf, size);
>> }
>> 
>> +/* Collect register REGNUM from REGCACHE and extract its contents into a signed
> 
> This line is too long.
> 
>> +   LONGEST.  */
>> +
>> +LONGEST
>> +regcache::raw_collect_signed (int regnum) const
> 
> We can define this method like this,
> 
> template<typename T>
> using LongType = typename std::enable_if<(std::is_same<T, LONGEST>::value
> 					  || std::is_same<T, ULONGEST>::value),
> 					 T>::type;
> 
>  template<typename T>
>  LongType<T> raw_collect (int regnum) const
>  {
>    ....
>    if (std::is_signed<T>::value)
>      return extract_signed_integer (regbuf, size, byte_order);
>    else
>      return extract_unsigned_integer (regbuf, size, byte_order);
>  }
> 
>> +{
>> +  enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
>> +  const gdb_byte *regbuf;
>> +  size_t size;
>> +
>> +  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
>> +
>> +  regbuf = register_buffer (regnum);
>> +  size = m_descr->sizeof_register[regnum];
>> +  return extract_signed_integer (regbuf, size, byte_order);
>> +}
> 
> If you want, we can add a function template for extract_signed_integer
> and extract_unsigned_integer.
> 
> template<typename T>
> LongType<T>
> extract_integer (const gdb_byte *addr, int len, enum bfd_endian byte_order)
> {
>  T retval;
> 
>  ...
>  if (std::is_signed<T>::value)
>    {
>      ...
>    }
>  else
>    {
>      ...
>    }
>  return retval;
> }
> 
> so, the raw_collect above becomes, 
> 
>  template<typename T>
>  LongType<T> raw_collect (int regnum) const
>  {
>    ....
>    return extract_integer<T> (regbuf, size, byte_order);
>  }
> 
> and raw_read_{signed,unsigned}, cooked_read_{signed,unsigned} can be
> merged as function template too.
> 


Added templates for extract_integer and store_integer, raw_supply and raw_collect.
Did not add raw_read and cooked_read, as they are out of the scope of this patch.


Tested on a --enable-targets=all build using make check with board files
unix and native-gdbserver.
I do not have a MIPS machine to test on.
Ok to commit?

Alan.

2017-05-16  Alan Hayward  <alan.hayward@arm.com>

	* gdb/defs.h (LongType): New templated type.
	(extract_integer): New declaration.
	(extract_signed_integer): Switched to inline.
	(extract_unsigned_integer): Likewise.
	(store_integer): New declaration
	(store_signed_integer): Switched to inline.
	(store_unsigned_integer): Likewise.
	* gdb/findvar.c (extract_integer): New function and instantiations.
	(extract_signed_integer): Removed function.
	(extract_unsigned_integer): Likewise.
	(store_integer): New function and instantiations.
	(store_signed_integer): Removed function.
	(store_unsigned_integer): Likewise.
	* mips-fbsd-tdep.c (mips_fbsd_supply_reg): Use templated raw_supply.
	(mips_fbsd_collect_reg): Use templated raw_collect.
	* mips-linux-tdep.c (supply_32bit_reg): Use templated raw_supply.
	(mips64_fill_gregset): Use templated raw_collect.
	(mips64_fill_fpregset): Use templated raw_supply.
	* gdb/regcache.c (regcache::raw_supply): New function and
	instantiations.
	(LongType regcache::raw_collect): Likewise
	* gdb/regcache.h (regcache::raw_supply): New declaration.
	(regcache::raw_collect): Likewise


diff --git a/gdb/defs.h b/gdb/defs.h
index a0b586f401eca205334e9f237081f4da97c83aa1..45c02f89d9c7a6326a82a6eaaf7d226175e24513 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -627,6 +627,11 @@ enum symbol_needs_kind
   SYMBOL_NEEDS_FRAME
 };

+template<typename T>
+using LongType = typename std::enable_if<(std::is_same<T, LONGEST>::value
+					  || std::is_same<T, ULONGEST>::value),
+					 T>::type;
+
 /* Dynamic target-system-dependent parameters for GDB.  */
 #include "gdbarch.h"

@@ -637,11 +642,21 @@ enum { MAX_REGISTER_SIZE = 64 };

 /* In findvar.c.  */

-extern LONGEST extract_signed_integer (const gdb_byte *, int,
-				       enum bfd_endian);
+template<typename T> LongType<T> extract_integer (const gdb_byte *addr,
+						  int len,
+						  enum bfd_endian byte_order);
+
+inline LONGEST extract_signed_integer (const gdb_byte *addr, int len,
+				       enum bfd_endian byte_order)
+{
+  return extract_integer<LONGEST> (addr, len, byte_order);
+}

-extern ULONGEST extract_unsigned_integer (const gdb_byte *, int,
-					  enum bfd_endian);
+inline ULONGEST extract_unsigned_integer (const gdb_byte *addr, int len,
+					  enum bfd_endian byte_order)
+{
+  return extract_integer<LONGEST> (addr, len, byte_order);
+}

 extern int extract_long_unsigned_integer (const gdb_byte *, int,
 					  enum bfd_endian, LONGEST *);
@@ -649,11 +664,21 @@ extern int extract_long_unsigned_integer (const gdb_byte *, int,
 extern CORE_ADDR extract_typed_address (const gdb_byte *buf,
 					struct type *type);

-extern void store_signed_integer (gdb_byte *, int,
-				  enum bfd_endian, LONGEST);
+template<typename T> void store_integer (gdb_byte *addr, int len,
+					 enum bfd_endian byte_order,
+					 LongType<T> val);

-extern void store_unsigned_integer (gdb_byte *, int,
-				    enum bfd_endian, ULONGEST);
+inline void store_signed_integer (gdb_byte *addr, int len,
+				  enum bfd_endian byte_order, LONGEST val)
+{
+  store_integer<LONGEST> (addr, len, byte_order, val);
+}
+
+inline void store_unsigned_integer (gdb_byte *addr, int len,
+				    enum bfd_endian byte_order, ULONGEST val)
+{
+  store_integer<ULONGEST> (addr, len, byte_order, val);
+}

 extern void store_typed_address (gdb_byte *buf, struct type *type,
 				 CORE_ADDR addr);
diff --git a/gdb/findvar.c b/gdb/findvar.c
index ed4d5c1266c9de069981b306bc8229ae5fb02350..3bcc98ce3421cb800fc8222535b16ca94ee043da 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -46,70 +46,54 @@
 you lose
 #endif

-LONGEST
-extract_signed_integer (const gdb_byte *addr, int len,
-			enum bfd_endian byte_order)
+template<typename T>
+LongType<T>
+extract_integer (const gdb_byte *addr, int len, enum bfd_endian byte_order)
 {
-  LONGEST retval;
+  T retval;
   const unsigned char *p;
   const unsigned char *startaddr = addr;
   const unsigned char *endaddr = startaddr + len;

-  if (len > (int) sizeof (LONGEST))
+  if (len > (int) sizeof (T))
     error (_("\
 That operation is not available on integers of more than %d bytes."),
-	   (int) sizeof (LONGEST));
+	   (int) sizeof (T));

   /* Start at the most significant end of the integer, and work towards
      the least significant.  */
   if (byte_order == BFD_ENDIAN_BIG)
     {
       p = startaddr;
-      /* Do the sign extension once at the start.  */
-      retval = ((LONGEST) * p ^ 0x80) - 0x80;
-      for (++p; p < endaddr; ++p)
+      if (std::is_signed<T>::value)
+	{
+	  /* Do the sign extension once at the start.  */
+	  retval = ((LONGEST) * p ^ 0x80) - 0x80;
+	  ++p;
+	}
+      for (; p < endaddr; ++p)
 	retval = (retval << 8) | *p;
     }
   else
     {
       p = endaddr - 1;
-      /* Do the sign extension once at the start.  */
-      retval = ((LONGEST) * p ^ 0x80) - 0x80;
-      for (--p; p >= startaddr; --p)
+      if (std::is_signed<T>::value)
+	{
+	  /* Do the sign extension once at the start.  */
+	  retval = ((LONGEST) * p ^ 0x80) - 0x80;
+	  --p;
+	}
+      for (; p >= startaddr; --p)
 	retval = (retval << 8) | *p;
     }
   return retval;
 }

-ULONGEST
-extract_unsigned_integer (const gdb_byte *addr, int len,
-			  enum bfd_endian byte_order)
-{
-  ULONGEST retval;
-  const unsigned char *p;
-  const unsigned char *startaddr = addr;
-  const unsigned char *endaddr = startaddr + len;
+template LongType<ULONGEST> extract_integer<ULONGEST>
+  (const gdb_byte *addr, int len, enum bfd_endian byte_order);

-  if (len > (int) sizeof (ULONGEST))
-    error (_("\
-That operation is not available on integers of more than %d bytes."),
-	   (int) sizeof (ULONGEST));
-
-  /* Start at the most significant end of the integer, and work towards
-     the least significant.  */
-  retval = 0;
-  if (byte_order == BFD_ENDIAN_BIG)
-    {
-      for (p = startaddr; p < endaddr; ++p)
-	retval = (retval << 8) | *p;
-    }
-  else
-    {
-      for (p = endaddr - 1; p >= startaddr; --p)
-	retval = (retval << 8) | *p;
-    }
-  return retval;
-}
+template LongType<LONGEST> extract_integer<LONGEST>
+  (const gdb_byte *addr, int len, enum bfd_endian byte_order);

 /* Sometimes a long long unsigned integer can be extracted as a
    LONGEST value.  This is done so that we can print these values
@@ -180,9 +164,10 @@ extract_typed_address (const gdb_byte *buf, struct type *type)
 /* All 'store' functions accept a host-format integer and store a
    target-format integer at ADDR which is LEN bytes long.  */

+template<typename T>
 void
-store_signed_integer (gdb_byte *addr, int len,
-		      enum bfd_endian byte_order, LONGEST val)
+store_integer (gdb_byte *addr, int len, enum bfd_endian byte_order,
+	       LongType<T> val)
 {
   gdb_byte *p;
   gdb_byte *startaddr = addr;
@@ -208,33 +193,12 @@ store_signed_integer (gdb_byte *addr, int len,
     }
 }

-void
-store_unsigned_integer (gdb_byte *addr, int len,
-			enum bfd_endian byte_order, ULONGEST val)
-{
-  unsigned char *p;
-  unsigned char *startaddr = (unsigned char *) addr;
-  unsigned char *endaddr = startaddr + len;
-
-  /* Start at the least significant end of the integer, and work towards
-     the most significant.  */
-  if (byte_order == BFD_ENDIAN_BIG)
-    {
-      for (p = endaddr - 1; p >= startaddr; --p)
-	{
-	  *p = val & 0xff;
-	  val >>= 8;
-	}
-    }
-  else
-    {
-      for (p = startaddr; p < endaddr; ++p)
-	{
-	  *p = val & 0xff;
-	  val >>= 8;
-	}
-    }
-}
+template void store_integer<ULONGEST> (gdb_byte *addr, int len,
+				       enum bfd_endian byte_order,
+				       LongType<ULONGEST> val);
+template void store_integer<LONGEST> (gdb_byte *addr, int len,
+				      enum bfd_endian byte_order,
+				      LongType<LONGEST> val);

 /* Store the address ADDR as a pointer of type TYPE at BUF, in target
    form.  */
diff --git a/gdb/mips-fbsd-tdep.c b/gdb/mips-fbsd-tdep.c
index 00fae0ec60ddc9e645d3236efe29f2f9e9ceab5c..1e391bfd16864d9d45f3b6e0de425aa5543515df 100644
--- a/gdb/mips-fbsd-tdep.c
+++ b/gdb/mips-fbsd-tdep.c
@@ -48,9 +48,9 @@
 #define MIPS_FBSD_NUM_FPREGS	34

 /* Supply a single register.  If the source register size matches the
-   size the regcache expects, this can use regcache_raw_supply().  If
+   size the regcache expects, this can use regcache->raw_supply ().  If
    they are different, this copies the source register into a buffer
-   that can be passed to regcache_raw_supply().  */
+   that can be passed to regcache->raw_supply ().  */

 static void
 mips_fbsd_supply_reg (struct regcache *regcache, int regnum, const void *addr,
@@ -63,20 +63,17 @@ mips_fbsd_supply_reg (struct regcache *regcache, int regnum, const void *addr,
   else
     {
       enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      gdb_byte buf[MAX_REGISTER_SIZE];
       LONGEST val;

-      val = extract_signed_integer ((const gdb_byte *) addr, len, byte_order);
-      store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
-			    val);
-      regcache_raw_supply (regcache, regnum, buf);
+      val = extract_integer<LONGEST> ((const gdb_byte *) addr, len, byte_order);
+      regcache->raw_supply<LONGEST> (regnum, val);
     }
 }

 /* Collect a single register.  If the destination register size
    matches the size the regcache expects, this can use
-   regcache_raw_supply().  If they are different, this fetches the
-   register via regcache_raw_supply() into a buffer and then copies it
+   regcache->raw_supply ().  If they are different, this fetches the
+   register via regcache->raw_supply () into a buffer and then copies it
    into the final destination.  */

 static void
@@ -90,13 +87,8 @@ mips_fbsd_collect_reg (const struct regcache *regcache, int regnum, void *addr,
   else
     {
       enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regnum, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regnum),
-				    byte_order);
-      store_signed_integer ((gdb_byte *) addr, len, byte_order, val);
+      LONGEST val = regcache->raw_collect<LONGEST> (regnum);
+      store_integer<LONGEST> ((gdb_byte *) addr, len, byte_order, val);
     }
 }

diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
index 48a582a16c934abe6e8f87c46a6009649c606d49..bac106ed5e9e5d89285420beb52b33d6ce36265a 100644
--- a/gdb/mips-linux-tdep.c
+++ b/gdb/mips-linux-tdep.c
@@ -118,11 +118,10 @@ supply_32bit_reg (struct regcache *regcache, int regnum, const void *addr)
 {
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-  gdb_byte buf[MAX_REGISTER_SIZE];
-  store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
-			extract_signed_integer ((const gdb_byte *) addr, 4,
-						byte_order));
-  regcache_raw_supply (regcache, regnum, buf);
+  LONGEST val;
+
+  val = extract_integer<LONGEST> ((const gdb_byte *) addr, 4, byte_order);
+  regcache->raw_supply<LONGEST> (regnum, val);
 }

 /* Unpack an elf_gregset_t into GDB's register cache.  */
@@ -460,14 +459,9 @@ mips64_fill_gregset (const struct regcache *regcache,

   if (regaddr != -1)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
+      LONGEST val = regcache->raw_collect<LONGEST> (regno);
       dst = regp + regaddr;
-      store_signed_integer ((gdb_byte *) dst, 8, byte_order, val);
+      store_integer<LONGEST> ((gdb_byte *) dst, 8, byte_order, val);
     }
 }

@@ -564,25 +558,15 @@ mips64_fill_fpregset (const struct regcache *regcache,
     }
   else if (regno == mips_regnum (gdbarch)->fp_control_status)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
+      LONGEST val = regcache->raw_collect<LONGEST> (regno);
       to = (gdb_byte *) (*fpregsetp + 32);
-      store_signed_integer (to, 4, byte_order, val);
+      store_integer<LONGEST> (to, 4, byte_order, val);
     }
   else if (regno == mips_regnum (gdbarch)->fp_implementation_revision)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
+      LONGEST val = regcache->raw_collect<LONGEST> (regno);
       to = (gdb_byte *) (*fpregsetp + 32) + 4;
-      store_signed_integer (to, 4, byte_order, val);
+      store_integer<LONGEST> (to, 4, byte_order, val);
     }
   else if (regno == -1)
     {
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 4dcfccbac70f0f962bf5e5596d035fda42322795..1ec8b98aa79c58c413786351a715ee46a0f5d5c2 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -21,6 +21,7 @@
 #define REGCACHE_H

 #include "common-regcache.h"
+#include "defs.h"

 struct regcache;
 struct regset;
@@ -294,8 +295,12 @@ public:

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

+  template<typename T> LongType<T> raw_collect (int regnum) const;
+
   void raw_supply (int regnum, const void *buf);

+  template<typename T> void raw_supply (int regnum, LongType<T> val);
+
   void raw_supply_zeroed (int regnum);

   void raw_copy (int regnum, struct regcache *src_regcache);
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 957b265c28d929376c3b7d8c100ea355d8292b94..4facc6b55e2bd08972cce1081b27e669103b6717 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1208,6 +1208,31 @@ regcache::raw_supply (int regnum, const void *buf)
     }
 }

+/* Supply register REGNUM, whose contents are stored in VAL, to
+   REGCACHE.  */
+
+template<typename T>
+void
+regcache::raw_supply (int regnum, LongType<T> val)
+{
+  enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
+  gdb_byte *regbuf;
+  size_t size;
+
+  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
+  gdb_assert (!m_readonly_p);
+
+  regbuf = register_buffer (regnum);
+  size = m_descr->sizeof_register[regnum];
+
+  store_integer<T> (regbuf, size, byte_order, val);
+  m_register_status[regnum] = REG_VALID;
+}
+
+template void regcache::raw_supply<ULONGEST> (int regnum,
+					      LongType<ULONGEST> val);
+template void regcache::raw_supply<LONGEST> (int regnum, LongType<LONGEST> val);
+
 /* 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).  */
@@ -1251,6 +1276,30 @@ regcache::raw_collect (int regnum, void *buf) const
   memcpy (buf, regbuf, size);
 }

+/* Collect register REGNUM from REGCACHE and extract its contents into a
+   LONGEST.  */
+
+template<typename T>
+LongType<T> regcache::raw_collect (int regnum) const
+{
+  enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
+  const gdb_byte *regbuf;
+  size_t size;
+
+  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
+
+  regbuf = register_buffer (regnum);
+  size = m_descr->sizeof_register[regnum];
+
+  if (std::is_signed<T>::value)
+    return extract_signed_integer (regbuf, size, byte_order);
+  else
+    return extract_unsigned_integer (regbuf, size, byte_order);
+}
+
+template LongType<ULONGEST> regcache::raw_collect<ULONGEST> (int regnum) const;
+template LongType<LONGEST> regcache::raw_collect<LONGEST> (int regnum) const;
+
 void
 regcache::raw_copy (int regnum, struct regcache *src_regcache)
 {

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Yao Qi]

Alan Hayward <Alan.Hayward@arm.com> writes:

> -extern ULONGEST extract_unsigned_integer (const gdb_byte *, int,
> -					  enum bfd_endian);
> +inline ULONGEST extract_unsigned_integer (const gdb_byte *addr, int len,
> +					  enum bfd_endian byte_order)
> +{
> +  return extract_integer<LONGEST> (addr, len, byte_order);

s/LONGEST/ULONGEST/

> +}
>

[My C++ knowledge is still poor, so forgive me if I ask something stupid]

Is there any reason you do not put function template (extract_integer and
store_integer) in header?  Any benefit of doing this?

>  extern int extract_long_unsigned_integer (const gdb_byte *, int,
>  					  enum bfd_endian, LONGEST *);
> @@ -649,11 +664,21 @@ extern int extract_long_unsigned_integer (const gdb_byte *, int,
>  extern CORE_ADDR extract_typed_address (const gdb_byte *buf,
>  					struct type *type);
>
> -extern void store_signed_integer (gdb_byte *, int,
> -				  enum bfd_endian, LONGEST);
> +template<typename T> void store_integer (gdb_byte *addr, int len,
> +					 enum bfd_endian byte_order,
> +					 LongType<T> val);

We can define store_integer slightly differently, so that we can use
implicit instantiation,

template<typename T>
typename std::enable_if<(std::is_same<T, LONGEST>::value
			 || std::is_same<T, ULONGEST>::value),
			void>::type
store_integer (gdb_byte *addr, int len, enum bfd_endian byte_order, T val)
{} so that ...

>
> -extern void store_unsigned_integer (gdb_byte *, int,
> -				    enum bfd_endian, ULONGEST);
> +inline void store_signed_integer (gdb_byte *addr, int len,
> +				  enum bfd_endian byte_order, LONGEST val)
> +{
> +  store_integer<LONGEST> (addr, len, byte_order, val);

... the template argument can be deduced,

    store_integer (addr, len, byte_order, val);

should be OK.

> diff --git a/gdb/findvar.c b/gdb/findvar.c
> index ed4d5c1266c9de069981b306bc8229ae5fb02350..3bcc98ce3421cb800fc8222535b16ca94ee043da 100644
> --- a/gdb/findvar.c
> +++ b/gdb/findvar.c
> @@ -46,70 +46,54 @@
>  you lose
>  #endif
>
> -LONGEST
> -extract_signed_integer (const gdb_byte *addr, int len,
> -			enum bfd_endian byte_order)
> +template<typename T>
> +LongType<T>
> +extract_integer (const gdb_byte *addr, int len, enum bfd_endian byte_order)
>  {
> -  LONGEST retval;
> +  T retval;
>    const unsigned char *p;
>    const unsigned char *startaddr = addr;
>    const unsigned char *endaddr = startaddr + len;
>
> -  if (len > (int) sizeof (LONGEST))
> +  if (len > (int) sizeof (T))
>      error (_("\
>  That operation is not available on integers of more than %d bytes."),
> -	   (int) sizeof (LONGEST));
> +	   (int) sizeof (T));
>
>    /* Start at the most significant end of the integer, and work towards
>       the least significant.  */
>    if (byte_order == BFD_ENDIAN_BIG)
>      {
>        p = startaddr;
> -      /* Do the sign extension once at the start.  */
> -      retval = ((LONGEST) * p ^ 0x80) - 0x80;
> -      for (++p; p < endaddr; ++p)
> +      if (std::is_signed<T>::value)
> +	{
> +	  /* Do the sign extension once at the start.  */
> +	  retval = ((LONGEST) * p ^ 0x80) - 0x80;
> +	  ++p;
> +	}

Need to set retval to zero if (!std::is_signed<T>::value), otherwise,
retval might be not initialized.


> diff --git a/gdb/mips-fbsd-tdep.c b/gdb/mips-fbsd-tdep.c
> index 00fae0ec60ddc9e645d3236efe29f2f9e9ceab5c..1e391bfd16864d9d45f3b6e0de425aa5543515df 100644
> --- a/gdb/mips-fbsd-tdep.c
> +++ b/gdb/mips-fbsd-tdep.c
> @@ -48,9 +48,9 @@
>  #define MIPS_FBSD_NUM_FPREGS	34
>
>  /* Supply a single register.  If the source register size matches the
> -   size the regcache expects, this can use regcache_raw_supply().  If
> +   size the regcache expects, this can use regcache->raw_supply ().  If
>     they are different, this copies the source register into a buffer
> -   that can be passed to regcache_raw_supply().  */
> +   that can be passed to regcache->raw_supply ().  */
>
>  static void
>  mips_fbsd_supply_reg (struct regcache *regcache, int regnum, const void *addr,
> @@ -63,20 +63,17 @@ mips_fbsd_supply_reg (struct regcache *regcache, int regnum, const void *addr,
>    else
>      {
>        enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> -      gdb_byte buf[MAX_REGISTER_SIZE];
>        LONGEST val;
>
> -      val = extract_signed_integer ((const gdb_byte *) addr, len, byte_order);
> -      store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
> -			    val);
> -      regcache_raw_supply (regcache, regnum, buf);
> +      val = extract_integer<LONGEST> ((const gdb_byte *) addr, len, byte_order);
> +      regcache->raw_supply<LONGEST> (regnum, val);
>      }
>  }
>
>  /* Collect a single register.  If the destination register size
>     matches the size the regcache expects, this can use
> -   regcache_raw_supply().  If they are different, this fetches the
> -   register via regcache_raw_supply() into a buffer and then copies it
> +   regcache->raw_supply ().  If they are different, this fetches the
> +   register via regcache->raw_supply () into a buffer and then copies it
>     into the final destination.  */
>
>  static void
> @@ -90,13 +87,8 @@ mips_fbsd_collect_reg (const struct regcache *regcache, int regnum, void *addr,
>    else
>      {
>        enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> -      gdb_byte buf[MAX_REGISTER_SIZE];
> -      LONGEST val;
> -
> -      regcache_raw_collect (regcache, regnum, buf);
> -      val = extract_signed_integer (buf, register_size (gdbarch, regnum),
> -				    byte_order);
> -      store_signed_integer ((gdb_byte *) addr, len, byte_order, val);
> +      LONGEST val = regcache->raw_collect<LONGEST> (regnum);
> +      store_integer<LONGEST> ((gdb_byte *) addr, len, byte_order, val);
>      }
>  }
>
> diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
> index 48a582a16c934abe6e8f87c46a6009649c606d49..bac106ed5e9e5d89285420beb52b33d6ce36265a 100644
> --- a/gdb/mips-linux-tdep.c
> +++ b/gdb/mips-linux-tdep.c
> @@ -118,11 +118,10 @@ supply_32bit_reg (struct regcache *regcache, int regnum, const void *addr)
>  {
>    struct gdbarch *gdbarch = get_regcache_arch (regcache);
>    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> -  gdb_byte buf[MAX_REGISTER_SIZE];
> -  store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
> -			extract_signed_integer ((const gdb_byte *) addr, 4,
> -						byte_order));
> -  regcache_raw_supply (regcache, regnum, buf);
> +  LONGEST val;
> +
> +  val = extract_integer<LONGEST> ((const gdb_byte *) addr, 4, byte_order);
> +  regcache->raw_supply<LONGEST> (regnum, val);

If we declare raw_supply a little bit differently (see below), we can do

   regcache->raw_supply (regnum, val);

>  }
>
>  /* Unpack an elf_gregset_t into GDB's register cache.  */
> @@ -460,14 +459,9 @@ mips64_fill_gregset (const struct regcache *regcache,
>
>    if (regaddr != -1)
>      {
> -      gdb_byte buf[MAX_REGISTER_SIZE];
> -      LONGEST val;
> -
> -      regcache_raw_collect (regcache, regno, buf);
> -      val = extract_signed_integer (buf, register_size (gdbarch, regno),
> -				    byte_order);
> +      LONGEST val = regcache->raw_collect<LONGEST> (regno);
>        dst = regp + regaddr;
> -      store_signed_integer ((gdb_byte *) dst, 8, byte_order, val);
> +      store_integer<LONGEST> ((gdb_byte *) dst, 8, byte_order, val);

Likewise,

        store_integer ((gdb_byte *) dst, 8, byte_order, val);

>      }
>  }
>

> diff --git a/gdb/regcache.h b/gdb/regcache.h
> index 4dcfccbac70f0f962bf5e5596d035fda42322795..1ec8b98aa79c58c413786351a715ee46a0f5d5c2 100644
> --- a/gdb/regcache.h
> +++ b/gdb/regcache.h
> @@ -21,6 +21,7 @@
>  #define REGCACHE_H
>
>  #include "common-regcache.h"
> +#include "defs.h"
>
>  struct regcache;
>  struct regset;
> @@ -294,8 +295,12 @@ public:
>
>    void raw_collect (int regnum, void *buf) const;
>
> +  template<typename T> LongType<T> raw_collect (int regnum) const;
> +
>    void raw_supply (int regnum, const void *buf);
>
> +  template<typename T> void raw_supply (int regnum, LongType<T> val);
> +

Similarly, we can do

  template<typename T>
  typename std::enable_if<(std::is_same<T, LONGEST>::value
			   || std::is_same<T, ULONGEST>::value),
			  void>::type
  raw_supply (int regnum, T val);

so that we don't need to explicit instantiate it.

>    void raw_supply_zeroed (int regnum);
>
>    void raw_copy (int regnum, struct regcache *src_regcache);
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 957b265c28d929376c3b7d8c100ea355d8292b94..4facc6b55e2bd08972cce1081b27e669103b6717 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -1208,6 +1208,31 @@ regcache::raw_supply (int regnum, const void *buf)
>      }
>  }
>
> +/* Supply register REGNUM, whose contents are stored in VAL, to
> +   REGCACHE.  */
> +
> +template<typename T>
> +void
> +regcache::raw_supply (int regnum, LongType<T> val)
> +{
> +  enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
> +  gdb_byte *regbuf;
> +  size_t size;
> +
> +  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
> +  gdb_assert (!m_readonly_p);
> +
> +  regbuf = register_buffer (regnum);
> +  size = m_descr->sizeof_register[regnum];
> +
> +  store_integer<T> (regbuf, size, byte_order, val);
> +  m_register_status[regnum] = REG_VALID;
> +}

Again, why don't you put it in regcache.h?

-- 
Yao (齐尧)

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Alan Hayward]

> On 22 May 2017, at 13:07, Yao Qi <qiyaoltc@gmail.com> wrote:
> 
> Alan Hayward <Alan.Hayward@arm.com> writes:
> 
>> -extern ULONGEST extract_unsigned_integer (const gdb_byte *, int,
>> -					  enum bfd_endian);
>> +inline ULONGEST extract_unsigned_integer (const gdb_byte *addr, int len,
>> +					  enum bfd_endian byte_order)
>> +{
>> +  return extract_integer<LONGEST> (addr, len, byte_order);
> 
> s/LONGEST/ULONGEST/

Oops. Fixed.

> 
>> +}
>> 
> 
> [My C++ knowledge is still poor, so forgive me if I ask something stupid]
> 
> Is there any reason you do not put function template (extract_integer and
> store_integer) in header?  Any benefit of doing this?

It made the header file neater.
But, putting in the header means the instantiations are in the header.
I’ve now moved it into the header.


> 
>> extern int extract_long_unsigned_integer (const gdb_byte *, int,
>> 					  enum bfd_endian, LONGEST *);
>> @@ -649,11 +664,21 @@ extern int extract_long_unsigned_integer (const gdb_byte *, int,
>> extern CORE_ADDR extract_typed_address (const gdb_byte *buf,
>> 					struct type *type);
>> 
>> -extern void store_signed_integer (gdb_byte *, int,
>> -				  enum bfd_endian, LONGEST);
>> +template<typename T> void store_integer (gdb_byte *addr, int len,
>> +					 enum bfd_endian byte_order,
>> +					 LongType<T> val);
> 
> We can define store_integer slightly differently, so that we can use
> implicit instantiation,
> 
> template<typename T>
> typename std::enable_if<(std::is_same<T, LONGEST>::value
> 			 || std::is_same<T, ULONGEST>::value),
> 			void>::type
> store_integer (gdb_byte *addr, int len, enum bfd_endian byte_order, T val)
> {} so that …
> 

Done. It’s a shame we can’t do the same with extract_integer.


<SNIP> (Comments resolved as suggested)

> 
>> 
>> +/* Supply register REGNUM, whose contents are stored in VAL, to
>> +   REGCACHE.  */
>> +
>> +template<typename T>
>> +void
>> +regcache::raw_supply (int regnum, LongType<T> val)
>> +{
>> +  enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
>> +  gdb_byte *regbuf;
>> +  size_t size;
>> +
>> +  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
>> +  gdb_assert (!m_readonly_p);
>> +
>> +  regbuf = register_buffer (regnum);
>> +  size = m_descr->sizeof_register[regnum];
>> +
>> +  store_integer<T> (regbuf, size, byte_order, val);
>> +  m_register_status[regnum] = REG_VALID;
>> +}
> 
> Again, why don't you put it in regcache.h?
> 

This bit cannot go into regcache.h.
The function uses m_descr - and regcache_descr is defined in recache.c
I did not want to move regcache_descr into recache.h


Tested on a --enable-targets=all build using make check with board files
unix and native-gdbserver.
I do not have a MIPS machine to test on.
Ok to commit?

Alan.



2017-05-22  Alan Hayward  <alan.hayward@arm.com>

	* gdb/defs.h (LongType): New templated type.
	(extract_integer): New declaration.
	(extract_signed_integer): Switched to inline.
	(extract_unsigned_integer): Likewise.
	(store_integer): New declaration
	(store_signed_integer): Switched to inline.
	(store_unsigned_integer): Likewise.
	* gdb/findvar.c (extract_signed_integer): Removed function.
	(extract_unsigned_integer): Likewise.
	(store_signed_integer): Removed function.
	(store_unsigned_integer): Likewise.
	* mips-fbsd-tdep.c (mips_fbsd_supply_reg): Use templated raw_supply.
	(mips_fbsd_collect_reg): Use templated raw_collect.
	* mips-linux-tdep.c (supply_32bit_reg): Use templated raw_supply.
	(mips64_fill_gregset): Use templated raw_collect.
	(mips64_fill_fpregset): Use templated raw_supply.
	* gdb/regcache.c (regcache::raw_supply): New function and
	instantiations.
	(LongType regcache::raw_collect): Likewise
	* gdb/regcache.h (regcache::raw_supply): New declaration.
	(regcache::raw_collect): Likewise



diff --git a/gdb/defs.h b/gdb/defs.h
index a0b586f401eca205334e9f237081f4da97c83aa1..010fe55a4760ebc7a420115e7590199fcab899a0 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -627,6 +627,11 @@ enum symbol_needs_kind
   SYMBOL_NEEDS_FRAME
 };

+template<typename T>
+using LongType = typename std::enable_if<(std::is_same<T, LONGEST>::value
+					  || std::is_same<T, ULONGEST>::value),
+					 T>::type;
+
 /* Dynamic target-system-dependent parameters for GDB.  */
 #include "gdbarch.h"

@@ -637,11 +642,69 @@ enum { MAX_REGISTER_SIZE = 64 };

 /* In findvar.c.  */

-extern LONGEST extract_signed_integer (const gdb_byte *, int,
-				       enum bfd_endian);
+/* All 'extract' functions return a host-format integer from a target-format
+   integer at ADDR which is LEN bytes long.  */

-extern ULONGEST extract_unsigned_integer (const gdb_byte *, int,
-					  enum bfd_endian);
+template<typename T>
+LongType<T>
+extract_integer (const gdb_byte *addr, int len, enum bfd_endian byte_order)
+{
+  T retval = 0;
+  const unsigned char *p;
+  const unsigned char *startaddr = addr;
+  const unsigned char *endaddr = startaddr + len;
+
+  if (len > (int) sizeof (T))
+    error (_("\
+That operation is not available on integers of more than %d bytes."),
+	   (int) sizeof (T));
+
+  /* Start at the most significant end of the integer, and work towards
+     the least significant.  */
+  if (byte_order == BFD_ENDIAN_BIG)
+    {
+      p = startaddr;
+      if (std::is_signed<T>::value)
+	{
+	  /* Do the sign extension once at the start.  */
+	  retval = ((LONGEST) * p ^ 0x80) - 0x80;
+	  ++p;
+	}
+      for (; p < endaddr; ++p)
+	retval = (retval << 8) | *p;
+    }
+  else
+    {
+      p = endaddr - 1;
+      if (std::is_signed<T>::value)
+	{
+	  /* Do the sign extension once at the start.  */
+	  retval = ((LONGEST) * p ^ 0x80) - 0x80;
+	  --p;
+	}
+      for (; p >= startaddr; --p)
+	retval = (retval << 8) | *p;
+    }
+  return retval;
+}
+
+template LongType<ULONGEST> extract_integer<ULONGEST>
+  (const gdb_byte *addr, int len, enum bfd_endian byte_order);
+
+template LongType<LONGEST> extract_integer<LONGEST>
+  (const gdb_byte *addr, int len, enum bfd_endian byte_order);
+
+inline ULONGEST extract_unsigned_integer (const gdb_byte *addr, int len,
+					  enum bfd_endian byte_order)
+{
+  return extract_integer<ULONGEST> (addr, len, byte_order);
+}
+
+inline LONGEST extract_signed_integer (const gdb_byte *addr, int len,
+				       enum bfd_endian byte_order)
+{
+  return extract_integer<LONGEST> (addr, len, byte_order);
+}

 extern int extract_long_unsigned_integer (const gdb_byte *, int,
 					  enum bfd_endian, LONGEST *);
@@ -649,11 +712,58 @@ extern int extract_long_unsigned_integer (const gdb_byte *, int,
 extern CORE_ADDR extract_typed_address (const gdb_byte *buf,
 					struct type *type);

-extern void store_signed_integer (gdb_byte *, int,
-				  enum bfd_endian, LONGEST);
+/* All 'store' functions accept a host-format integer and store a
+   target-format integer at ADDR which is LEN bytes long.  */

-extern void store_unsigned_integer (gdb_byte *, int,
-				    enum bfd_endian, ULONGEST);
+template<typename T>
+typename std::enable_if<(std::is_same<T, LONGEST>::value
+			 || std::is_same<T, ULONGEST>::value),
+			void>::type
+store_integer (gdb_byte *addr, int len, enum bfd_endian byte_order,
+	       T val)
+{
+  gdb_byte *p;
+  gdb_byte *startaddr = addr;
+  gdb_byte *endaddr = startaddr + len;
+
+  /* Start at the least significant end of the integer, and work towards
+     the most significant.  */
+  if (byte_order == BFD_ENDIAN_BIG)
+    {
+      for (p = endaddr - 1; p >= startaddr; --p)
+	{
+	  *p = val & 0xff;
+	  val >>= 8;
+	}
+    }
+  else
+    {
+      for (p = startaddr; p < endaddr; ++p)
+	{
+	  *p = val & 0xff;
+	  val >>= 8;
+	}
+    }
+}
+
+template void store_integer (gdb_byte *addr, int len,
+				       enum bfd_endian byte_order,
+				       LongType<ULONGEST> val);
+template void store_integer (gdb_byte *addr, int len,
+				      enum bfd_endian byte_order,
+				      LongType<LONGEST> val);
+
+inline void store_signed_integer (gdb_byte *addr, int len,
+				  enum bfd_endian byte_order, LONGEST val)
+{
+  store_integer (addr, len, byte_order, val);
+}
+
+inline void store_unsigned_integer (gdb_byte *addr, int len,
+				    enum bfd_endian byte_order, ULONGEST val)
+{
+  store_integer (addr, len, byte_order, val);
+}

 extern void store_typed_address (gdb_byte *buf, struct type *type,
 				 CORE_ADDR addr);
diff --git a/gdb/findvar.c b/gdb/findvar.c
index ed4d5c1266c9de069981b306bc8229ae5fb02350..614d1291eac5a7438a1bf3f6dd0b5d012a0dfaa7 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -34,9 +34,7 @@
 #include "language.h"
 #include "dwarf2loc.h"

-/* Basic byte-swapping routines.  All 'extract' functions return a
-   host-format integer from a target-format integer at ADDR which is
-   LEN bytes long.  */
+/* Basic byte-swapping routines.  */

 #if TARGET_CHAR_BIT != 8 || HOST_CHAR_BIT != 8
   /* 8 bit characters are a pretty safe assumption these days, so we
@@ -46,70 +44,6 @@
 you lose
 #endif

-LONGEST
-extract_signed_integer (const gdb_byte *addr, int len,
-			enum bfd_endian byte_order)
-{
-  LONGEST retval;
-  const unsigned char *p;
-  const unsigned char *startaddr = addr;
-  const unsigned char *endaddr = startaddr + len;
-
-  if (len > (int) sizeof (LONGEST))
-    error (_("\
-That operation is not available on integers of more than %d bytes."),
-	   (int) sizeof (LONGEST));
-
-  /* Start at the most significant end of the integer, and work towards
-     the least significant.  */
-  if (byte_order == BFD_ENDIAN_BIG)
-    {
-      p = startaddr;
-      /* Do the sign extension once at the start.  */
-      retval = ((LONGEST) * p ^ 0x80) - 0x80;
-      for (++p; p < endaddr; ++p)
-	retval = (retval << 8) | *p;
-    }
-  else
-    {
-      p = endaddr - 1;
-      /* Do the sign extension once at the start.  */
-      retval = ((LONGEST) * p ^ 0x80) - 0x80;
-      for (--p; p >= startaddr; --p)
-	retval = (retval << 8) | *p;
-    }
-  return retval;
-}
-
-ULONGEST
-extract_unsigned_integer (const gdb_byte *addr, int len,
-			  enum bfd_endian byte_order)
-{
-  ULONGEST retval;
-  const unsigned char *p;
-  const unsigned char *startaddr = addr;
-  const unsigned char *endaddr = startaddr + len;
-
-  if (len > (int) sizeof (ULONGEST))
-    error (_("\
-That operation is not available on integers of more than %d bytes."),
-	   (int) sizeof (ULONGEST));
-
-  /* Start at the most significant end of the integer, and work towards
-     the least significant.  */
-  retval = 0;
-  if (byte_order == BFD_ENDIAN_BIG)
-    {
-      for (p = startaddr; p < endaddr; ++p)
-	retval = (retval << 8) | *p;
-    }
-  else
-    {
-      for (p = endaddr - 1; p >= startaddr; --p)
-	retval = (retval << 8) | *p;
-    }
-  return retval;
-}

 /* Sometimes a long long unsigned integer can be extracted as a
    LONGEST value.  This is done so that we can print these values
@@ -177,65 +111,6 @@ extract_typed_address (const gdb_byte *buf, struct type *type)
   return gdbarch_pointer_to_address (get_type_arch (type), type, buf);
 }

-/* All 'store' functions accept a host-format integer and store a
-   target-format integer at ADDR which is LEN bytes long.  */
-
-void
-store_signed_integer (gdb_byte *addr, int len,
-		      enum bfd_endian byte_order, LONGEST val)
-{
-  gdb_byte *p;
-  gdb_byte *startaddr = addr;
-  gdb_byte *endaddr = startaddr + len;
-
-  /* Start at the least significant end of the integer, and work towards
-     the most significant.  */
-  if (byte_order == BFD_ENDIAN_BIG)
-    {
-      for (p = endaddr - 1; p >= startaddr; --p)
-	{
-	  *p = val & 0xff;
-	  val >>= 8;
-	}
-    }
-  else
-    {
-      for (p = startaddr; p < endaddr; ++p)
-	{
-	  *p = val & 0xff;
-	  val >>= 8;
-	}
-    }
-}
-
-void
-store_unsigned_integer (gdb_byte *addr, int len,
-			enum bfd_endian byte_order, ULONGEST val)
-{
-  unsigned char *p;
-  unsigned char *startaddr = (unsigned char *) addr;
-  unsigned char *endaddr = startaddr + len;
-
-  /* Start at the least significant end of the integer, and work towards
-     the most significant.  */
-  if (byte_order == BFD_ENDIAN_BIG)
-    {
-      for (p = endaddr - 1; p >= startaddr; --p)
-	{
-	  *p = val & 0xff;
-	  val >>= 8;
-	}
-    }
-  else
-    {
-      for (p = startaddr; p < endaddr; ++p)
-	{
-	  *p = val & 0xff;
-	  val >>= 8;
-	}
-    }
-}
-
 /* Store the address ADDR as a pointer of type TYPE at BUF, in target
    form.  */
 void
diff --git a/gdb/mips-fbsd-tdep.c b/gdb/mips-fbsd-tdep.c
index 00fae0ec60ddc9e645d3236efe29f2f9e9ceab5c..9e2025e58c4cf6d887acb0019f39a1993777aeab 100644
--- a/gdb/mips-fbsd-tdep.c
+++ b/gdb/mips-fbsd-tdep.c
@@ -48,9 +48,9 @@
 #define MIPS_FBSD_NUM_FPREGS	34

 /* Supply a single register.  If the source register size matches the
-   size the regcache expects, this can use regcache_raw_supply().  If
+   size the regcache expects, this can use regcache->raw_supply ().  If
    they are different, this copies the source register into a buffer
-   that can be passed to regcache_raw_supply().  */
+   that can be passed to regcache->raw_supply ().  */

 static void
 mips_fbsd_supply_reg (struct regcache *regcache, int regnum, const void *addr,
@@ -63,20 +63,17 @@ mips_fbsd_supply_reg (struct regcache *regcache, int regnum, const void *addr,
   else
     {
       enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      gdb_byte buf[MAX_REGISTER_SIZE];
       LONGEST val;

-      val = extract_signed_integer ((const gdb_byte *) addr, len, byte_order);
-      store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
-			    val);
-      regcache_raw_supply (regcache, regnum, buf);
+      val = extract_integer<LONGEST> ((const gdb_byte *) addr, len, byte_order);
+      regcache->raw_supply (regnum, val);
     }
 }

 /* Collect a single register.  If the destination register size
    matches the size the regcache expects, this can use
-   regcache_raw_supply().  If they are different, this fetches the
-   register via regcache_raw_supply() into a buffer and then copies it
+   regcache->raw_supply ().  If they are different, this fetches the
+   register via regcache->raw_supply () into a buffer and then copies it
    into the final destination.  */

 static void
@@ -90,13 +87,8 @@ mips_fbsd_collect_reg (const struct regcache *regcache, int regnum, void *addr,
   else
     {
       enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regnum, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regnum),
-				    byte_order);
-      store_signed_integer ((gdb_byte *) addr, len, byte_order, val);
+      LONGEST val = regcache->raw_collect<LONGEST> (regnum);
+      store_integer ((gdb_byte *) addr, len, byte_order, val);
     }
 }

diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
index 48a582a16c934abe6e8f87c46a6009649c606d49..6dcb759cb85083a426466b8cd00560bebe6a35a3 100644
--- a/gdb/mips-linux-tdep.c
+++ b/gdb/mips-linux-tdep.c
@@ -118,11 +118,10 @@ supply_32bit_reg (struct regcache *regcache, int regnum, const void *addr)
 {
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-  gdb_byte buf[MAX_REGISTER_SIZE];
-  store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
-			extract_signed_integer ((const gdb_byte *) addr, 4,
-						byte_order));
-  regcache_raw_supply (regcache, regnum, buf);
+  LONGEST val;
+
+  val = extract_integer<LONGEST> ((const gdb_byte *) addr, 4, byte_order);
+  regcache->raw_supply (regnum, val);
 }

 /* Unpack an elf_gregset_t into GDB's register cache.  */
@@ -460,14 +459,9 @@ mips64_fill_gregset (const struct regcache *regcache,

   if (regaddr != -1)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
+      LONGEST val = regcache->raw_collect<LONGEST> (regno);
       dst = regp + regaddr;
-      store_signed_integer ((gdb_byte *) dst, 8, byte_order, val);
+      store_integer ((gdb_byte *) dst, 8, byte_order, val);
     }
 }

@@ -564,25 +558,15 @@ mips64_fill_fpregset (const struct regcache *regcache,
     }
   else if (regno == mips_regnum (gdbarch)->fp_control_status)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
+      LONGEST val = regcache->raw_collect<LONGEST> (regno);
       to = (gdb_byte *) (*fpregsetp + 32);
-      store_signed_integer (to, 4, byte_order, val);
+      store_integer (to, 4, byte_order, val);
     }
   else if (regno == mips_regnum (gdbarch)->fp_implementation_revision)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
+      LONGEST val = regcache->raw_collect<LONGEST> (regno);
       to = (gdb_byte *) (*fpregsetp + 32) + 4;
-      store_signed_integer (to, 4, byte_order, val);
+      store_integer (to, 4, byte_order, val);
     }
   else if (regno == -1)
     {
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 4dcfccbac70f0f962bf5e5596d035fda42322795..ee5e12312fe731b03b6b4e54802988d239aed488 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -21,6 +21,7 @@
 #define REGCACHE_H

 #include "common-regcache.h"
+#include "defs.h"

 struct regcache;
 struct regset;
@@ -294,8 +295,16 @@ public:

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

+  template<typename T> LongType<T> raw_collect (int regnum) const;
+
   void raw_supply (int regnum, const void *buf);

+  template<typename T>
+  typename std::enable_if<(std::is_same<T, LONGEST>::value
+			   || std::is_same<T, ULONGEST>::value),
+			  void>::type
+  raw_supply (int regnum, T val);
+
   void raw_supply_zeroed (int regnum);

   void raw_copy (int regnum, struct regcache *src_regcache);
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 660558f7ff10f9d8346b6e08422e16c38c3c4d7d..1412c16beb97841847690ecfcac64121298edfc9 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1189,6 +1189,32 @@ regcache::raw_supply (int regnum, const void *buf)
     }
 }

+/* Supply register REGNUM, whose contents are stored in VAL, to
+   REGCACHE.  */
+
+template<typename T>
+typename std::enable_if<(std::is_same<T, LONGEST>::value
+			 || std::is_same<T, ULONGEST>::value),
+			void>::type
+regcache::raw_supply (int regnum, T val)
+{
+  enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
+  gdb_byte *regbuf;
+  size_t size;
+
+  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
+  gdb_assert (!m_readonly_p);
+
+  regbuf = register_buffer (regnum);
+  size = m_descr->sizeof_register[regnum];
+
+  store_integer (regbuf, size, byte_order, val);
+  m_register_status[regnum] = REG_VALID;
+}
+
+template void regcache::raw_supply (int regnum, LONGEST val);
+template void regcache::raw_supply (int regnum, ULONGEST val);
+
 /* 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).  */
@@ -1232,6 +1258,30 @@ regcache::raw_collect (int regnum, void *buf) const
   memcpy (buf, regbuf, size);
 }

+/* Collect register REGNUM from REGCACHE and extract its contents into a
+   LONGEST.  */
+
+template<typename T>
+LongType<T> regcache::raw_collect (int regnum) const
+{
+  enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
+  const gdb_byte *regbuf;
+  size_t size;
+
+  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
+
+  regbuf = register_buffer (regnum);
+  size = m_descr->sizeof_register[regnum];
+
+  if (std::is_signed<T>::value)
+    return extract_signed_integer (regbuf, size, byte_order);
+  else
+    return extract_unsigned_integer (regbuf, size, byte_order);
+}
+
+template LongType<ULONGEST> regcache::raw_collect<ULONGEST> (int regnum) const;
+template LongType<LONGEST> regcache::raw_collect<LONGEST> (int regnum) const;
+
 void
 regcache::raw_copy (int regnum, struct regcache *src_regcache)
 {

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Pedro Alves]

On 05/22/2017 05:05 PM, Alan Hayward wrote:

>      {
>        enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> -      gdb_byte buf[MAX_REGISTER_SIZE];
>        LONGEST val;
> 
> -      val = extract_signed_integer ((const gdb_byte *) addr, len, byte_order);
> -      store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
> -			    val);
> -      regcache_raw_supply (regcache, regnum, buf);
> +      val = extract_integer<LONGEST> ((const gdb_byte *) addr, len, byte_order);
> +      regcache->raw_supply (regnum, val);
>      }
>  }
> 

>    else
>      {
>        enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> -      gdb_byte buf[MAX_REGISTER_SIZE];
> -      LONGEST val;
> -
> -      regcache_raw_collect (regcache, regnum, buf);
> -      val = extract_signed_integer (buf, register_size (gdbarch, regnum),
> -				    byte_order);
> -      store_signed_integer ((gdb_byte *) addr, len, byte_order, val);
> +      LONGEST val = regcache->raw_collect<LONGEST> (regnum);
> +      store_integer ((gdb_byte *) addr, len, byte_order, val);

I wonder whether we can get rid of the LONGEST / host integer
middleman and simplify things while at it.  For instance, what if we
had a version of raw_collect that took the destination buffer length
as parameter:

      regcache->raw_collect_integer (regnum, (gdb_byte *) addr, len);

that would copy bytes over into addr, and if the register is
narrower than LEN, then it'd insert the necessary
leading zeros (or 0xFFs if signed extension necessary),
and if the registers is wider than LEN, then it'd skip
copying enough significant bytes so that LEN fits.

Likewise for regcache->raw_supply.


> --- a/gdb/regcache.h
> +++ b/gdb/regcache.h
> @@ -21,6 +21,7 @@
>  #define REGCACHE_H
> 
>  #include "common-regcache.h"
> +#include "defs.h"

Headers should not include defs.h.  Is there some .c file that
misses including defs.h first thing?

Thanks,
Pedro Alves

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Alan Hayward]

> On 22 May 2017, at 18:15, Pedro Alves <palves@redhat.com> wrote:
> 
> On 05/22/2017 05:05 PM, Alan Hayward wrote:
> 
>>     {
>>       enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>> -      gdb_byte buf[MAX_REGISTER_SIZE];
>>       LONGEST val;
>> 
>> -      val = extract_signed_integer ((const gdb_byte *) addr, len, byte_order);
>> -      store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
>> -			    val);
>> -      regcache_raw_supply (regcache, regnum, buf);
>> +      val = extract_integer<LONGEST> ((const gdb_byte *) addr, len, byte_order);
>> +      regcache->raw_supply (regnum, val);
>>     }
>> }
>> 
> 
>>   else
>>     {
>>       enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>> -      gdb_byte buf[MAX_REGISTER_SIZE];
>> -      LONGEST val;
>> -
>> -      regcache_raw_collect (regcache, regnum, buf);
>> -      val = extract_signed_integer (buf, register_size (gdbarch, regnum),
>> -				    byte_order);
>> -      store_signed_integer ((gdb_byte *) addr, len, byte_order, val);
>> +      LONGEST val = regcache->raw_collect<LONGEST> (regnum);
>> +      store_integer ((gdb_byte *) addr, len, byte_order, val);
> 
> I wonder whether we can get rid of the LONGEST / host integer
> middleman and simplify things while at it.  For instance, what if we
> had a version of raw_collect that took the destination buffer length
> as parameter:
> 
>      regcache->raw_collect_integer (regnum, (gdb_byte *) addr, len);
> 
> that would copy bytes over into addr, and if the register is
> narrower than LEN, then it'd insert the necessary
> leading zeros (or 0xFFs if signed extension necessary),
> and if the registers is wider than LEN, then it'd skip
> copying enough significant bytes so that LEN fits.
> 
> Likewise for regcache->raw_supply.

Is it the case that gdb always does a store_integer after a raw_collect
of a (U)LONGEST?
And always an extract_integer before a raw_supply of a (U)LONGEST ?
(Both of these are tricky to grep for, because the code sequence is over
multiple lines)

I was going to mock up a new raw_collect_integer, but then got carried
away and wrote the full patch changes.
This version makes the MIPS files look neater.


> 
> 
>> --- a/gdb/regcache.h
>> +++ b/gdb/regcache.h
>> @@ -21,6 +21,7 @@
>> #define REGCACHE_H
>> 
>> #include "common-regcache.h"
>> +#include "defs.h"
> 
> Headers should not include defs.h.  Is there some .c file that
> misses including defs.h first thing?
> 

I think I needed it in an earlier version of my patch.
Removed.


New version with raw_collect_integer and raw_supply_integer.

Tested on a --enable-targets=all build using make check with board files
unix and native-gdbserver.
I do not have a MIPS machine to test on.
Ok to commit?

Alan.


2017-05-23  Alan Hayward  <alan.hayward@arm.com>

	* gdb/defs.h (LongType): New templated type.
	(extract_integer): New declaration.
	(extract_signed_integer): Switched to inline.
	(extract_unsigned_integer): Likewise.
	(store_integer): New declaration
	(store_signed_integer): Switched to inline.
	(store_unsigned_integer): Likewise.
	* gdb/findvar.c (extract_signed_integer): Removed function.
	(extract_unsigned_integer): Likewise.
	(store_signed_integer): Removed function.
	(store_unsigned_integer): Likewise.
	* mips-fbsd-tdep.c (mips_fbsd_supply_reg): Use raw_supply_integer.
	(mips_fbsd_collect_reg): Use templated raw_collect_integer.
	* mips-linux-tdep.c (supply_32bit_reg): Use raw_supply_integer.
	(mips64_fill_gregset): Use raw_collect_integer.
	(mips64_fill_fpregset): Use raw_supply_integer.
	* gdb/regcache.c (regcache::raw_supply_integer): New function and
	instantiations.
	(regcache::raw_collect_integer): Likewise
	* gdb/regcache.h (regcache::raw_supply): New declaration.
	(regcache::raw_collect): Likewise


diff --git a/gdb/defs.h b/gdb/defs.h
index a0b586f401eca205334e9f237081f4da97c83aa1..010fe55a4760ebc7a420115e7590199fcab899a0 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -627,6 +627,11 @@ enum symbol_needs_kind
   SYMBOL_NEEDS_FRAME
 };

+template<typename T>
+using LongType = typename std::enable_if<(std::is_same<T, LONGEST>::value
+					  || std::is_same<T, ULONGEST>::value),
+					 T>::type;
+
 /* Dynamic target-system-dependent parameters for GDB.  */
 #include "gdbarch.h"

@@ -637,11 +642,69 @@ enum { MAX_REGISTER_SIZE = 64 };

 /* In findvar.c.  */

-extern LONGEST extract_signed_integer (const gdb_byte *, int,
-				       enum bfd_endian);
+/* All 'extract' functions return a host-format integer from a target-format
+   integer at ADDR which is LEN bytes long.  */

-extern ULONGEST extract_unsigned_integer (const gdb_byte *, int,
-					  enum bfd_endian);
+template<typename T>
+LongType<T>
+extract_integer (const gdb_byte *addr, int len, enum bfd_endian byte_order)
+{
+  T retval = 0;
+  const unsigned char *p;
+  const unsigned char *startaddr = addr;
+  const unsigned char *endaddr = startaddr + len;
+
+  if (len > (int) sizeof (T))
+    error (_("\
+That operation is not available on integers of more than %d bytes."),
+	   (int) sizeof (T));
+
+  /* Start at the most significant end of the integer, and work towards
+     the least significant.  */
+  if (byte_order == BFD_ENDIAN_BIG)
+    {
+      p = startaddr;
+      if (std::is_signed<T>::value)
+	{
+	  /* Do the sign extension once at the start.  */
+	  retval = ((LONGEST) * p ^ 0x80) - 0x80;
+	  ++p;
+	}
+      for (; p < endaddr; ++p)
+	retval = (retval << 8) | *p;
+    }
+  else
+    {
+      p = endaddr - 1;
+      if (std::is_signed<T>::value)
+	{
+	  /* Do the sign extension once at the start.  */
+	  retval = ((LONGEST) * p ^ 0x80) - 0x80;
+	  --p;
+	}
+      for (; p >= startaddr; --p)
+	retval = (retval << 8) | *p;
+    }
+  return retval;
+}
+
+template LongType<ULONGEST> extract_integer<ULONGEST>
+  (const gdb_byte *addr, int len, enum bfd_endian byte_order);
+
+template LongType<LONGEST> extract_integer<LONGEST>
+  (const gdb_byte *addr, int len, enum bfd_endian byte_order);
+
+inline ULONGEST extract_unsigned_integer (const gdb_byte *addr, int len,
+					  enum bfd_endian byte_order)
+{
+  return extract_integer<ULONGEST> (addr, len, byte_order);
+}
+
+inline LONGEST extract_signed_integer (const gdb_byte *addr, int len,
+				       enum bfd_endian byte_order)
+{
+  return extract_integer<LONGEST> (addr, len, byte_order);
+}

 extern int extract_long_unsigned_integer (const gdb_byte *, int,
 					  enum bfd_endian, LONGEST *);
@@ -649,11 +712,58 @@ extern int extract_long_unsigned_integer (const gdb_byte *, int,
 extern CORE_ADDR extract_typed_address (const gdb_byte *buf,
 					struct type *type);

-extern void store_signed_integer (gdb_byte *, int,
-				  enum bfd_endian, LONGEST);
+/* All 'store' functions accept a host-format integer and store a
+   target-format integer at ADDR which is LEN bytes long.  */

-extern void store_unsigned_integer (gdb_byte *, int,
-				    enum bfd_endian, ULONGEST);
+template<typename T>
+typename std::enable_if<(std::is_same<T, LONGEST>::value
+			 || std::is_same<T, ULONGEST>::value),
+			void>::type
+store_integer (gdb_byte *addr, int len, enum bfd_endian byte_order,
+	       T val)
+{
+  gdb_byte *p;
+  gdb_byte *startaddr = addr;
+  gdb_byte *endaddr = startaddr + len;
+
+  /* Start at the least significant end of the integer, and work towards
+     the most significant.  */
+  if (byte_order == BFD_ENDIAN_BIG)
+    {
+      for (p = endaddr - 1; p >= startaddr; --p)
+	{
+	  *p = val & 0xff;
+	  val >>= 8;
+	}
+    }
+  else
+    {
+      for (p = startaddr; p < endaddr; ++p)
+	{
+	  *p = val & 0xff;
+	  val >>= 8;
+	}
+    }
+}
+
+template void store_integer (gdb_byte *addr, int len,
+				       enum bfd_endian byte_order,
+				       LongType<ULONGEST> val);
+template void store_integer (gdb_byte *addr, int len,
+				      enum bfd_endian byte_order,
+				      LongType<LONGEST> val);
+
+inline void store_signed_integer (gdb_byte *addr, int len,
+				  enum bfd_endian byte_order, LONGEST val)
+{
+  store_integer (addr, len, byte_order, val);
+}
+
+inline void store_unsigned_integer (gdb_byte *addr, int len,
+				    enum bfd_endian byte_order, ULONGEST val)
+{
+  store_integer (addr, len, byte_order, val);
+}

 extern void store_typed_address (gdb_byte *buf, struct type *type,
 				 CORE_ADDR addr);
diff --git a/gdb/findvar.c b/gdb/findvar.c
index ed4d5c1266c9de069981b306bc8229ae5fb02350..614d1291eac5a7438a1bf3f6dd0b5d012a0dfaa7 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -34,9 +34,7 @@
 #include "language.h"
 #include "dwarf2loc.h"

-/* Basic byte-swapping routines.  All 'extract' functions return a
-   host-format integer from a target-format integer at ADDR which is
-   LEN bytes long.  */
+/* Basic byte-swapping routines.  */

 #if TARGET_CHAR_BIT != 8 || HOST_CHAR_BIT != 8
   /* 8 bit characters are a pretty safe assumption these days, so we
@@ -46,70 +44,6 @@
 you lose
 #endif

-LONGEST
-extract_signed_integer (const gdb_byte *addr, int len,
-			enum bfd_endian byte_order)
-{
-  LONGEST retval;
-  const unsigned char *p;
-  const unsigned char *startaddr = addr;
-  const unsigned char *endaddr = startaddr + len;
-
-  if (len > (int) sizeof (LONGEST))
-    error (_("\
-That operation is not available on integers of more than %d bytes."),
-	   (int) sizeof (LONGEST));
-
-  /* Start at the most significant end of the integer, and work towards
-     the least significant.  */
-  if (byte_order == BFD_ENDIAN_BIG)
-    {
-      p = startaddr;
-      /* Do the sign extension once at the start.  */
-      retval = ((LONGEST) * p ^ 0x80) - 0x80;
-      for (++p; p < endaddr; ++p)
-	retval = (retval << 8) | *p;
-    }
-  else
-    {
-      p = endaddr - 1;
-      /* Do the sign extension once at the start.  */
-      retval = ((LONGEST) * p ^ 0x80) - 0x80;
-      for (--p; p >= startaddr; --p)
-	retval = (retval << 8) | *p;
-    }
-  return retval;
-}
-
-ULONGEST
-extract_unsigned_integer (const gdb_byte *addr, int len,
-			  enum bfd_endian byte_order)
-{
-  ULONGEST retval;
-  const unsigned char *p;
-  const unsigned char *startaddr = addr;
-  const unsigned char *endaddr = startaddr + len;
-
-  if (len > (int) sizeof (ULONGEST))
-    error (_("\
-That operation is not available on integers of more than %d bytes."),
-	   (int) sizeof (ULONGEST));
-
-  /* Start at the most significant end of the integer, and work towards
-     the least significant.  */
-  retval = 0;
-  if (byte_order == BFD_ENDIAN_BIG)
-    {
-      for (p = startaddr; p < endaddr; ++p)
-	retval = (retval << 8) | *p;
-    }
-  else
-    {
-      for (p = endaddr - 1; p >= startaddr; --p)
-	retval = (retval << 8) | *p;
-    }
-  return retval;
-}

 /* Sometimes a long long unsigned integer can be extracted as a
    LONGEST value.  This is done so that we can print these values
@@ -177,65 +111,6 @@ extract_typed_address (const gdb_byte *buf, struct type *type)
   return gdbarch_pointer_to_address (get_type_arch (type), type, buf);
 }

-/* All 'store' functions accept a host-format integer and store a
-   target-format integer at ADDR which is LEN bytes long.  */
-
-void
-store_signed_integer (gdb_byte *addr, int len,
-		      enum bfd_endian byte_order, LONGEST val)
-{
-  gdb_byte *p;
-  gdb_byte *startaddr = addr;
-  gdb_byte *endaddr = startaddr + len;
-
-  /* Start at the least significant end of the integer, and work towards
-     the most significant.  */
-  if (byte_order == BFD_ENDIAN_BIG)
-    {
-      for (p = endaddr - 1; p >= startaddr; --p)
-	{
-	  *p = val & 0xff;
-	  val >>= 8;
-	}
-    }
-  else
-    {
-      for (p = startaddr; p < endaddr; ++p)
-	{
-	  *p = val & 0xff;
-	  val >>= 8;
-	}
-    }
-}
-
-void
-store_unsigned_integer (gdb_byte *addr, int len,
-			enum bfd_endian byte_order, ULONGEST val)
-{
-  unsigned char *p;
-  unsigned char *startaddr = (unsigned char *) addr;
-  unsigned char *endaddr = startaddr + len;
-
-  /* Start at the least significant end of the integer, and work towards
-     the most significant.  */
-  if (byte_order == BFD_ENDIAN_BIG)
-    {
-      for (p = endaddr - 1; p >= startaddr; --p)
-	{
-	  *p = val & 0xff;
-	  val >>= 8;
-	}
-    }
-  else
-    {
-      for (p = startaddr; p < endaddr; ++p)
-	{
-	  *p = val & 0xff;
-	  val >>= 8;
-	}
-    }
-}
-
 /* Store the address ADDR as a pointer of type TYPE at BUF, in target
    form.  */
 void
diff --git a/gdb/mips-fbsd-tdep.c b/gdb/mips-fbsd-tdep.c
index 00fae0ec60ddc9e645d3236efe29f2f9e9ceab5c..7278fb71e3ad1f18d35cd696832d99bf90198620 100644
--- a/gdb/mips-fbsd-tdep.c
+++ b/gdb/mips-fbsd-tdep.c
@@ -48,9 +48,8 @@
 #define MIPS_FBSD_NUM_FPREGS	34

 /* Supply a single register.  If the source register size matches the
-   size the regcache expects, this can use regcache_raw_supply().  If
-   they are different, this copies the source register into a buffer
-   that can be passed to regcache_raw_supply().  */
+   size the regcache expects, this can use regcache->raw_supply ().  If
+   they are different, this can use regcache->raw_supply_integer ().  */

 static void
 mips_fbsd_supply_reg (struct regcache *regcache, int regnum, const void *addr,
@@ -59,25 +58,16 @@ mips_fbsd_supply_reg (struct regcache *regcache, int regnum, const void *addr,
   struct gdbarch *gdbarch = get_regcache_arch (regcache);

   if (register_size (gdbarch, regnum) == len)
-    regcache_raw_supply (regcache, regnum, addr);
+    regcache->raw_supply (regnum, addr);
   else
-    {
-      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      val = extract_signed_integer ((const gdb_byte *) addr, len, byte_order);
-      store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
-			    val);
-      regcache_raw_supply (regcache, regnum, buf);
-    }
+    regcache->raw_supply_integer<LONGEST> (regnum, (const gdb_byte *) addr,
+					   len);
 }

 /* Collect a single register.  If the destination register size
    matches the size the regcache expects, this can use
-   regcache_raw_supply().  If they are different, this fetches the
-   register via regcache_raw_supply() into a buffer and then copies it
-   into the final destination.  */
+   regcache->raw_collect ().  If they are different, this can use
+   regcache->raw_collect_integer ().  */

 static void
 mips_fbsd_collect_reg (const struct regcache *regcache, int regnum, void *addr,
@@ -86,18 +76,9 @@ mips_fbsd_collect_reg (const struct regcache *regcache, int regnum, void *addr,
   struct gdbarch *gdbarch = get_regcache_arch (regcache);

   if (register_size (gdbarch, regnum) == len)
-    regcache_raw_collect (regcache, regnum, addr);
+    regcache->raw_collect (regnum, addr);
   else
-    {
-      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regnum, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regnum),
-				    byte_order);
-      store_signed_integer ((gdb_byte *) addr, len, byte_order, val);
-    }
+    regcache->raw_collect_integer<LONGEST> (regnum, (gdb_byte *) addr, len);
 }

 /* Supply the floating-point registers stored in FPREGS to REGCACHE.
diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
index 48a582a16c934abe6e8f87c46a6009649c606d49..b30fe294a0bbbb815418e65149a88f59eb15befb 100644
--- a/gdb/mips-linux-tdep.c
+++ b/gdb/mips-linux-tdep.c
@@ -116,13 +116,7 @@ mips_linux_get_longjmp_target (struct frame_info *frame, CORE_ADDR *pc)
 static void
 supply_32bit_reg (struct regcache *regcache, int regnum, const void *addr)
 {
-  struct gdbarch *gdbarch = get_regcache_arch (regcache);
-  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-  gdb_byte buf[MAX_REGISTER_SIZE];
-  store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
-			extract_signed_integer ((const gdb_byte *) addr, 4,
-						byte_order));
-  regcache_raw_supply (regcache, regnum, buf);
+  regcache->raw_supply_integer<LONGEST> (regnum, (const gdb_byte *) addr, 4);
 }

 /* Unpack an elf_gregset_t into GDB's register cache.  */
@@ -417,7 +411,6 @@ mips64_fill_gregset (const struct regcache *regcache,
 		     mips64_elf_gregset_t *gregsetp, int regno)
 {
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
-  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   int regaddr, regi;
   mips64_elf_greg_t *regp = *gregsetp;
   void *dst;
@@ -460,14 +453,8 @@ mips64_fill_gregset (const struct regcache *regcache,

   if (regaddr != -1)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
       dst = regp + regaddr;
-      store_signed_integer ((gdb_byte *) dst, 8, byte_order, val);
+      regcache->raw_collect_integer<LONGEST> (regno, (gdb_byte *) dst, 8);
     }
 }

@@ -564,25 +551,13 @@ mips64_fill_fpregset (const struct regcache *regcache,
     }
   else if (regno == mips_regnum (gdbarch)->fp_control_status)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
       to = (gdb_byte *) (*fpregsetp + 32);
-      store_signed_integer (to, 4, byte_order, val);
+      regcache->raw_collect_integer<LONGEST> (regno, to, 4);
     }
   else if (regno == mips_regnum (gdbarch)->fp_implementation_revision)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
       to = (gdb_byte *) (*fpregsetp + 32) + 4;
-      store_signed_integer (to, 4, byte_order, val);
+      regcache->raw_collect_integer<LONGEST> (regno, to, 4);
     }
   else if (regno == -1)
     {
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 4dcfccbac70f0f962bf5e5596d035fda42322795..c47401948270b1c75d9d3b02cbbc3256db85f84e 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -294,8 +294,20 @@ public:

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

+  template<typename T>
+  typename std::enable_if<(std::is_same<T, LONGEST>::value
+			 || std::is_same<T, ULONGEST>::value),
+			void>::type
+  raw_collect_integer (int regnum, gdb_byte *addr, int addr_len) const;
+
   void raw_supply (int regnum, const void *buf);

+  template<typename T>
+  typename std::enable_if<(std::is_same<T, LONGEST>::value
+			   || std::is_same<T, ULONGEST>::value),
+			   void>::type
+  raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len);
+
   void raw_supply_zeroed (int regnum);

   void raw_copy (int regnum, struct regcache *src_regcache);
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 660558f7ff10f9d8346b6e08422e16c38c3c4d7d..cc9ab74b087cbda2d79264674e9b463e1528363f 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1189,6 +1189,39 @@ regcache::raw_supply (int regnum, const void *buf)
     }
 }

+/* Supply register REGNUM, whose contents are stored in ADDR, with length
+   ADDR_LEN to a (U)LONGEST, then store to REGCACHE, taking BYTE_ORDER into
+   account.  */
+
+template<typename T>
+typename std::enable_if<(std::is_same<T, LONGEST>::value
+			 || std::is_same<T, ULONGEST>::value),
+			void>::type
+regcache::raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len)
+{
+  enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
+  gdb_byte *regbuf;
+  size_t regsize;
+  T val;
+
+  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
+  gdb_assert (!m_readonly_p);
+
+  regbuf = register_buffer (regnum);
+  regsize = m_descr->sizeof_register[regnum];
+
+  val = extract_integer<T> (addr, addr_len, byte_order);
+  store_integer (regbuf, regsize, byte_order, val);
+  m_register_status[regnum] = REG_VALID;
+}
+
+template void regcache::raw_supply_integer<ULONGEST> (int regnum,
+						      const gdb_byte *addr,
+						      int addr_len);
+template void regcache::raw_supply_integer<LONGEST> (int regnum,
+						     const gdb_byte *addr,
+						     int addr_len);
+
 /* 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).  */
@@ -1232,6 +1265,37 @@ regcache::raw_collect (int regnum, void *buf) const
   memcpy (buf, regbuf, size);
 }

+/* Collect register REGNUM from REGCACHE into a (U)LONGEST, then store ADDR_LEN
+   bytes into buffer ADDR, taking BYTE_ORDER into account.  */
+
+template<typename T>
+typename std::enable_if<(std::is_same<T, LONGEST>::value
+			 || std::is_same<T, ULONGEST>::value),
+			void>::type
+regcache::raw_collect_integer (int regnum, gdb_byte *addr, int addr_len) const
+{
+  enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
+  const gdb_byte *regbuf;
+  size_t regsize;
+  T val;
+
+  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
+
+  regbuf = register_buffer (regnum);
+  regsize = m_descr->sizeof_register[regnum];
+
+  val = extract_integer<T> (regbuf, regsize, byte_order);
+  store_integer (addr, addr_len, byte_order, val);
+}
+
+template void regcache::raw_collect_integer<ULONGEST> (int regnum,
+						       gdb_byte *addr,
+						       int addr_len) const;
+template void regcache::raw_collect_integer<LONGEST> (int regnum,
+						      gdb_byte *addr,
+						      int addr_len) const;
+
+
 void
 regcache::raw_copy (int regnum, struct regcache *src_regcache)
 {

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Pedro Alves]

On 05/23/2017 06:49 PM, Alan Hayward wrote:
> 
>> On 22 May 2017, at 18:15, Pedro Alves <palves@redhat.com> wrote:
>> I wonder whether we can get rid of the LONGEST / host integer
>> middleman and simplify things while at it.  For instance, what if we
>> had a version of raw_collect that took the destination buffer length
>> as parameter:
>>
>>      regcache->raw_collect_integer (regnum, (gdb_byte *) addr, len);
>>
>> that would copy bytes over into addr, and if the register is
>> narrower than LEN, then it'd insert the necessary
>> leading zeros (or 0xFFs if signed extension necessary),
>> and if the registers is wider than LEN, then it'd skip
>> copying enough significant bytes so that LEN fits.
>>
>> Likewise for regcache->raw_supply.
> 
> Is it the case that gdb always does a store_integer after a raw_collect
> of a (U)LONGEST?
> And always an extract_integer before a raw_supply of a (U)LONGEST ?
> (Both of these are tricky to grep for, because the code sequence is over
> multiple lines)

The observation here is that we're transferring integer data between
two places that seemingly both use target formatting:

 target integer 1 -> host integer -> target integer 2

when the target integer 1 and 2 have the same sizes, then we
copy data directly without the host integer middle man.
But when they don't have the same size, we do the host integer
conversion steps.  I was questioning having that step in the
first place.  If the target integers have different sizes,
we'll either end up with zero/sign extension, or truncation.
It seems to me offhand that doing those directly in the destination
buffer shouldn't be difficult?

> 
> I was going to mock up a new raw_collect_integer, but then got carried
> away and wrote the full patch changes.
> This version makes the MIPS files look neater.

Hmm, I think you may have misunderstood.  The main point was to
avoid having to have T/LONGEST temporary at all here:

> +template<typename T>
> +typename std::enable_if<(std::is_same<T, LONGEST>::value
> +			 || std::is_same<T, ULONGEST>::value),
> +			void>::type
> +regcache::raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len)
> +{
> +  enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
> +  gdb_byte *regbuf;
> +  size_t regsize;
> +  T val;
> +
> +  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
> +  gdb_assert (!m_readonly_p);
> +
> +  regbuf = register_buffer (regnum);
> +  regsize = m_descr->sizeof_register[regnum];
> +
> +  val = extract_integer<T> (addr, addr_len, byte_order);
> +  store_integer (regbuf, regsize, byte_order, val);
> +  m_register_status[regnum] = REG_VALID;

and maybe the need for all the templating.

I.e., in the cases at hand, both the regcache buffer an
the ADDR/LEN buffer are in target format, with the mismatch
being in integer width (i.e., may need zero/sign extension or
truncation), so it seems to me that we should be able to copy
data to/from the register buffer directly, without having to
convert to host format (the T / LONGEST) as an intermediate step.

So basically if we start with that you have, what we'd need
is a version of store_integer that takes a ADDR/LEN pair
instead of a T val.

Thanks,
Pedro Alves

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Alan Hayward]

> On 23 May 2017, at 19:30, Pedro Alves <palves@redhat.com> wrote:
> 
> On 05/23/2017 06:49 PM, Alan Hayward wrote:
>> 
>>> On 22 May 2017, at 18:15, Pedro Alves <palves@redhat.com> wrote:
>>> I wonder whether we can get rid of the LONGEST / host integer
>>> middleman and simplify things while at it.  For instance, what if we
>>> had a version of raw_collect that took the destination buffer length
>>> as parameter:
>>> 
>>>     regcache->raw_collect_integer (regnum, (gdb_byte *) addr, len);
>>> 
>>> that would copy bytes over into addr, and if the register is
>>> narrower than LEN, then it'd insert the necessary
>>> leading zeros (or 0xFFs if signed extension necessary),
>>> and if the registers is wider than LEN, then it'd skip
>>> copying enough significant bytes so that LEN fits.
>>> 
>>> Likewise for regcache->raw_supply.
>> 
>> Is it the case that gdb always does a store_integer after a raw_collect
>> of a (U)LONGEST?
>> And always an extract_integer before a raw_supply of a (U)LONGEST ?
>> (Both of these are tricky to grep for, because the code sequence is over
>> multiple lines)
> 
> The observation here is that we're transferring integer data between
> two places that seemingly both use target formatting:
> 
> target integer 1 -> host integer -> target integer 2
> 
> when the target integer 1 and 2 have the same sizes, then we
> copy data directly without the host integer middle man.
> But when they don't have the same size, we do the host integer
> conversion steps.  I was questioning having that step in the
> first place.  If the target integers have different sizes,
> we'll either end up with zero/sign extension, or truncation.
> It seems to me offhand that doing those directly in the destination
> buffer shouldn't be difficult?
> 
>> 
>> I was going to mock up a new raw_collect_integer, but then got carried
>> away and wrote the full patch changes.
>> This version makes the MIPS files look neater.
> 
> Hmm, I think you may have misunderstood.  The main point was to
> avoid having to have T/LONGEST temporary at all here:
> 
>> +template<typename T>
>> +typename std::enable_if<(std::is_same<T, LONGEST>::value
>> +			 || std::is_same<T, ULONGEST>::value),
>> +			void>::type
>> +regcache::raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len)
>> +{
>> +  enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
>> +  gdb_byte *regbuf;
>> +  size_t regsize;
>> +  T val;
>> +
>> +  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
>> +  gdb_assert (!m_readonly_p);
>> +
>> +  regbuf = register_buffer (regnum);
>> +  regsize = m_descr->sizeof_register[regnum];
>> +
>> +  val = extract_integer<T> (addr, addr_len, byte_order);
>> +  store_integer (regbuf, regsize, byte_order, val);
>> +  m_register_status[regnum] = REG_VALID;
> 
> and maybe the need for all the templating.
> 

Would still need to have eater signed/unsigned versions of the functions,
or maybe have “bool signed” parameter.


> I.e., in the cases at hand, both the regcache buffer an
> the ADDR/LEN buffer are in target format, with the mismatch
> being in integer width (i.e., may need zero/sign extension or
> truncation), so it seems to me that we should be able to copy
> data to/from the register buffer directly, without having to
> convert to host format (the T / LONGEST) as an intermediate step.
> 
> So basically if we start with that you have, what we'd need
> is a version of store_integer that takes a ADDR/LEN pair
> instead of a T val.
> 

Are you suggesting something like this:
(Warning - this snippet may not even compile, and I’m not sure on the endian
logic)


void
regcache::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);
  void *regbuf;
  size_t regsize;

  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
  gdb_assert (!m_readonly_p);
  gdb_assert (addr != 0);
  gdb_assert (addr_len <= regsize);

  regbuf = register_buffer (regnum);
  regsize = m_descr->sizeof_register[regnum];

  copy_and_fill_to_size (regbuf, addr, addr_len, regsize, is_signed, byte_order);
  m_register_status[regnum] = REG_VALID;
}

/* Copy COPY_LEN bytes from SOURCE to DEST, then sign extend or zero extend
   to FILL_LEN bytes.  */
void copy_and_fill_to_size (const gdb_byte *dest, const gdb_byte *source,
			    int copy_len, int fill_len, bool is_signed,
			    enum bfd_endian byte_order)
{
  signed int len_diff = fill_len - copy_len;
  gdb_assert (len_diff >= 0);

  if (byte_order == BFD_ENDIAN_BIG)
    memcpy (dest+len_diff, source, copy_len);
  else
    memcpy (dest, source, copy_len);

  if (len_diff > 0)
    {
      if (signed)
      	// TODO: sign extend from copy_len to fill_len
      else
	{
	  if (byte_order == BFD_ENDIAN_BIG)
      	    memset (dest, 0, len_diff);
      	  else
      	    memset (dest+copy_len, 0, len_diff);
	}
    }
}


Meanwhile raw_collect_integer doesn’t need a signed parameter:

void
regcache::raw_collect_integer (int regnum, gdb_byte *addr, int addr_len) const
{
  enum bfd_endian byte_order = gdbarch_byte_order (m_descr->gdbarch);
  const gdb_byte *regbuf;
  size_t regsize;

  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
  gdb_assert (addr_len <= regsize);

  regbuf = register_buffer (regnum);
  regsize = m_descr->sizeof_register[regnum];

  if (byte_order == BFD_ENDIAN_BIG)
    regbuf += regsize - addr_len;

  memcpy (addr, regbuf, addr_len);
}



> Thanks,
> Pedro Alves
> 

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Pedro Alves]

On 05/24/2017 10:07 AM, Alan Hayward wrote:

>> Hmm, I think you may have misunderstood.  The main point was to
>> avoid having to have T/LONGEST temporary at all here:

...

>>
>> and maybe the need for all the templating.
>>
> 
> Would still need to have eater signed/unsigned versions of the functions,
> or maybe have “bool signed” parameter.

Parameter sounds fine to me.

> Are you suggesting something like this:
> (Warning - this snippet may not even compile, and I’m not sure on the endian
> logic)

Yes, exactly.  WDYT?

> Meanwhile raw_collect_integer doesn’t need a signed parameter:

Wouldn't we need to support ADDR_LEN larger than the register size?

Thanks,
Pedro Alves

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Pedro Alves]

On 05/24/2017 10:07 AM, Alan Hayward wrote:
> /* Copy COPY_LEN bytes from SOURCE to DEST, then sign extend or zero extend
>    to FILL_LEN bytes.  */
> void copy_and_fill_to_size (const gdb_byte *dest, const gdb_byte *source,
> 			    int copy_len, int fill_len, bool is_signed,
> 			    enum bfd_endian byte_order)
> {
>   signed int len_diff = fill_len - copy_len;
>   gdb_assert (len_diff >= 0);
> 
>   if (byte_order == BFD_ENDIAN_BIG)
>     memcpy (dest+len_diff, source, copy_len);
>   else
>     memcpy (dest, source, copy_len);

Note here I was thinking you'd need to handle truncation as well.
I.e., only copy fill_len bytes when fill_len is narrower than
copy_len.  So I'd probably rename copy_len/fill_len to dest_len/source_len
to match 'dest' and 'source', and name the function something
else that doesn't have "fill" in it.

Thanks,
Pedro Alves

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Alan Hayward]

> On 24 May 2017, at 10:20, Pedro Alves <palves@redhat.com> wrote:
> 
> On 05/24/2017 10:07 AM, Alan Hayward wrote:
> 
>>> Hmm, I think you may have misunderstood.  The main point was to
>>> avoid having to have T/LONGEST temporary at all here:
> 
> ...
> 
>>> 
>>> and maybe the need for all the templating.
>>> 
>> 
>> Would still need to have eater signed/unsigned versions of the functions,
>> or maybe have “bool signed” parameter.
> 
> Parameter sounds fine to me.
> 
>> Are you suggesting something like this:
>> (Warning - this snippet may not even compile, and I’m not sure on the endian
>> logic)
> 
> Yes, exactly.  WDYT?

I’m happy in principle with it (after fix ups)

> 
>> Meanwhile raw_collect_integer doesn’t need a signed parameter:
> 
> Wouldn't we need to support ADDR_LEN larger than the register size?

This might be me misunderstanding gdb,
But I assumed that addr would always be shorter than the register size.
If addr is bigger than the register size then the most significant bits will
be chopped off (including the sign), which I think would be a bad idea?


> 
> On 24 May 2017, at 10:29, Pedro Alves <palves@redhat.com> wrote:
> 
> On 05/24/2017 10:07 AM, Alan Hayward wrote:
>> /* Copy COPY_LEN bytes from SOURCE to DEST, then sign extend or zero extend
>>   to FILL_LEN bytes.  */
>> void copy_and_fill_to_size (const gdb_byte *dest, const gdb_byte *source,
>> 			    int copy_len, int fill_len, bool is_signed,
>> 			    enum bfd_endian byte_order)
>> {
>>  signed int len_diff = fill_len - copy_len;
>>  gdb_assert (len_diff >= 0);
>> 
>>  if (byte_order == BFD_ENDIAN_BIG)
>>    memcpy (dest+len_diff, source, copy_len);
>>  else
>>    memcpy (dest, source, copy_len);
> 
> Note here I was thinking you'd need to handle truncation as well.
> I.e., only copy fill_len bytes when fill_len is narrower than
> copy_len.  So I'd probably rename copy_len/fill_len to dest_len/source_len
> to match 'dest' and 'source', and name the function something
> else that doesn't have "fill" in it.

Agreed (regardless of the outcome for the previous question).


Alan.

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Pedro Alves]

On 05/24/2017 11:20 AM, Alan Hayward wrote:

>>> Meanwhile raw_collect_integer doesn’t need a signed parameter:
>>
>> Wouldn't we need to support ADDR_LEN larger than the register size?
> 
> This might be me misunderstanding gdb,
> But I assumed that addr would always be shorter than the register size.
> If addr is bigger than the register size then the most significant bits will
> be chopped off (including the sign), which I think would be a bad idea?

Yeah, the case of a 32-bit register being given a 64-bit slot in a ptrace
register buffer is actually not unheard of.  For example the
segment registers on x86-64 (cs, ss, ds, etc.) are 32-bit in
gdb's register cache, but Linux ptrace transfers them as 64-bit
[see /usr/include/sys/reg.h].  I'm not exactly sure whether
in such cases we end up needing to sign/zero extend when copying
back, or whether the kernel ignores the upper bits.  I think that
on x86 we just copy the lower 4 bytes and leave the upper ones as
they were, so probably the latter.  The MIPS architecture is special
around addresses being signed though, and given the existing code,
I'd play it safe and keep the collect/store functions mirrors - if
one truncates, the other fills/extends, and vice versa.  There's also

  /* Is the target using 64-bit raw integer registers but only
     storing a left-aligned 32-bit value in each?  */
  int mips64_transfers_32bit_regs_p;

which most probably doesn't apply in this case (FreeBSD, while
I think that was originally added for remote), but it compounds in
the weirdness.

Thanks,
Pedro Alves

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Alan Hayward]

> On 24 May 2017, at 12:07, Pedro Alves <palves@redhat.com> wrote:
> 
> On 05/24/2017 11:20 AM, Alan Hayward wrote:
> 
>>>> Meanwhile raw_collect_integer doesn’t need a signed parameter:
>>> 
>>> Wouldn't we need to support ADDR_LEN larger than the register size?
>> 
>> This might be me misunderstanding gdb,
>> But I assumed that addr would always be shorter than the register size.
>> If addr is bigger than the register size then the most significant bits will
>> be chopped off (including the sign), which I think would be a bad idea?
> 
> Yeah, the case of a 32-bit register being given a 64-bit slot in a ptrace
> register buffer is actually not unheard of.  For example the
> segment registers on x86-64 (cs, ss, ds, etc.) are 32-bit in
> gdb's register cache, but Linux ptrace transfers them as 64-bit
> [see /usr/include/sys/reg.h].  I'm not exactly sure whether
> in such cases we end up needing to sign/zero extend when copying
> back, or whether the kernel ignores the upper bits.  I think that
> on x86 we just copy the lower 4 bytes and leave the upper ones as
> they were, so probably the latter.  The MIPS architecture is special
> around addresses being signed though, and given the existing code,
> I'd play it safe and keep the collect/store functions mirrors - if
> one truncates, the other fills/extends, and vice versa.  There's also
> 
>  /* Is the target using 64-bit raw integer registers but only
>     storing a left-aligned 32-bit value in each?  */
>  int mips64_transfers_32bit_regs_p;
> 
> which most probably doesn't apply in this case (FreeBSD, while
> I think that was originally added for remote), but it compounds in
> the weirdness.
> 
> Thanks,
> Pedro Alves
> 

Added copy_integer_to_size, and removed the templates.

Manually tested of copy_integer_to_size to make sure the signs and
endian parts all work.

Tested on a --enable-targets=all build using make check with board files
unix and native-gdbserver.
I do not have a MIPS machine to test on.
Ok to commit?

Alan.


2017-05-24  Alan Hayward  <alan.hayward@arm.com>

	* gdb/defs.h (copy_integer_to_size): New declaration.
	* gdb/findvar.c (extract_signed_integer): Removed function.
	(extract_unsigned_integer): Likewise.
	(store_signed_integer): Removed function.
	(store_unsigned_integer): Likewise.
	* mips-fbsd-tdep.c (mips_fbsd_supply_reg): Use raw_supply_integer.
	(mips_fbsd_collect_reg): Use templated raw_collect_integer.
	* mips-linux-tdep.c (supply_32bit_reg): Use raw_supply_integer.
	(mips64_fill_gregset): Use raw_collect_integer.
	(mips64_fill_fpregset): Use raw_supply_integer.
	* gdb/regcache.c (regcache::raw_supply_integer): New function.
	(regcache::raw_collect_integer): Likewise
	* gdb/regcache.h (regcache::raw_supply): New declaration.
	(regcache::raw_collect): Likewise


diff --git a/gdb/defs.h b/gdb/defs.h
index a0b586f401eca205334e9f237081f4da97c83aa1..a1a97bb1e791d4f423788797d1f04c3e89877d90 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -658,7 +658,10 @@ extern void store_unsigned_integer (gdb_byte *, int,
 extern void store_typed_address (gdb_byte *buf, struct type *type,
 				 CORE_ADDR addr);

-

+extern void copy_integer_to_size (gdb_byte *dest, int dest_size,
+				  const gdb_byte *source, int source_size,
+				  bool is_signed, enum bfd_endian byte_order);
+
 /* From valops.c */

 extern int watchdog;
diff --git a/gdb/findvar.c b/gdb/findvar.c
index ed4d5c1266c9de069981b306bc8229ae5fb02350..5a82e493f9ca6d9337a22defc4377235f36acba8 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -249,7 +249,47 @@ store_typed_address (gdb_byte *buf, struct type *type, CORE_ADDR addr)
   gdbarch_address_to_pointer (get_type_arch (type), type, buf, addr);
 }

+/* Copy a value from SOURCE of size SOURCE_SIZE bytes to DEST of size DEST_SIZE
+   bytes.  If SOURCE_SIZE is greater than DEST_SIZE, then truncate the most
+   significant bytes.  If SOURCE_SIZE is less than DEST_SIZE then either sign
+   or zero extended according to IS_SIGNED.  Values are stored in memory with
+   endianess BYTE_ORDER.  */

+void
+copy_integer_to_size (gdb_byte *dest, int dest_size, const gdb_byte *source,
+		      int source_size, bool is_signed,
+		      enum bfd_endian byte_order)
+{
+  signed int size_diff = dest_size - source_size;
+
+  /* Copy across everything from SOURCE that can fit into DEST.  */
+
+  if (byte_order == BFD_ENDIAN_BIG && size_diff > 0)
+    memcpy (dest + size_diff, source, source_size);
+  else if (byte_order == BFD_ENDIAN_BIG && size_diff < 0)
+    memcpy (dest, source - size_diff, dest_size);
+  else
+    memcpy (dest, source, std::min (source_size, dest_size));
+
+  /* Fill the remaining space in DEST by either zero extending or sign
+     extending.  */
+
+  if (size_diff > 0)
+    {
+      char extension = 0;
+      if (is_signed)
+	if ((byte_order == BFD_ENDIAN_BIG && source[0] & 0x80)
+	    || (byte_order != BFD_ENDIAN_BIG
+		&& source[source_size - 1] & 0x80))
+	  extension = 0xff;
+
+      /* Extend into MSBs of SOURCE.  */
+      if (byte_order == BFD_ENDIAN_BIG)
+	memset (dest, extension, size_diff);
+      else
+	memset (dest + source_size, extension, size_diff);
+    }
+}

 /* Return a `value' with the contents of (virtual or cooked) register
    REGNUM as found in the specified FRAME.  The register's type is
diff --git a/gdb/mips-fbsd-tdep.c b/gdb/mips-fbsd-tdep.c
index 00fae0ec60ddc9e645d3236efe29f2f9e9ceab5c..13cf98585f96f1acfe6decbe320530d609bee646 100644
--- a/gdb/mips-fbsd-tdep.c
+++ b/gdb/mips-fbsd-tdep.c
@@ -47,57 +47,24 @@
    34th is a dummy for padding.  */
 #define MIPS_FBSD_NUM_FPREGS	34

-/* Supply a single register.  If the source register size matches the
-   size the regcache expects, this can use regcache_raw_supply().  If
-   they are different, this copies the source register into a buffer
-   that can be passed to regcache_raw_supply().  */
+/* Supply a single register.  The register size might not match, so use
+   regcache->raw_supply_integer ().  */

 static void
 mips_fbsd_supply_reg (struct regcache *regcache, int regnum, const void *addr,
 		      size_t len)
 {
-  struct gdbarch *gdbarch = get_regcache_arch (regcache);
-
-  if (register_size (gdbarch, regnum) == len)
-    regcache_raw_supply (regcache, regnum, addr);
-  else
-    {
-      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      val = extract_signed_integer ((const gdb_byte *) addr, len, byte_order);
-      store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
-			    val);
-      regcache_raw_supply (regcache, regnum, buf);
-    }
+  regcache->raw_supply_integer (regnum, (const gdb_byte *) addr, len, true);
 }

-/* Collect a single register.  If the destination register size
-   matches the size the regcache expects, this can use
-   regcache_raw_supply().  If they are different, this fetches the
-   register via regcache_raw_supply() into a buffer and then copies it
-   into the final destination.  */
+/* Collect a single register.  The register size might not match, so use
+   regcache->raw_collect_integer ().  */

 static void
 mips_fbsd_collect_reg (const struct regcache *regcache, int regnum, void *addr,
 		       size_t len)
 {
-  struct gdbarch *gdbarch = get_regcache_arch (regcache);
-
-  if (register_size (gdbarch, regnum) == len)
-    regcache_raw_collect (regcache, regnum, addr);
-  else
-    {
-      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regnum, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regnum),
-				    byte_order);
-      store_signed_integer ((gdb_byte *) addr, len, byte_order, val);
-    }
+  regcache->raw_collect_integer (regnum, (gdb_byte *) addr, len, true);
 }

 /* Supply the floating-point registers stored in FPREGS to REGCACHE.
diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
index 48a582a16c934abe6e8f87c46a6009649c606d49..ccfdcdf98bc1e528cd768efaaaffaa3405708f71 100644
--- a/gdb/mips-linux-tdep.c
+++ b/gdb/mips-linux-tdep.c
@@ -116,13 +116,7 @@ mips_linux_get_longjmp_target (struct frame_info *frame, CORE_ADDR *pc)
 static void
 supply_32bit_reg (struct regcache *regcache, int regnum, const void *addr)
 {
-  struct gdbarch *gdbarch = get_regcache_arch (regcache);
-  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-  gdb_byte buf[MAX_REGISTER_SIZE];
-  store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
-			extract_signed_integer ((const gdb_byte *) addr, 4,
-						byte_order));
-  regcache_raw_supply (regcache, regnum, buf);
+  regcache->raw_supply_integer (regnum, (const gdb_byte *) addr, 4, true);
 }

 /* Unpack an elf_gregset_t into GDB's register cache.  */
@@ -417,7 +411,6 @@ mips64_fill_gregset (const struct regcache *regcache,
 		     mips64_elf_gregset_t *gregsetp, int regno)
 {
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
-  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   int regaddr, regi;
   mips64_elf_greg_t *regp = *gregsetp;
   void *dst;
@@ -460,14 +453,8 @@ mips64_fill_gregset (const struct regcache *regcache,

   if (regaddr != -1)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
       dst = regp + regaddr;
-      store_signed_integer ((gdb_byte *) dst, 8, byte_order, val);
+      regcache->raw_collect_integer (regno, (gdb_byte *) dst, 8, true);
     }
 }

@@ -564,25 +551,13 @@ mips64_fill_fpregset (const struct regcache *regcache,
     }
   else if (regno == mips_regnum (gdbarch)->fp_control_status)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
       to = (gdb_byte *) (*fpregsetp + 32);
-      store_signed_integer (to, 4, byte_order, val);
+      regcache->raw_collect_integer (regno, to, 4, true);
     }
   else if (regno == mips_regnum (gdbarch)->fp_implementation_revision)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
       to = (gdb_byte *) (*fpregsetp + 32) + 4;
-      store_signed_integer (to, 4, byte_order, val);
+      regcache->raw_collect_integer (regno, to, 4, true);
     }
   else if (regno == -1)
     {
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 4dcfccbac70f0f962bf5e5596d035fda42322795..409482d17c0542c7a53620d88d33fa9706fa72c5 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -294,8 +294,14 @@ public:

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

+  void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
+			    bool is_signed) const;
+
   void raw_supply (int regnum, const void *buf);

+  void raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len,
+			   bool is_signed);
+
   void raw_supply_zeroed (int regnum);

   void raw_copy (int regnum, struct regcache *src_regcache);
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 660558f7ff10f9d8346b6e08422e16c38c3c4d7d..ec6446b897922a8f9f44bbf94b7f1d198b0a6d4b 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1189,6 +1189,28 @@ regcache::raw_supply (int regnum, const void *buf)
     }
 }

+/* Supply register REGNUM with an integer, whose contents are stored in ADDR,
+   with length ADDR_LEN and sign IS_SIGNED, to REGCACHE.  */
+
+void
+regcache::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;
+  size_t regsize;
+
+  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
+  gdb_assert (!m_readonly_p);
+
+  regbuf = register_buffer (regnum);
+  regsize = m_descr->sizeof_register[regnum];
+
+  copy_integer_to_size (regbuf, regsize, addr, addr_len, is_signed,
+			byte_order);
+  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).  */
@@ -1232,6 +1254,26 @@ regcache::raw_collect (int regnum, void *buf) const
   memcpy (buf, regbuf, size);
 }

+/* Collect register REGNUM from regcache to an integer, whose contents are
+   stored in ADDR, with length ADDR_LEN and sign IS_SIGNED.  */
+
+void
+regcache::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;
+  size_t regsize;
+
+  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
+
+  regbuf = register_buffer (regnum);
+  regsize = m_descr->sizeof_register[regnum];
+
+  copy_integer_to_size (addr, addr_len, regbuf, regsize, is_signed,
+			byte_order);
+}
+
 void
 regcache::raw_copy (int regnum, struct regcache *src_regcache)
 {

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Pedro Alves]

On 05/24/2017 08:45 PM, Alan Hayward wrote:

> Added copy_integer_to_size, and removed the templates.
> 
> Manually tested of copy_integer_to_size to make sure the signs and
> endian parts all work.

Those manual tests would have been perfect candidates for some
unit tests.  All you'd need to do is add this at the bottom
of gdb/findvar.c:

#if GDB_SELF_TEST
namespace selftests {
namespace findvar_tests {

static void
run_test ()
{
  // Here, exercise the various code paths of copy_integer_to_size,
  // calling SELF_CHECK.
}

} // namespace findvar_test
} // namespace selftests

#endif

void
_initialize_findvar (void)
{
#if GDB_SELF_TEST
  register_self_test (selftests::findvar_tests::run_test);
#endif
}


(and include selftest.h at the top of the file).

You'd run those tests with:
 $ make check TESTS="gdb.gdb/unittest.exp"

Or (my preferred when hacking):
 $ gdb --batch -q -ex "maint selftest"


> Tested on a --enable-targets=all build using make check with board files
> unix and native-gdbserver.
> I do not have a MIPS machine to test on.
> Ok to commit?


On 05/24/2017 08:45 PM, Alan Hayward wrote:
> 
> 
> 2017-05-24  Alan Hayward  <alan.hayward@arm.com>
> 
> 	* gdb/defs.h (copy_integer_to_size): New declaration.
> 	* gdb/findvar.c (extract_signed_integer): Removed function.
> 	(extract_unsigned_integer): Likewise.
> 	(store_signed_integer): Removed function.
> 	(store_unsigned_integer): Likewise.
> 	* mips-fbsd-tdep.c (mips_fbsd_supply_reg): Use raw_supply_integer.
> 	(mips_fbsd_collect_reg): Use templated raw_collect_integer.
> 	* mips-linux-tdep.c (supply_32bit_reg): Use raw_supply_integer.
> 	(mips64_fill_gregset): Use raw_collect_integer.
> 	(mips64_fill_fpregset): Use raw_supply_integer.
> 	* gdb/regcache.c (regcache::raw_supply_integer): New function.
> 	(regcache::raw_collect_integer): Likewise
> 	* gdb/regcache.h (regcache::raw_supply): New declaration.
> 	(regcache::raw_collect): Likewise

There are stale entries above.  Also, drop "gdb/" prefix, and add 
missing periods after "Likewise".

> 
> 
> diff --git a/gdb/defs.h b/gdb/defs.h
> index a0b586f401eca205334e9f237081f4da97c83aa1..a1a97bb1e791d4f423788797d1f04c3e89877d90 100644
> --- a/gdb/defs.h
> +++ b/gdb/defs.h
> @@ -658,7 +658,10 @@ extern void store_unsigned_integer (gdb_byte *, int,
>  extern void store_typed_address (gdb_byte *buf, struct type *type,
>  				 CORE_ADDR addr);
> 
> -
> 
> +extern void copy_integer_to_size (gdb_byte *dest, int dest_size,
> +				  const gdb_byte *source, int source_size,
> +				  bool is_signed, enum bfd_endian byte_order);
> +
>  /* From valops.c */
> 
>  extern int watchdog;
> diff --git a/gdb/findvar.c b/gdb/findvar.c
> index ed4d5c1266c9de069981b306bc8229ae5fb02350..5a82e493f9ca6d9337a22defc4377235f36acba8 100644
> --- a/gdb/findvar.c
> +++ b/gdb/findvar.c
> @@ -249,7 +249,47 @@ store_typed_address (gdb_byte *buf, struct type *type, CORE_ADDR addr)
>    gdbarch_address_to_pointer (get_type_arch (type), type, buf, addr);
>  }
> 
> +/* Copy a value from SOURCE of size SOURCE_SIZE bytes to DEST of size DEST_SIZE
> +   bytes.  If SOURCE_SIZE is greater than DEST_SIZE, then truncate the most
> +   significant bytes.  If SOURCE_SIZE is less than DEST_SIZE then either sign
> +   or zero extended according to IS_SIGNED.  Values are stored in memory with
> +   endianess BYTE_ORDER.  */
> 
> +void
> +copy_integer_to_size (gdb_byte *dest, int dest_size, const gdb_byte *source,
> +		      int source_size, bool is_signed,
> +		      enum bfd_endian byte_order)
> +{
> +  signed int size_diff = dest_size - source_size;
> +
> +  /* Copy across everything from SOURCE that can fit into DEST.  */
> +
> +  if (byte_order == BFD_ENDIAN_BIG && size_diff > 0)
> +    memcpy (dest + size_diff, source, source_size);
> +  else if (byte_order == BFD_ENDIAN_BIG && size_diff < 0)
> +    memcpy (dest, source - size_diff, dest_size);
> +  else
> +    memcpy (dest, source, std::min (source_size, dest_size));
> +
> +  /* Fill the remaining space in DEST by either zero extending or sign
> +     extending.  */
> +
> +  if (size_diff > 0)
> +    {
> +      char extension = 0;

gdb_byte.

> +      if (is_signed)
> +	if ((byte_order == BFD_ENDIAN_BIG && source[0] & 0x80)
> +	    || (byte_order != BFD_ENDIAN_BIG
> +		&& source[source_size - 1] & 0x80))
> +	  extension = 0xff;
> +

Merge the ifs?  Like, e.g.:

  gdb_byte extension;

  if (is_signed
      && ((byte_order == BFD_ENDIAN_BIG && source[0] & 0x80)
	  || (byte_order != BFD_ENDIAN_BIG
	      && source[source_size - 1] & 0x80)))
    extension = 0xff;
  else
    extension = 0;


> +      /* Extend into MSBs of SOURCE.  */
> +      if (byte_order == BFD_ENDIAN_BIG)
> +	memset (dest, extension, size_diff);
> +      else
> +	memset (dest + source_size, extension, size_diff);
> +    }
> +}
> 
>  /* Return a `value' with the contents of (virtual or cooked) register
>     REGNUM as found in the specified FRAME.  The register's type is
> diff --git a/gdb/mips-fbsd-tdep.c b/gdb/mips-fbsd-tdep.c
> index 00fae0ec60ddc9e645d3236efe29f2f9e9ceab5c..13cf98585f96f1acfe6decbe320530d609bee646 100644
> --- a/gdb/mips-fbsd-tdep.c
> +++ b/gdb/mips-fbsd-tdep.c
> @@ -47,57 +47,24 @@
>     34th is a dummy for padding.  */
>  #define MIPS_FBSD_NUM_FPREGS	34
> 
> -/* Supply a single register.  If the source register size matches the
> -   size the regcache expects, this can use regcache_raw_supply().  If
> -   they are different, this copies the source register into a buffer
> -   that can be passed to regcache_raw_supply().  */
> +/* Supply a single register.  The register size might not match, so use
> +   regcache->raw_supply_integer ().  */
> 
>  static void
>  mips_fbsd_supply_reg (struct regcache *regcache, int regnum, const void *addr,
>  		      size_t len)
>  {
> -  struct gdbarch *gdbarch = get_regcache_arch (regcache);
> -
> -  if (register_size (gdbarch, regnum) == len)
> -    regcache_raw_supply (regcache, regnum, addr);
> -  else
> -    {
> -      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> -      gdb_byte buf[MAX_REGISTER_SIZE];
> -      LONGEST val;
> -
> -      val = extract_signed_integer ((const gdb_byte *) addr, len, byte_order);
> -      store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
> -			    val);
> -      regcache_raw_supply (regcache, regnum, buf);
> -    }
> +  regcache->raw_supply_integer (regnum, (const gdb_byte *) addr, len, true);
>  }

Nice!

> --- a/gdb/mips-linux-tdep.c
> +++ b/gdb/mips-linux-tdep.c
> @@ -116,13 +116,7 @@ mips_linux_get_longjmp_target (struct frame_info *frame, CORE_ADDR *pc)
>  static void
>  supply_32bit_reg (struct regcache *regcache, int regnum, const void *addr)
>  {
> -  struct gdbarch *gdbarch = get_regcache_arch (regcache);
> -  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> -  gdb_byte buf[MAX_REGISTER_SIZE];
> -  store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
> -			extract_signed_integer ((const gdb_byte *) addr, 4,
> -						byte_order));
> -  regcache_raw_supply (regcache, regnum, buf);
> +  regcache->raw_supply_integer (regnum, (const gdb_byte *) addr, 4, true);
>  }

Nice.  :-)
[snip several "nice"s]

> diff --git a/gdb/regcache.h b/gdb/regcache.h
> index 4dcfccbac70f0f962bf5e5596d035fda42322795..409482d17c0542c7a53620d88d33fa9706fa72c5 100644
> --- a/gdb/regcache.h
> +++ b/gdb/regcache.h
> @@ -294,8 +294,14 @@ public:
> 
>    void raw_collect (int regnum, void *buf) const;
> 
> +  void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
> +			    bool is_signed) const;
> +
>    void raw_supply (int regnum, const void *buf);
> 
> +  void raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len,
> +			   bool is_signed);
> +
>    void raw_supply_zeroed (int regnum);
> 
>    void raw_copy (int regnum, struct regcache *src_regcache);
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 660558f7ff10f9d8346b6e08422e16c38c3c4d7d..ec6446b897922a8f9f44bbf94b7f1d198b0a6d4b 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -1189,6 +1189,28 @@ regcache::raw_supply (int regnum, const void *buf)
>      }
>  }
> 
> +/* Supply register REGNUM with an integer, whose contents are stored in ADDR,
> +   with length ADDR_LEN and sign IS_SIGNED, to REGCACHE.  */

It'd be good to say something about extending/truncating here, as
well as mention that the ADDR contents are assumed to be in
target byte order.

> +
> +void
> +regcache::raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len,
> +			      bool is_signed)
> +{

> +/* Collect register REGNUM from regcache to an integer, whose contents are
> +   stored in ADDR, with length ADDR_LEN and sign IS_SIGNED.  */

Ditto.

> +
> +void
> +regcache::raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
> +			       bool is_signed) const
> +{

Otherwise this looks good to me.  It'd look great with unit tests.  :-)

Yao, what do you think?

Thanks,
Pedro Alves

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> Those manual tests would have been perfect candidates for some
> unit tests.  All you'd need to do is add this at the bottom
> of gdb/findvar.c:
>
> #if GDB_SELF_TEST
> namespace selftests {
> namespace findvar_tests {
>
> static void
> run_test ()
> {
>   // Here, exercise the various code paths of copy_integer_to_size,
>   // calling SELF_CHECK.
> }
>
> } // namespace findvar_test
> } // namespace selftests
>
> #endif
>
> void
> _initialize_findvar (void)
> {
> #if GDB_SELF_TEST
>   register_self_test (selftests::findvar_tests::run_test);
> #endif
> }
>
>

It is great to have a unit test, and that is what I want to suggest.

>> +/* Supply a single register.  The register size might not match, so use
>> +   regcache->raw_supply_integer ().  */
>> 
>>  static void
>>  mips_fbsd_supply_reg (struct regcache *regcache, int regnum, const void *addr,
>>  		      size_t len)
>>  {
>> -  struct gdbarch *gdbarch = get_regcache_arch (regcache);
>> -
>> -  if (register_size (gdbarch, regnum) == len)
>> -    regcache_raw_supply (regcache, regnum, addr);
>> -  else
>> -    {
>> -      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>> -      gdb_byte buf[MAX_REGISTER_SIZE];
>> -      LONGEST val;
>> -
>> -      val = extract_signed_integer ((const gdb_byte *) addr, len, byte_order);
>> -      store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
>> -			    val);
>> -      regcache_raw_supply (regcache, regnum, buf);
>> -    }
>> +  regcache->raw_supply_integer (regnum, (const gdb_byte *) addr, len, true);
>>  }
>
> Nice!
>

Can we completely remove mips_fbsd_supply_reg? and use
regcache->raw_supply_integer instead?

>> --- a/gdb/mips-linux-tdep.c
>> +++ b/gdb/mips-linux-tdep.c
>> @@ -116,13 +116,7 @@ mips_linux_get_longjmp_target (struct
>> frame_info *frame, CORE_ADDR *pc)
>>  static void
>>  supply_32bit_reg (struct regcache *regcache, int regnum, const void *addr)
>>  {
>> -  struct gdbarch *gdbarch = get_regcache_arch (regcache);
>> -  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>> -  gdb_byte buf[MAX_REGISTER_SIZE];
>> -  store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
>> -			extract_signed_integer ((const gdb_byte *) addr, 4,
>> -						byte_order));
>> -  regcache_raw_supply (regcache, regnum, buf);
>> +  regcache->raw_supply_integer (regnum, (const gdb_byte *) addr, 4, true);
>>  }
>
> Nice.  :-)
> [snip several "nice"s]
>

Likewise, remove supply_32bit_reg and use regcache->raw_supply_integer.

-- 
Yao (齐尧)

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Pedro Alves]

On 05/25/2017 12:43 PM, Yao Qi wrote:
>>> >>  static void
>>> >>  supply_32bit_reg (struct regcache *regcache, int regnum, const void *addr)
>>> >>  {
>>> >> -  struct gdbarch *gdbarch = get_regcache_arch (regcache);
>>> >> -  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>>> >> -  gdb_byte buf[MAX_REGISTER_SIZE];
>>> >> -  store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
>>> >> -			extract_signed_integer ((const gdb_byte *) addr, 4,
>>> >> -						byte_order));
>>> >> -  regcache_raw_supply (regcache, regnum, buf);
>>> >> +  regcache->raw_supply_integer (regnum, (const gdb_byte *) addr, 4, true);
>>> >>  }
>> >
>> > Nice.  :-)
>> > [snip several "nice"s]
>> >
> Likewise, remove supply_32bit_reg and use regcache->raw_supply_integer.

There are multiple calls to it, so inlining it would require writing
the "4, true" arguments at all call sites.
Leaving supply_32bit_reg (etc.) as small wrapper functions helps
readability, IMHO.

Thanks,
Pedro Alves

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Alan Hayward]

> On 25 May 2017, at 12:48, Pedro Alves <palves@redhat.com> wrote:
> 
> On 05/25/2017 12:43 PM, Yao Qi wrote:
>>>>>> static void
>>>>>> supply_32bit_reg (struct regcache *regcache, int regnum, const void *addr)
>>>>>> {
>>>>>> -  struct gdbarch *gdbarch = get_regcache_arch (regcache);
>>>>>> -  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>>>>>> -  gdb_byte buf[MAX_REGISTER_SIZE];
>>>>>> -  store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
>>>>>> -			extract_signed_integer ((const gdb_byte *) addr, 4,
>>>>>> -						byte_order));
>>>>>> -  regcache_raw_supply (regcache, regnum, buf);
>>>>>> +  regcache->raw_supply_integer (regnum, (const gdb_byte *) addr, 4, true);
>>>>>> }
>>>> 
>>>> Nice.  :-)
>>>> [snip several "nice"s]
>>>> 
>> Likewise, remove supply_32bit_reg and use regcache->raw_supply_integer.
> 
> There are multiple calls to it, so inlining it would require writing
> the "4, true" arguments at all call sites.
> Leaving supply_32bit_reg (etc.) as small wrapper functions helps
> readability, IMHO.
> 

Agreed.
I considered removing, but it adds lots of extra arguments everywhere.

Unit tests added as suggested in the other email.
Using store_unsigned_integer and extract_unsigned_integer means that
I don't have to worry about endian specifics in the test.
I could template the two tests, but it looks cleaner this way.


Tested on a --enable-targets=all build using make check with board files
unix and native-gdbserver.
I do not have a MIPS machine to test on.
Ok to commit?

Alan.

2017-05-25  Alan Hayward  <alan.hayward@arm.com>

	* defs.h (copy_integer_to_size): New declaration.
	* findvar.c (copy_integer_to_size): New function.
	(do_cuint_test): New selftest function.
	(do_cint_test): New selftest function.
	(copy_integer_to_size_test): Likewise.
	(_initialize_findvar): Likewise.
	* mips-fbsd-tdep.c (mips_fbsd_supply_reg): Use raw_supply_integer.
	(mips_fbsd_collect_reg): Use raw_collect_integer.
	* mips-linux-tdep.c (supply_32bit_reg): Use raw_supply_integer.
	(mips64_fill_gregset): Use raw_collect_integer
	(mips64_fill_fpregset): Use raw_supply_integer.
	* regcache.c (regcache::raw_supply_integer): New function.
	(regcache::raw_collect_integer): Likewise.
	* regcache.h: (regcache::raw_supply_integer): New declaration.
        (regcache::raw_collect_integer): Likewise.


diff --git a/gdb/defs.h b/gdb/defs.h
index a0b586f401eca205334e9f237081f4da97c83aa1..a1a97bb1e791d4f423788797d1f04c3e89877d90 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -658,7 +658,10 @@ extern void store_unsigned_integer (gdb_byte *, int,
 extern void store_typed_address (gdb_byte *buf, struct type *type,
 				 CORE_ADDR addr);

-

+extern void copy_integer_to_size (gdb_byte *dest, int dest_size,
+				  const gdb_byte *source, int source_size,
+				  bool is_signed, enum bfd_endian byte_order);
+
 /* From valops.c */

 extern int watchdog;
diff --git a/gdb/findvar.c b/gdb/findvar.c
index ed4d5c1266c9de069981b306bc8229ae5fb02350..bf72b2dc7710ca21b16056b8f0f274044bf744ca 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -33,6 +33,7 @@
 #include "objfiles.h"
 #include "language.h"
 #include "dwarf2loc.h"
+#include "selftest.h"

 /* Basic byte-swapping routines.  All 'extract' functions return a
    host-format integer from a target-format integer at ADDR which is
@@ -249,7 +250,46 @@ store_typed_address (gdb_byte *buf, struct type *type, CORE_ADDR addr)
   gdbarch_address_to_pointer (get_type_arch (type), type, buf, addr);
 }

+/* Copy a value from SOURCE of size SOURCE_SIZE bytes to DEST of size DEST_SIZE
+   bytes.  If SOURCE_SIZE is greater than DEST_SIZE, then truncate the most
+   significant bytes.  If SOURCE_SIZE is less than DEST_SIZE then either sign
+   or zero extended according to IS_SIGNED.  Values are stored in memory with
+   endianess BYTE_ORDER.  */

+void
+copy_integer_to_size (gdb_byte *dest, int dest_size, const gdb_byte *source,
+		      int source_size, bool is_signed,
+		      enum bfd_endian byte_order)
+{
+  signed int size_diff = dest_size - source_size;
+
+  /* Copy across everything from SOURCE that can fit into DEST.  */
+
+  if (byte_order == BFD_ENDIAN_BIG && size_diff > 0)
+    memcpy (dest + size_diff, source, source_size);
+  else if (byte_order == BFD_ENDIAN_BIG && size_diff < 0)
+    memcpy (dest, source - size_diff, dest_size);
+  else
+    memcpy (dest, source, std::min (source_size, dest_size));
+
+  /* Fill the remaining space in DEST by either zero extending or sign
+     extending.  */
+
+  if (size_diff > 0)
+    {
+      gdb_byte extension = 0;
+      if (is_signed
+	  && ((byte_order != BFD_ENDIAN_BIG && source[source_size - 1] & 0x80)
+	      || (byte_order == BFD_ENDIAN_BIG && source[0] & 0x80)))
+	extension = 0xff;
+
+      /* Extend into MSBs of SOURCE.  */
+      if (byte_order == BFD_ENDIAN_BIG)
+	memset (dest, extension, size_diff);
+      else
+	memset (dest + source_size, extension, size_diff);
+    }
+}

 /* Return a `value' with the contents of (virtual or cooked) register
    REGNUM as found in the specified FRAME.  The register's type is
@@ -1005,3 +1045,78 @@ address_from_register (int regnum, struct frame_info *frame)
   return result;
 }

+#if GDB_SELF_TEST
+namespace selftests {
+namespace findvar_tests {
+
+/* Functions to test copy_integer_to_size.  Store SOURCE_VAL with size
+   SOURCE_SIZE to a buffer, making sure no sign extending happens at this
+   stage.  Copy buffer to a new buffer using copy_integer_to_size.  Extract
+   copied value and compare to DEST_VAL.  Do all of this for both LITTLE and
+   BIG target endians.  */
+
+static void
+do_cuint_test (long dest_val, int dest_size, ULONGEST src_val, int src_size)
+{
+  for (int i = 0; i < 2 ; i++)
+    {
+      ULONGEST srcbuf_val = 0, destbuf_val = 0;
+      gdb_byte* srcbuf = (gdb_byte*) &srcbuf_val;
+      gdb_byte* destbuf = (gdb_byte*) &destbuf_val;
+      enum bfd_endian byte_order = i ? BFD_ENDIAN_BIG : BFD_ENDIAN_LITTLE;
+
+      store_unsigned_integer (srcbuf, src_size, byte_order, src_val);
+      copy_integer_to_size (destbuf, dest_size, srcbuf, src_size, false,
+			    byte_order);
+      SELF_CHECK (dest_val == extract_unsigned_integer (destbuf, dest_size,
+							byte_order));
+    }
+}
+
+static void
+do_cint_test (long dest_val, int dest_size, LONGEST src_val, int src_size)
+{
+  for (int i = 0; i < 2 ; i++)
+    {
+      LONGEST srcbuf_val = 0, destbuf_val = 0;
+      gdb_byte* srcbuf = (gdb_byte*) &srcbuf_val;
+      gdb_byte* destbuf = (gdb_byte*) &destbuf_val;
+      enum bfd_endian byte_order = i ? BFD_ENDIAN_LITTLE : BFD_ENDIAN_BIG;
+
+      store_unsigned_integer (srcbuf, src_size, byte_order, src_val);
+      copy_integer_to_size (destbuf, dest_size, srcbuf, src_size, true,
+			    byte_order);
+      SELF_CHECK (dest_val == extract_signed_integer (destbuf, dest_size,
+						      byte_order));
+    }
+}
+
+static void
+copy_integer_to_size_test ()
+{
+  do_cuint_test (0x12345678, 8, 0x12345678, 4);
+  do_cuint_test (0x345678, 8, 0x12345678, 3);
+  do_cuint_test (0x5678, 2, 0x12345678, 3);
+  do_cuint_test (0xdeadbeef, 8, 0xdeadbeef, 4);
+  do_cuint_test (0xadbeef, 8, 0xdeadbeef, 3);
+  do_cuint_test (0xbeef, 2, 0xdeadbeef, 3);
+  do_cint_test (0x12345678, 8, 0x12345678, 4);
+  do_cint_test (0x345678, 8, 0x12345678, 3);
+  do_cint_test (0x5678, 2, 0x12345678, 3);
+  do_cint_test (0xffffffffdeadbeef, 8, 0xdeadbeef, 4);
+  do_cint_test (0xffffffffffadbeef, 8, 0xdeadbeef, 3);
+  do_cint_test (0xffffffffffffbeef, 2, 0xbeef, 3);
+}
+
+} // namespace findvar_test
+} // namespace selftests
+
+#endif
+
+void
+_initialize_findvar (void)
+{
+#if GDB_SELF_TEST
+  register_self_test (selftests::findvar_tests::copy_integer_to_size_test);
+#endif
+}
diff --git a/gdb/mips-fbsd-tdep.c b/gdb/mips-fbsd-tdep.c
index 00fae0ec60ddc9e645d3236efe29f2f9e9ceab5c..13cf98585f96f1acfe6decbe320530d609bee646 100644
--- a/gdb/mips-fbsd-tdep.c
+++ b/gdb/mips-fbsd-tdep.c
@@ -47,57 +47,24 @@
    34th is a dummy for padding.  */
 #define MIPS_FBSD_NUM_FPREGS	34

-/* Supply a single register.  If the source register size matches the
-   size the regcache expects, this can use regcache_raw_supply().  If
-   they are different, this copies the source register into a buffer
-   that can be passed to regcache_raw_supply().  */
+/* Supply a single register.  The register size might not match, so use
+   regcache->raw_supply_integer ().  */

 static void
 mips_fbsd_supply_reg (struct regcache *regcache, int regnum, const void *addr,
 		      size_t len)
 {
-  struct gdbarch *gdbarch = get_regcache_arch (regcache);
-
-  if (register_size (gdbarch, regnum) == len)
-    regcache_raw_supply (regcache, regnum, addr);
-  else
-    {
-      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      val = extract_signed_integer ((const gdb_byte *) addr, len, byte_order);
-      store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
-			    val);
-      regcache_raw_supply (regcache, regnum, buf);
-    }
+  regcache->raw_supply_integer (regnum, (const gdb_byte *) addr, len, true);
 }

-/* Collect a single register.  If the destination register size
-   matches the size the regcache expects, this can use
-   regcache_raw_supply().  If they are different, this fetches the
-   register via regcache_raw_supply() into a buffer and then copies it
-   into the final destination.  */
+/* Collect a single register.  The register size might not match, so use
+   regcache->raw_collect_integer ().  */

 static void
 mips_fbsd_collect_reg (const struct regcache *regcache, int regnum, void *addr,
 		       size_t len)
 {
-  struct gdbarch *gdbarch = get_regcache_arch (regcache);
-
-  if (register_size (gdbarch, regnum) == len)
-    regcache_raw_collect (regcache, regnum, addr);
-  else
-    {
-      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regnum, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regnum),
-				    byte_order);
-      store_signed_integer ((gdb_byte *) addr, len, byte_order, val);
-    }
+  regcache->raw_collect_integer (regnum, (gdb_byte *) addr, len, true);
 }

 /* Supply the floating-point registers stored in FPREGS to REGCACHE.
diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
index 48a582a16c934abe6e8f87c46a6009649c606d49..ccfdcdf98bc1e528cd768efaaaffaa3405708f71 100644
--- a/gdb/mips-linux-tdep.c
+++ b/gdb/mips-linux-tdep.c
@@ -116,13 +116,7 @@ mips_linux_get_longjmp_target (struct frame_info *frame, CORE_ADDR *pc)
 static void
 supply_32bit_reg (struct regcache *regcache, int regnum, const void *addr)
 {
-  struct gdbarch *gdbarch = get_regcache_arch (regcache);
-  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-  gdb_byte buf[MAX_REGISTER_SIZE];
-  store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
-			extract_signed_integer ((const gdb_byte *) addr, 4,
-						byte_order));
-  regcache_raw_supply (regcache, regnum, buf);
+  regcache->raw_supply_integer (regnum, (const gdb_byte *) addr, 4, true);
 }

 /* Unpack an elf_gregset_t into GDB's register cache.  */
@@ -417,7 +411,6 @@ mips64_fill_gregset (const struct regcache *regcache,
 		     mips64_elf_gregset_t *gregsetp, int regno)
 {
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
-  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   int regaddr, regi;
   mips64_elf_greg_t *regp = *gregsetp;
   void *dst;
@@ -460,14 +453,8 @@ mips64_fill_gregset (const struct regcache *regcache,

   if (regaddr != -1)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
       dst = regp + regaddr;
-      store_signed_integer ((gdb_byte *) dst, 8, byte_order, val);
+      regcache->raw_collect_integer (regno, (gdb_byte *) dst, 8, true);
     }
 }

@@ -564,25 +551,13 @@ mips64_fill_fpregset (const struct regcache *regcache,
     }
   else if (regno == mips_regnum (gdbarch)->fp_control_status)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
       to = (gdb_byte *) (*fpregsetp + 32);
-      store_signed_integer (to, 4, byte_order, val);
+      regcache->raw_collect_integer (regno, to, 4, true);
     }
   else if (regno == mips_regnum (gdbarch)->fp_implementation_revision)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
       to = (gdb_byte *) (*fpregsetp + 32) + 4;
-      store_signed_integer (to, 4, byte_order, val);
+      regcache->raw_collect_integer (regno, to, 4, true);
     }
   else if (regno == -1)
     {
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 4dcfccbac70f0f962bf5e5596d035fda42322795..409482d17c0542c7a53620d88d33fa9706fa72c5 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -294,8 +294,14 @@ public:

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

+  void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
+			    bool is_signed) const;
+
   void raw_supply (int regnum, const void *buf);

+  void raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len,
+			   bool is_signed);
+
   void raw_supply_zeroed (int regnum);

   void raw_copy (int regnum, struct regcache *src_regcache);
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 660558f7ff10f9d8346b6e08422e16c38c3c4d7d..90462aea73ba810bd1af9ddb8c6d02e1f0e95991 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1189,6 +1189,31 @@ 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.  */
+
+void
+regcache::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;
+  size_t regsize;
+
+  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
+  gdb_assert (!m_readonly_p);
+
+  regbuf = register_buffer (regnum);
+  regsize = m_descr->sizeof_register[regnum];
+
+  copy_integer_to_size (regbuf, regsize, addr, addr_len, is_signed,
+			byte_order);
+  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).  */
@@ -1232,6 +1257,29 @@ regcache::raw_collect (int regnum, void *buf) const
   memcpy (buf, regbuf, size);
 }

+/* 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
+regcache::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;
+  size_t regsize;
+
+  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
+
+  regbuf = register_buffer (regnum);
+  regsize = m_descr->sizeof_register[regnum];
+
+  copy_integer_to_size (addr, addr_len, regbuf, regsize, is_signed,
+			byte_order);
+}
+
 void
 regcache::raw_copy (int regnum, struct regcache *src_regcache)
 {

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Pedro Alves]

On 05/26/2017 09:54 AM, Alan Hayward wrote:

> 2017-05-25  Alan Hayward  <alan.hayward@arm.com>
> 
> 	* defs.h (copy_integer_to_size): New declaration.
> 	* findvar.c (copy_integer_to_size): New function.
> 	(do_cuint_test): New selftest function.
> 	(do_cint_test): New selftest function.
> 	(copy_integer_to_size_test): Likewise.
> 	(_initialize_findvar): Likewise.
> 	* mips-fbsd-tdep.c (mips_fbsd_supply_reg): Use raw_supply_integer.
> 	(mips_fbsd_collect_reg): Use raw_collect_integer.
> 	* mips-linux-tdep.c (supply_32bit_reg): Use raw_supply_integer.
> 	(mips64_fill_gregset): Use raw_collect_integer
> 	(mips64_fill_fpregset): Use raw_supply_integer.
> 	* regcache.c (regcache::raw_supply_integer): New function.
> 	(regcache::raw_collect_integer): Likewise.
> 	* regcache.h: (regcache::raw_supply_integer): New declaration.
>         (regcache::raw_collect_integer): Likewise.

Last line looks like needs s/spaces/tab/.

> +#if GDB_SELF_TEST
> +namespace selftests {
> +namespace findvar_tests {
> +
> +/* Functions to test copy_integer_to_size.  Store SOURCE_VAL with size
> +   SOURCE_SIZE to a buffer, making sure no sign extending happens at this
> +   stage.  Copy buffer to a new buffer using copy_integer_to_size.  Extract
> +   copied value and compare to DEST_VAL.  Do all of this for both LITTLE and
> +   BIG target endians.  */
> +
> +static void
> +do_cuint_test (long dest_val, int dest_size, ULONGEST src_val, int src_size)
> +{
> +  for (int i = 0; i < 2 ; i++)
> +    {
> +      ULONGEST srcbuf_val = 0, destbuf_val = 0;
> +      gdb_byte* srcbuf = (gdb_byte*) &srcbuf_val;
> +      gdb_byte* destbuf = (gdb_byte*) &destbuf_val;

Nit, I'd prefer writing instead:

      gdb_byte srcbuf[sizeof (ULONGEST)] = {};
      gdb_byte destbuf[sizeof (ULONGEST)] = {};

I.e., get rid of the local ULONGEST variables.  It took me a second
to realize that below you're writing to these variables instead of
to "dest_val" directly, which made me first wonder about
strict aliasing issues.

(But see further below.)

> +static void
> +do_cint_test (long dest_val, int dest_size, LONGEST src_val, int src_size)
> +{
> +  for (int i = 0; i < 2 ; i++)
> +    {
> +      LONGEST srcbuf_val = 0, destbuf_val = 0;
> +      gdb_byte* srcbuf = (gdb_byte*) &srcbuf_val;
> +      gdb_byte* destbuf = (gdb_byte*) &destbuf_val;

Ditto.

> +      enum bfd_endian byte_order = i ? BFD_ENDIAN_LITTLE : BFD_ENDIAN_BIG;
> +
> +      store_unsigned_integer (srcbuf, src_size, byte_order, src_val);
> +      copy_integer_to_size (destbuf, dest_size, srcbuf, src_size, true,
> +			    byte_order);
> +      SELF_CHECK (dest_val == extract_signed_integer (destbuf, dest_size,
> +						      byte_order));
> +    }
> +}
> +
> +static void
> +copy_integer_to_size_test ()
> +{

...

> +  do_cint_test (0xffffffffdeadbeef, 8, 0xdeadbeef, 4);
> +  do_cint_test (0xffffffffffadbeef, 8, 0xdeadbeef, 3);
> +  do_cint_test (0xffffffffffffbeef, 2, 0xbeef, 3);

Note that:

> +static void
> +do_cint_test (long dest_val, int dest_size, LONGEST src_val, int src_size)

"long" can be 32-bit, depending on host.  It's 32-bit on 64-bit Windows.

So you're passing 0xffffffffffffbeef, which gets truncated
to 0xffffbeef, but then sign extended here again:

      SELF_CHECK (dest_val == extract_signed_integer (destbuf, dest_size,
						      byte_order));

and it'll work out (by chance).

I think it would be better to always pass an unsigned ULONGEST
as dest_val:

 static void
 do_cint_test (ULONGEST dest_val, int dest_size, LONGEST src_val, int src_size)

likewise for do_cuint_test, of course.

Makes me notice that you didn't add any unsigned test for
src/dest size > 4, which would fail on 64-bit Windows, for the
same reason (32-bit long), like:

 do_cuint_test (0xff12345678, 8, 0xff12345678, 6);


I notice you have no tests that exercise src_size == dest_size,
for multiple sizes.  It'd be good to add them.


Another issue with the test is that by relying on
extract_(un)signed_integer, the test can't notice if the
zero / sign extension in copy_integer_bytes filled the right
number of bytes:

> +  do_cint_test (0xffffffffffffbeef, 2, 0xbeef, 3);

I.e., above, the last test _must_ not write more than
two bytes to the destination, so those leading
0xFFs before "beef" are artificial.

Passing ULONGEST as destination value avoids that problem, making
you write that last test as:

   do_cint_test (0x000000000000beef, 2, 0xbeef, 3);


That brings us to the next problem, that you can't tell
whether zero extension writes the zeros in the first place.

I.e., here:

 do_cuint_test (0x12345678, 8, 0x12345678, 4);

Given do_cuint_test does this:

+      ULONGEST srcbuf_val = 0, destbuf_val = 0;

You can't tell whether copy_integer_to_size really
filled in bytes 4-8 with zero.

You can get that back by making the do_cint_test/do_int_test
functions memset the temporary buffers with some byte
that the callers never pass down as part of any src/dest value.
Like, say reserve 0xaa for that:

      gdb_byte srcbuf[sizeof (ULONGEST)];
      gdb_byte destbuf[sizeof (ULONGEST)];

      memset (srcbuf, 0xaa, sizeof (srcbuf));
      memset (destbuf, 0xaa, sizeof (srcbuf));

(with suitable comments)

So in sum:

#1 - ULONGEST as destination value, both signed and unsigned tests.
#2 - Add unsigned tests with size >4
#3 - Add tests src_size == dest_size
#4 - Use gdb_byte arrays for temporary buffers, and memset them with 0xaa.
#5 - Adjust tests to pass.
#6 - Add comments where appropriate.

Thanks,
Pedro Alves

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Alan Hayward]

> On 26 May 2017, at 11:26, Pedro Alves <palves@redhat.com> wrote:


> Another issue with the test is that by relying on
> extract_(un)signed_integer, the test can't notice if the
> zero / sign extension in copy_integer_bytes filled the right
> number of bytes:
> 
>> +  do_cint_test (0xffffffffffffbeef, 2, 0xbeef, 3);
> 
> I.e., above, the last test _must_ not write more than
> two bytes to the destination, so those leading
> 0xFFs before "beef" are artificial.
> 
> Passing ULONGEST as destination value avoids that problem, making
> you write that last test as:
> 
>   do_cint_test (0x000000000000beef, 2, 0xbeef, 3);
> 

To make that work, I also had to make extract use the unsigned
version. So now all the stores and extracts are working on unsigned data.

> 
> 
> So in sum:
> 
> #1 - ULONGEST as destination value, both signed and unsigned tests.
> #2 - Add unsigned tests with size >4
> #3 - Add tests src_size == dest_size
> #4 - Use gdb_byte arrays for temporary buffers, and memset them with 0xaa.
> #5 - Adjust tests to pass.
> #6 - Add comments where appropriate.
> 

All the above fixed up.
After doing this, I noticed it was easier to combine the two test functions
into one.


Tested on a --enable-targets=all build using make check with board files
unix and native-gdbserver.
I do not have a MIPS machine to test on.
Ok to commit?

Alan.

2017-05-26  Alan Hayward  <alan.hayward@arm.com>

	* defs.h (copy_integer_to_size): New declaration.
	* findvar.c (copy_integer_to_size): New function.
	(do_cint_test): New selftest function.
	(copy_integer_to_size_test): Likewise.
	(_initialize_findvar): Likewise.
	* mips-fbsd-tdep.c (mips_fbsd_supply_reg): Use raw_supply_integer.
	(mips_fbsd_collect_reg): Use raw_collect_integer.
	* mips-linux-tdep.c (supply_32bit_reg): Use raw_supply_integer.
	(mips64_fill_gregset): Use raw_collect_integer
	(mips64_fill_fpregset): Use raw_supply_integer.
	* regcache.c (regcache::raw_supply_integer): New function.
	(regcache::raw_collect_integer): Likewise.
	* regcache.h: (regcache::raw_supply_integer): New declaration.
	(regcache::raw_collect_integer): Likewise.


diff --git a/gdb/defs.h b/gdb/defs.h
index a0b586f401eca205334e9f237081f4da97c83aa1..a1a97bb1e791d4f423788797d1f04c3e89877d90 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -658,7 +658,10 @@ extern void store_unsigned_integer (gdb_byte *, int,
 extern void store_typed_address (gdb_byte *buf, struct type *type,
 				 CORE_ADDR addr);

-

+extern void copy_integer_to_size (gdb_byte *dest, int dest_size,
+				  const gdb_byte *source, int source_size,
+				  bool is_signed, enum bfd_endian byte_order);
+
 /* From valops.c */

 extern int watchdog;
diff --git a/gdb/findvar.c b/gdb/findvar.c
index ed4d5c1266c9de069981b306bc8229ae5fb02350..afa0ea428f67093c5b750687b54c5889a389b52f 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -33,6 +33,7 @@
 #include "objfiles.h"
 #include "language.h"
 #include "dwarf2loc.h"
+#include "selftest.h"

 /* Basic byte-swapping routines.  All 'extract' functions return a
    host-format integer from a target-format integer at ADDR which is
@@ -249,7 +250,46 @@ store_typed_address (gdb_byte *buf, struct type *type, CORE_ADDR addr)
   gdbarch_address_to_pointer (get_type_arch (type), type, buf, addr);
 }

+/* Copy a value from SOURCE of size SOURCE_SIZE bytes to DEST of size DEST_SIZE
+   bytes.  If SOURCE_SIZE is greater than DEST_SIZE, then truncate the most
+   significant bytes.  If SOURCE_SIZE is less than DEST_SIZE then either sign
+   or zero extended according to IS_SIGNED.  Values are stored in memory with
+   endianess BYTE_ORDER.  */

+void
+copy_integer_to_size (gdb_byte *dest, int dest_size, const gdb_byte *source,
+		      int source_size, bool is_signed,
+		      enum bfd_endian byte_order)
+{
+  signed int size_diff = dest_size - source_size;
+
+  /* Copy across everything from SOURCE that can fit into DEST.  */
+
+  if (byte_order == BFD_ENDIAN_BIG && size_diff > 0)
+    memcpy (dest + size_diff, source, source_size);
+  else if (byte_order == BFD_ENDIAN_BIG && size_diff < 0)
+    memcpy (dest, source - size_diff, dest_size);
+  else
+    memcpy (dest, source, std::min (source_size, dest_size));
+
+  /* Fill the remaining space in DEST by either zero extending or sign
+     extending.  */
+
+  if (size_diff > 0)
+    {
+      gdb_byte extension = 0;
+      if (is_signed
+	  && ((byte_order != BFD_ENDIAN_BIG && source[source_size - 1] & 0x80)
+	      || (byte_order == BFD_ENDIAN_BIG && source[0] & 0x80)))
+	extension = 0xff;
+
+      /* Extend into MSBs of SOURCE.  */
+      if (byte_order == BFD_ENDIAN_BIG)
+	memset (dest, extension, size_diff);
+      else
+	memset (dest + source_size, extension, size_diff);
+    }
+}

 /* Return a `value' with the contents of (virtual or cooked) register
    REGNUM as found in the specified FRAME.  The register's type is
@@ -1005,3 +1045,89 @@ address_from_register (int regnum, struct frame_info *frame)
   return result;
 }

+#if GDB_SELF_TEST
+namespace selftests {
+namespace findvar_tests {
+
+/* Function to test copy_integer_to_size.  Store SOURCE_VAL with size
+   SOURCE_SIZE to a buffer, making sure no sign extending happens at this
+   stage.  Copy buffer to a new buffer using copy_integer_to_size.  Extract
+   copied value and compare to DEST_VALU.  Copy again with a signed
+   copy_integer_to_size and compare to DEST_VALS.  Do everything for both
+   LITTLE and BIG target endians.  Use unsigned values throughout to make
+   sure there are no implicit sign extensions.  */
+
+static void
+do_cint_test (ULONGEST dest_valu, ULONGEST dest_vals, int dest_size,
+	      ULONGEST src_val, int src_size)
+{
+  for (int i = 0; i < 2 ; i++)
+    {
+      gdb_byte srcbuf[sizeof (ULONGEST)] = {};
+      gdb_byte destbuf[sizeof (ULONGEST)] = {};
+      enum bfd_endian byte_order = i ? BFD_ENDIAN_BIG : BFD_ENDIAN_LITTLE;
+
+      /* Fill the src buffer (and later the dest buffer) with non-zero junk,
+	 to ensure zero extensions aren't hidden.  */
+      memset (srcbuf, 0xaa, sizeof (srcbuf));
+
+      /* Store (and later extract) using unsigned to ensure there are no sign
+	 extensions.  */
+      store_unsigned_integer (srcbuf, src_size, byte_order, src_val);
+
+      /* Test unsigned.  */
+      memset (destbuf, 0xaa, sizeof (destbuf));
+      copy_integer_to_size (destbuf, dest_size, srcbuf, src_size, false,
+			    byte_order);
+      SELF_CHECK (dest_valu == extract_unsigned_integer (destbuf, dest_size,
+							 byte_order));
+
+      /* Test signed.  */
+      memset (destbuf, 0xaa, sizeof (destbuf));
+      copy_integer_to_size (destbuf, dest_size, srcbuf, src_size, true,
+			    byte_order);
+      SELF_CHECK (dest_vals == extract_unsigned_integer (destbuf, dest_size,
+							 byte_order));
+    }
+}
+
+static void
+copy_integer_to_size_test ()
+{
+  /* Destination is bigger than the source, which has the signed bit unset.  */
+  do_cint_test (0x12345678, 0x12345678, 8, 0x12345678, 4);
+  do_cint_test (0x345678, 0x345678, 8, 0x12345678, 3);
+  do_cint_test (0x5678, 0x5678, 2, 0x12345678, 3);
+
+  /* Destination is bigger than the source, which has the signed bit set.  */
+  do_cint_test (0xdeadbeef, 0xffffffffdeadbeef, 8, 0xdeadbeef, 4);
+  do_cint_test (0xadbeef, 0xffffffffffadbeef, 8, 0xdeadbeef, 3);
+  do_cint_test (0xbeef, 0xbeef, 2, 0xdeadbeef, 3);
+
+  /* Destination and source are the same size.  */
+  do_cint_test (0x8765432112345678, 0x8765432112345678, 8, 0x8765432112345678,
+		8);
+  do_cint_test (0x432112345678, 0x432112345678, 6, 0x8765432112345678, 6);
+  do_cint_test (0xfeedbeaddeadbeef, 0xfeedbeaddeadbeef, 8, 0xfeedbeaddeadbeef,
+		8);
+  do_cint_test (0xbeaddeadbeef, 0xbeaddeadbeef, 6, 0xfeedbeaddeadbeef, 6);
+
+  /* Destination is bigger than the source.  Source is bigger than 32bits.  */
+  do_cint_test (0x3412345678, 0x3412345678, 8, 0x3412345678, 6);
+  do_cint_test (0xff12345678, 0xff12345678, 8, 0xff12345678, 6);
+  do_cint_test (0x432112345678, 0x432112345678, 8, 0x8765432112345678, 6);
+  do_cint_test (0xff2112345678, 0xffffff2112345678, 8, 0xffffff2112345678, 6);
+}
+
+} // namespace findvar_test
+} // namespace selftests
+
+#endif
+
+void
+_initialize_findvar (void)
+{
+#if GDB_SELF_TEST
+  register_self_test (selftests::findvar_tests::copy_integer_to_size_test);
+#endif
+}
diff --git a/gdb/mips-fbsd-tdep.c b/gdb/mips-fbsd-tdep.c
index 00fae0ec60ddc9e645d3236efe29f2f9e9ceab5c..13cf98585f96f1acfe6decbe320530d609bee646 100644
--- a/gdb/mips-fbsd-tdep.c
+++ b/gdb/mips-fbsd-tdep.c
@@ -47,57 +47,24 @@
    34th is a dummy for padding.  */
 #define MIPS_FBSD_NUM_FPREGS	34

-/* Supply a single register.  If the source register size matches the
-   size the regcache expects, this can use regcache_raw_supply().  If
-   they are different, this copies the source register into a buffer
-   that can be passed to regcache_raw_supply().  */
+/* Supply a single register.  The register size might not match, so use
+   regcache->raw_supply_integer ().  */

 static void
 mips_fbsd_supply_reg (struct regcache *regcache, int regnum, const void *addr,
 		      size_t len)
 {
-  struct gdbarch *gdbarch = get_regcache_arch (regcache);
-
-  if (register_size (gdbarch, regnum) == len)
-    regcache_raw_supply (regcache, regnum, addr);
-  else
-    {
-      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      val = extract_signed_integer ((const gdb_byte *) addr, len, byte_order);
-      store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
-			    val);
-      regcache_raw_supply (regcache, regnum, buf);
-    }
+  regcache->raw_supply_integer (regnum, (const gdb_byte *) addr, len, true);
 }

-/* Collect a single register.  If the destination register size
-   matches the size the regcache expects, this can use
-   regcache_raw_supply().  If they are different, this fetches the
-   register via regcache_raw_supply() into a buffer and then copies it
-   into the final destination.  */
+/* Collect a single register.  The register size might not match, so use
+   regcache->raw_collect_integer ().  */

 static void
 mips_fbsd_collect_reg (const struct regcache *regcache, int regnum, void *addr,
 		       size_t len)
 {
-  struct gdbarch *gdbarch = get_regcache_arch (regcache);
-
-  if (register_size (gdbarch, regnum) == len)
-    regcache_raw_collect (regcache, regnum, addr);
-  else
-    {
-      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regnum, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regnum),
-				    byte_order);
-      store_signed_integer ((gdb_byte *) addr, len, byte_order, val);
-    }
+  regcache->raw_collect_integer (regnum, (gdb_byte *) addr, len, true);
 }

 /* Supply the floating-point registers stored in FPREGS to REGCACHE.
diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
index 48a582a16c934abe6e8f87c46a6009649c606d49..ccfdcdf98bc1e528cd768efaaaffaa3405708f71 100644
--- a/gdb/mips-linux-tdep.c
+++ b/gdb/mips-linux-tdep.c
@@ -116,13 +116,7 @@ mips_linux_get_longjmp_target (struct frame_info *frame, CORE_ADDR *pc)
 static void
 supply_32bit_reg (struct regcache *regcache, int regnum, const void *addr)
 {
-  struct gdbarch *gdbarch = get_regcache_arch (regcache);
-  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-  gdb_byte buf[MAX_REGISTER_SIZE];
-  store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
-			extract_signed_integer ((const gdb_byte *) addr, 4,
-						byte_order));
-  regcache_raw_supply (regcache, regnum, buf);
+  regcache->raw_supply_integer (regnum, (const gdb_byte *) addr, 4, true);
 }

 /* Unpack an elf_gregset_t into GDB's register cache.  */
@@ -417,7 +411,6 @@ mips64_fill_gregset (const struct regcache *regcache,
 		     mips64_elf_gregset_t *gregsetp, int regno)
 {
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
-  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   int regaddr, regi;
   mips64_elf_greg_t *regp = *gregsetp;
   void *dst;
@@ -460,14 +453,8 @@ mips64_fill_gregset (const struct regcache *regcache,

   if (regaddr != -1)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
       dst = regp + regaddr;
-      store_signed_integer ((gdb_byte *) dst, 8, byte_order, val);
+      regcache->raw_collect_integer (regno, (gdb_byte *) dst, 8, true);
     }
 }

@@ -564,25 +551,13 @@ mips64_fill_fpregset (const struct regcache *regcache,
     }
   else if (regno == mips_regnum (gdbarch)->fp_control_status)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
       to = (gdb_byte *) (*fpregsetp + 32);
-      store_signed_integer (to, 4, byte_order, val);
+      regcache->raw_collect_integer (regno, to, 4, true);
     }
   else if (regno == mips_regnum (gdbarch)->fp_implementation_revision)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
       to = (gdb_byte *) (*fpregsetp + 32) + 4;
-      store_signed_integer (to, 4, byte_order, val);
+      regcache->raw_collect_integer (regno, to, 4, true);
     }
   else if (regno == -1)
     {
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 4dcfccbac70f0f962bf5e5596d035fda42322795..409482d17c0542c7a53620d88d33fa9706fa72c5 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -294,8 +294,14 @@ public:

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

+  void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
+			    bool is_signed) const;
+
   void raw_supply (int regnum, const void *buf);

+  void raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len,
+			   bool is_signed);
+
   void raw_supply_zeroed (int regnum);

   void raw_copy (int regnum, struct regcache *src_regcache);
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 660558f7ff10f9d8346b6e08422e16c38c3c4d7d..90462aea73ba810bd1af9ddb8c6d02e1f0e95991 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1189,6 +1189,31 @@ 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.  */
+
+void
+regcache::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;
+  size_t regsize;
+
+  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
+  gdb_assert (!m_readonly_p);
+
+  regbuf = register_buffer (regnum);
+  regsize = m_descr->sizeof_register[regnum];
+
+  copy_integer_to_size (regbuf, regsize, addr, addr_len, is_signed,
+			byte_order);
+  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).  */
@@ -1232,6 +1257,29 @@ regcache::raw_collect (int regnum, void *buf) const
   memcpy (buf, regbuf, size);
 }

+/* 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
+regcache::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;
+  size_t regsize;
+
+  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
+
+  regbuf = register_buffer (regnum);
+  regsize = m_descr->sizeof_register[regnum];
+
+  copy_integer_to_size (addr, addr_len, regbuf, regsize, is_signed,
+			byte_order);
+}
+
 void
 regcache::raw_copy (int regnum, struct regcache *src_regcache)
 {

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Pedro Alves]

On 05/26/2017 04:30 PM, Alan Hayward wrote:
> 
>> On 26 May 2017, at 11:26, Pedro Alves <palves@redhat.com> wrote:
> 
> 
>> Another issue with the test is that by relying on
>> extract_(un)signed_integer, the test can't notice if the
>> zero / sign extension in copy_integer_bytes filled the right
>> number of bytes:
>>
>>> +  do_cint_test (0xffffffffffffbeef, 2, 0xbeef, 3);
>>
>> I.e., above, the last test _must_ not write more than
>> two bytes to the destination, so those leading
>> 0xFFs before "beef" are artificial.
>>
>> Passing ULONGEST as destination value avoids that problem, making
>> you write that last test as:
>>
>>   do_cint_test (0x000000000000beef, 2, 0xbeef, 3);
>>
> 
> To make that work, I also had to make extract use the unsigned
> version. So now all the stores and extracts are working on unsigned data.

Makes sense.
> All the above fixed up.
> After doing this, I noticed it was easier to combine the two test functions
> into one.

Agreed.

> 
> 
> Tested on a --enable-targets=all build using make check with board files
> unix and native-gdbserver.
> I do not have a MIPS machine to test on.

I think the unit tests now give us sufficient assurance.

> Ok to commit?

Yes, with nit below addressed.

> +static void
> +copy_integer_to_size_test ()
> +{
> +  /* Destination is bigger than the source, which has the signed bit unset.  */
> +  do_cint_test (0x12345678, 0x12345678, 8, 0x12345678, 4);
> +  do_cint_test (0x345678, 0x345678, 8, 0x12345678, 3);
> +  do_cint_test (0x5678, 0x5678, 2, 0x12345678, 3);

The third test does not agree with the comment - destination is
smaller than source.

> +
> +  /* Destination is bigger than the source, which has the signed bit set.  */
> +  do_cint_test (0xdeadbeef, 0xffffffffdeadbeef, 8, 0xdeadbeef, 4);
> +  do_cint_test (0xadbeef, 0xffffffffffadbeef, 8, 0xdeadbeef, 3);
> +  do_cint_test (0xbeef, 0xbeef, 2, 0xdeadbeef, 3);

Ditto.

Re-sort to a separate block, and/or update comments, and this
is good to go.

Thanks,
Pedro Alves

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[John Baldwin]

On Friday, May 26, 2017 03:30:44 PM Alan Hayward wrote:
> 
> > On 26 May 2017, at 11:26, Pedro Alves <palves@redhat.com> wrote:
> 
> 
> > Another issue with the test is that by relying on
> > extract_(un)signed_integer, the test can't notice if the
> > zero / sign extension in copy_integer_bytes filled the right
> > number of bytes:
> > 
> >> +  do_cint_test (0xffffffffffffbeef, 2, 0xbeef, 3);
> > 
> > I.e., above, the last test _must_ not write more than
> > two bytes to the destination, so those leading
> > 0xFFs before "beef" are artificial.
> > 
> > Passing ULONGEST as destination value avoids that problem, making
> > you write that last test as:
> > 
> >   do_cint_test (0x000000000000beef, 2, 0xbeef, 3);
> > 
> 
> To make that work, I also had to make extract use the unsigned
> version. So now all the stores and extracts are working on unsigned data.
> 
> > 
> > 
> > So in sum:
> > 
> > #1 - ULONGEST as destination value, both signed and unsigned tests.
> > #2 - Add unsigned tests with size >4
> > #3 - Add tests src_size == dest_size
> > #4 - Use gdb_byte arrays for temporary buffers, and memset them with 0xaa.
> > #5 - Adjust tests to pass.
> > #6 - Add comments where appropriate.
> > 
> 
> All the above fixed up.
> After doing this, I noticed it was easier to combine the two test functions
> into one.
> 
> 
> Tested on a --enable-targets=all build using make check with board files
> unix and native-gdbserver.
> I do not have a MIPS machine to test on.
> Ok to commit?

The mips-fbsd-tdep.c bits look fine to me.  I can run-test them post-commit.
For now I think it's simplest (especially for the current patch) to follow
your current approach of replacing the body of the helper functions in that
file.  I may inline the helpers as a future followup, but I think it's
cleaner if those are separate steps anyway.  Thanks!

-- 
John Baldwin

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (2/4)

[Alan Hayward]

>> Ok to commit?
> 
> Yes, with nit below addressed.
> 
>> +static void
>> +copy_integer_to_size_test ()
>> +{
>> +  /* Destination is bigger than the source, which has the signed bit unset.  */
>> +  do_cint_test (0x12345678, 0x12345678, 8, 0x12345678, 4);
>> +  do_cint_test (0x345678, 0x345678, 8, 0x12345678, 3);
>> +  do_cint_test (0x5678, 0x5678, 2, 0x12345678, 3);
> 
> The third test does not agree with the comment - destination is
> smaller than source.
> 
>> +
>> +  /* Destination is bigger than the source, which has the signed bit set.  */
>> +  do_cint_test (0xdeadbeef, 0xffffffffdeadbeef, 8, 0xdeadbeef, 4);
>> +  do_cint_test (0xadbeef, 0xffffffffffadbeef, 8, 0xdeadbeef, 3);
>> +  do_cint_test (0xbeef, 0xbeef, 2, 0xdeadbeef, 3);
> 
> Ditto.
> 
> Re-sort to a separate block, and/or update comments, and this
> is good to go.
> 
> 

Thanks for the reviews!


> 
> On 26 May 2017, at 16:52, John Baldwin <jhb@freebsd.org> wrote:
> 
> The mips-fbsd-tdep.c bits look fine to me.  I can run-test them post-commit.
> For now I think it's simplest (especially for the current patch) to follow
> your current approach of replacing the body of the helper functions in that
> file.  I may inline the helpers as a future followup, but I think it's
> cleaner if those are separate steps anyway.  Thanks!
> 

And thanks also!


For reference, pushed the following (plus changelog):



diff --git a/gdb/defs.h b/gdb/defs.h
index a0b586f401eca205334e9f237081f4da97c83aa1..a1a97bb1e791d4f423788797d1f04c3e89877d90 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -658,7 +658,10 @@ extern void store_unsigned_integer (gdb_byte *, int,
 extern void store_typed_address (gdb_byte *buf, struct type *type,
 				 CORE_ADDR addr);

-

+extern void copy_integer_to_size (gdb_byte *dest, int dest_size,
+				  const gdb_byte *source, int source_size,
+				  bool is_signed, enum bfd_endian byte_order);
+
 /* From valops.c */

 extern int watchdog;
diff --git a/gdb/findvar.c b/gdb/findvar.c
index ed4d5c1266c9de069981b306bc8229ae5fb02350..6c18e25fe2e28214de5acf571ebb3e45bf694d95 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -33,6 +33,7 @@
 #include "objfiles.h"
 #include "language.h"
 #include "dwarf2loc.h"
+#include "selftest.h"

 /* Basic byte-swapping routines.  All 'extract' functions return a
    host-format integer from a target-format integer at ADDR which is
@@ -249,7 +250,46 @@ store_typed_address (gdb_byte *buf, struct type *type, CORE_ADDR addr)
   gdbarch_address_to_pointer (get_type_arch (type), type, buf, addr);
 }

+/* Copy a value from SOURCE of size SOURCE_SIZE bytes to DEST of size DEST_SIZE
+   bytes.  If SOURCE_SIZE is greater than DEST_SIZE, then truncate the most
+   significant bytes.  If SOURCE_SIZE is less than DEST_SIZE then either sign
+   or zero extended according to IS_SIGNED.  Values are stored in memory with
+   endianess BYTE_ORDER.  */

+void
+copy_integer_to_size (gdb_byte *dest, int dest_size, const gdb_byte *source,
+		      int source_size, bool is_signed,
+		      enum bfd_endian byte_order)
+{
+  signed int size_diff = dest_size - source_size;
+
+  /* Copy across everything from SOURCE that can fit into DEST.  */
+
+  if (byte_order == BFD_ENDIAN_BIG && size_diff > 0)
+    memcpy (dest + size_diff, source, source_size);
+  else if (byte_order == BFD_ENDIAN_BIG && size_diff < 0)
+    memcpy (dest, source - size_diff, dest_size);
+  else
+    memcpy (dest, source, std::min (source_size, dest_size));
+
+  /* Fill the remaining space in DEST by either zero extending or sign
+     extending.  */
+
+  if (size_diff > 0)
+    {
+      gdb_byte extension = 0;
+      if (is_signed
+	  && ((byte_order != BFD_ENDIAN_BIG && source[source_size - 1] & 0x80)
+	      || (byte_order == BFD_ENDIAN_BIG && source[0] & 0x80)))
+	extension = 0xff;
+
+      /* Extend into MSBs of SOURCE.  */
+      if (byte_order == BFD_ENDIAN_BIG)
+	memset (dest, extension, size_diff);
+      else
+	memset (dest + source_size, extension, size_diff);
+    }
+}

 /* Return a `value' with the contents of (virtual or cooked) register
    REGNUM as found in the specified FRAME.  The register's type is
@@ -1005,3 +1045,91 @@ address_from_register (int regnum, struct frame_info *frame)
   return result;
 }

+#if GDB_SELF_TEST
+namespace selftests {
+namespace findvar_tests {
+
+/* Function to test copy_integer_to_size.  Store SOURCE_VAL with size
+   SOURCE_SIZE to a buffer, making sure no sign extending happens at this
+   stage.  Copy buffer to a new buffer using copy_integer_to_size.  Extract
+   copied value and compare to DEST_VALU.  Copy again with a signed
+   copy_integer_to_size and compare to DEST_VALS.  Do everything for both
+   LITTLE and BIG target endians.  Use unsigned values throughout to make
+   sure there are no implicit sign extensions.  */
+
+static void
+do_cint_test (ULONGEST dest_valu, ULONGEST dest_vals, int dest_size,
+	      ULONGEST src_val, int src_size)
+{
+  for (int i = 0; i < 2 ; i++)
+    {
+      gdb_byte srcbuf[sizeof (ULONGEST)] = {};
+      gdb_byte destbuf[sizeof (ULONGEST)] = {};
+      enum bfd_endian byte_order = i ? BFD_ENDIAN_BIG : BFD_ENDIAN_LITTLE;
+
+      /* Fill the src buffer (and later the dest buffer) with non-zero junk,
+	 to ensure zero extensions aren't hidden.  */
+      memset (srcbuf, 0xaa, sizeof (srcbuf));
+
+      /* Store (and later extract) using unsigned to ensure there are no sign
+	 extensions.  */
+      store_unsigned_integer (srcbuf, src_size, byte_order, src_val);
+
+      /* Test unsigned.  */
+      memset (destbuf, 0xaa, sizeof (destbuf));
+      copy_integer_to_size (destbuf, dest_size, srcbuf, src_size, false,
+			    byte_order);
+      SELF_CHECK (dest_valu == extract_unsigned_integer (destbuf, dest_size,
+							 byte_order));
+
+      /* Test signed.  */
+      memset (destbuf, 0xaa, sizeof (destbuf));
+      copy_integer_to_size (destbuf, dest_size, srcbuf, src_size, true,
+			    byte_order);
+      SELF_CHECK (dest_vals == extract_unsigned_integer (destbuf, dest_size,
+							 byte_order));
+    }
+}
+
+static void
+copy_integer_to_size_test ()
+{
+  /* Destination is bigger than the source, which has the signed bit unset.  */
+  do_cint_test (0x12345678, 0x12345678, 8, 0x12345678, 4);
+  do_cint_test (0x345678, 0x345678, 8, 0x12345678, 3);
+
+  /* Destination is bigger than the source, which has the signed bit set.  */
+  do_cint_test (0xdeadbeef, 0xffffffffdeadbeef, 8, 0xdeadbeef, 4);
+  do_cint_test (0xadbeef, 0xffffffffffadbeef, 8, 0xdeadbeef, 3);
+
+  /* Destination is smaller than the source.  */
+  do_cint_test (0x5678, 0x5678, 2, 0x12345678, 3);
+  do_cint_test (0xbeef, 0xbeef, 2, 0xdeadbeef, 3);
+
+  /* Destination and source are the same size.  */
+  do_cint_test (0x8765432112345678, 0x8765432112345678, 8, 0x8765432112345678,
+		8);
+  do_cint_test (0x432112345678, 0x432112345678, 6, 0x8765432112345678, 6);
+  do_cint_test (0xfeedbeaddeadbeef, 0xfeedbeaddeadbeef, 8, 0xfeedbeaddeadbeef,
+		8);
+  do_cint_test (0xbeaddeadbeef, 0xbeaddeadbeef, 6, 0xfeedbeaddeadbeef, 6);
+
+  /* Destination is bigger than the source.  Source is bigger than 32bits.  */
+  do_cint_test (0x3412345678, 0x3412345678, 8, 0x3412345678, 6);
+  do_cint_test (0xff12345678, 0xff12345678, 8, 0xff12345678, 6);
+  do_cint_test (0x432112345678, 0x432112345678, 8, 0x8765432112345678, 6);
+  do_cint_test (0xff2112345678, 0xffffff2112345678, 8, 0xffffff2112345678, 6);
+}
+
+} // namespace findvar_test
+} // namespace selftests
+
+#endif
+
+void
+_initialize_findvar (void)
+{
+#if GDB_SELF_TEST
+  register_self_test (selftests::findvar_tests::copy_integer_to_size_test);
+#endif
+}
diff --git a/gdb/mips-fbsd-tdep.c b/gdb/mips-fbsd-tdep.c
index 00fae0ec60ddc9e645d3236efe29f2f9e9ceab5c..13cf98585f96f1acfe6decbe320530d609bee646 100644
--- a/gdb/mips-fbsd-tdep.c
+++ b/gdb/mips-fbsd-tdep.c
@@ -47,57 +47,24 @@
    34th is a dummy for padding.  */
 #define MIPS_FBSD_NUM_FPREGS	34

-/* Supply a single register.  If the source register size matches the
-   size the regcache expects, this can use regcache_raw_supply().  If
-   they are different, this copies the source register into a buffer
-   that can be passed to regcache_raw_supply().  */
+/* Supply a single register.  The register size might not match, so use
+   regcache->raw_supply_integer ().  */

 static void
 mips_fbsd_supply_reg (struct regcache *regcache, int regnum, const void *addr,
 		      size_t len)
 {
-  struct gdbarch *gdbarch = get_regcache_arch (regcache);
-
-  if (register_size (gdbarch, regnum) == len)
-    regcache_raw_supply (regcache, regnum, addr);
-  else
-    {
-      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      val = extract_signed_integer ((const gdb_byte *) addr, len, byte_order);
-      store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
-			    val);
-      regcache_raw_supply (regcache, regnum, buf);
-    }
+  regcache->raw_supply_integer (regnum, (const gdb_byte *) addr, len, true);
 }

-/* Collect a single register.  If the destination register size
-   matches the size the regcache expects, this can use
-   regcache_raw_supply().  If they are different, this fetches the
-   register via regcache_raw_supply() into a buffer and then copies it
-   into the final destination.  */
+/* Collect a single register.  The register size might not match, so use
+   regcache->raw_collect_integer ().  */

 static void
 mips_fbsd_collect_reg (const struct regcache *regcache, int regnum, void *addr,
 		       size_t len)
 {
-  struct gdbarch *gdbarch = get_regcache_arch (regcache);
-
-  if (register_size (gdbarch, regnum) == len)
-    regcache_raw_collect (regcache, regnum, addr);
-  else
-    {
-      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regnum, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regnum),
-				    byte_order);
-      store_signed_integer ((gdb_byte *) addr, len, byte_order, val);
-    }
+  regcache->raw_collect_integer (regnum, (gdb_byte *) addr, len, true);
 }

 /* Supply the floating-point registers stored in FPREGS to REGCACHE.
diff --git a/gdb/mips-linux-tdep.c b/gdb/mips-linux-tdep.c
index 48a582a16c934abe6e8f87c46a6009649c606d49..ccfdcdf98bc1e528cd768efaaaffaa3405708f71 100644
--- a/gdb/mips-linux-tdep.c
+++ b/gdb/mips-linux-tdep.c
@@ -116,13 +116,7 @@ mips_linux_get_longjmp_target (struct frame_info *frame, CORE_ADDR *pc)
 static void
 supply_32bit_reg (struct regcache *regcache, int regnum, const void *addr)
 {
-  struct gdbarch *gdbarch = get_regcache_arch (regcache);
-  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-  gdb_byte buf[MAX_REGISTER_SIZE];
-  store_signed_integer (buf, register_size (gdbarch, regnum), byte_order,
-			extract_signed_integer ((const gdb_byte *) addr, 4,
-						byte_order));
-  regcache_raw_supply (regcache, regnum, buf);
+  regcache->raw_supply_integer (regnum, (const gdb_byte *) addr, 4, true);
 }

 /* Unpack an elf_gregset_t into GDB's register cache.  */
@@ -417,7 +411,6 @@ mips64_fill_gregset (const struct regcache *regcache,
 		     mips64_elf_gregset_t *gregsetp, int regno)
 {
   struct gdbarch *gdbarch = get_regcache_arch (regcache);
-  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   int regaddr, regi;
   mips64_elf_greg_t *regp = *gregsetp;
   void *dst;
@@ -460,14 +453,8 @@ mips64_fill_gregset (const struct regcache *regcache,

   if (regaddr != -1)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
       dst = regp + regaddr;
-      store_signed_integer ((gdb_byte *) dst, 8, byte_order, val);
+      regcache->raw_collect_integer (regno, (gdb_byte *) dst, 8, true);
     }
 }

@@ -564,25 +551,13 @@ mips64_fill_fpregset (const struct regcache *regcache,
     }
   else if (regno == mips_regnum (gdbarch)->fp_control_status)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
       to = (gdb_byte *) (*fpregsetp + 32);
-      store_signed_integer (to, 4, byte_order, val);
+      regcache->raw_collect_integer (regno, to, 4, true);
     }
   else if (regno == mips_regnum (gdbarch)->fp_implementation_revision)
     {
-      gdb_byte buf[MAX_REGISTER_SIZE];
-      LONGEST val;
-
-      regcache_raw_collect (regcache, regno, buf);
-      val = extract_signed_integer (buf, register_size (gdbarch, regno),
-				    byte_order);
       to = (gdb_byte *) (*fpregsetp + 32) + 4;
-      store_signed_integer (to, 4, byte_order, val);
+      regcache->raw_collect_integer (regno, to, 4, true);
     }
   else if (regno == -1)
     {
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 4dcfccbac70f0f962bf5e5596d035fda42322795..409482d17c0542c7a53620d88d33fa9706fa72c5 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -294,8 +294,14 @@ public:

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

+  void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
+			    bool is_signed) const;
+
   void raw_supply (int regnum, const void *buf);

+  void raw_supply_integer (int regnum, const gdb_byte *addr, int addr_len,
+			   bool is_signed);
+
   void raw_supply_zeroed (int regnum);

   void raw_copy (int regnum, struct regcache *src_regcache);
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 660558f7ff10f9d8346b6e08422e16c38c3c4d7d..90462aea73ba810bd1af9ddb8c6d02e1f0e95991 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1189,6 +1189,31 @@ 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.  */
+
+void
+regcache::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;
+  size_t regsize;
+
+  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
+  gdb_assert (!m_readonly_p);
+
+  regbuf = register_buffer (regnum);
+  regsize = m_descr->sizeof_register[regnum];
+
+  copy_integer_to_size (regbuf, regsize, addr, addr_len, is_signed,
+			byte_order);
+  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).  */
@@ -1232,6 +1257,29 @@ regcache::raw_collect (int regnum, void *buf) const
   memcpy (buf, regbuf, size);
 }

+/* 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
+regcache::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;
+  size_t regsize;
+
+  gdb_assert (regnum >= 0 && regnum < m_descr->nr_raw_registers);
+
+  regbuf = register_buffer (regnum);
+  regsize = m_descr->sizeof_register[regnum];
+
+  copy_integer_to_size (addr, addr_len, regbuf, regsize, is_signed,
+			byte_order);
+}
+
 void
 regcache::raw_copy (int regnum, struct regcache *src_regcache)
 {

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (4/4)

[Alan Hayward]

> On 5 May 2017, at 09:04, Alan Hayward <Alan.Hayward@arm.com> wrote:
> 
> 
>> On 11 Apr 2017, at 16:37, Yao Qi <qiyaoltc@gmail.com> wrote:
>> 
>> Alan Hayward <Alan.Hayward@arm.com> writes:
>> 


I’ve rebased this patch due to Yao’s unit test changes.

I don't have a MIPS machine to test on.
Tested on a --enable-targets=all and asan build using
make check with board files unix, native-gdbserver and unittest


Ok to commit?

Alan.

2017-05-30 Alan Hayward  <alan.hayward@arm.com>

	* mips-tdep.c (mips_eabi_push_dummy_call): Hard code buffer size.


diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 82f91ba2cd950c5f48f8f408f645ea49e952ef29..52d2ca134f8d14f54c6f4e450c84597c4d6a0e0e 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -4528,7 +4528,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
   for (argnum = 0; argnum < nargs; argnum++)
     {
       const gdb_byte *val;
-      gdb_byte valbuf[MAX_REGISTER_SIZE];
+      gdb_byte valbuf[8];
       struct value *arg = args[argnum];
       struct type *arg_type = check_typedef (value_type (arg));
       int len = TYPE_LENGTH (arg_type);
@@ -4544,6 +4544,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
       if (len > regsize
 	  && (typecode == TYPE_CODE_STRUCT || typecode == TYPE_CODE_UNION))
 	{
+	  gdb_assert (regsize <= 8);
 	  store_unsigned_integer (valbuf, regsize, byte_order,
 				  value_address (arg));
 	  typecode = TYPE_CODE_PTR;

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (4/4)

[Maciej W. Rozycki]

On Wed, 7 Jun 2017, Alan Hayward wrote:

> I don't have a MIPS machine to test on.

 I could schedule a test run over the coming weekend, however your change 
applies to EABI support, which I believe is long dead (as in: nobody uses 
it).  Consequently I don't have a way to test with an EABI target either.

> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
> index 82f91ba2cd950c5f48f8f408f645ea49e952ef29..52d2ca134f8d14f54c6f4e450c84597c4d6a0e0e 100644
> --- a/gdb/mips-tdep.c
> +++ b/gdb/mips-tdep.c
> @@ -4528,7 +4528,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>    for (argnum = 0; argnum < nargs; argnum++)
>      {
>        const gdb_byte *val;
> -      gdb_byte valbuf[MAX_REGISTER_SIZE];
> +      gdb_byte valbuf[8];
>        struct value *arg = args[argnum];
>        struct type *arg_type = check_typedef (value_type (arg));
>        int len = TYPE_LENGTH (arg_type);

 If you need to get rid of MAX_REGISTER_SIZE here (what was reason 
again?), then why not simply use `regsize' instead as the array size?

 AFAICS `valbuf' is only written once, with the size of data requested 
specified exactly as `regsize'.  The only explanation I have been able to 
come up with as to why MAX_REGISTER_SIZE has been chosen for `valbuf' 
allocation is it was made before we could assume variable-length automatic 
array support.

> @@ -4544,6 +4544,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>        if (len > regsize
>  	  && (typecode == TYPE_CODE_STRUCT || typecode == TYPE_CODE_UNION))
>  	{
> +	  gdb_assert (regsize <= 8);
>  	  store_unsigned_integer (valbuf, regsize, byte_order,
>  				  value_address (arg));
>  	  typecode = TYPE_CODE_PTR;

 You obviously don't need the assertion then.

  Maciej

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (4/4)

[Alan Hayward]

> On 8 Jun 2017, at 21:26, Maciej W. Rozycki <macro@imgtec.com> wrote:
> 
> On Wed, 7 Jun 2017, Alan Hayward wrote:
> 
>> I don't have a MIPS machine to test on.
> 
> I could schedule a test run over the coming weekend, however your change 
> applies to EABI support, which I believe is long dead (as in: nobody uses 
> it).  Consequently I don't have a way to test with an EABI target either.

Thanks for the offer, but sounds like the only real option then is to just
ensure the change is safe enough to not break.

> 
>> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
>> index 82f91ba2cd950c5f48f8f408f645ea49e952ef29..52d2ca134f8d14f54c6f4e450c84597c4d6a0e0e 100644
>> --- a/gdb/mips-tdep.c
>> +++ b/gdb/mips-tdep.c
>> @@ -4528,7 +4528,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>>   for (argnum = 0; argnum < nargs; argnum++)
>>     {
>>       const gdb_byte *val;
>> -      gdb_byte valbuf[MAX_REGISTER_SIZE];
>> +      gdb_byte valbuf[8];
>>       struct value *arg = args[argnum];
>>       struct type *arg_type = check_typedef (value_type (arg));
>>       int len = TYPE_LENGTH (arg_type);
> 
> If you need to get rid of MAX_REGISTER_SIZE here (what was reason 
> again?), then why not simply use `regsize' instead as the array size?
> 
> AFAICS `valbuf' is only written once, with the size of data requested 
> specified exactly as `regsize'.  The only explanation I have been able to 
> come up with as to why MAX_REGISTER_SIZE has been chosen for `valbuf' 
> allocation is it was made before we could assume variable-length automatic 
> array support.

The reason for getting rid of MAX_REGISTER_SIZE is that Aarch64 SVE will
require increasing the size massively, with potential for future increases.
Hence the large number of patches that are gradually removing MAX_REGISTER_SIZE.

I’ve avoided using variable-length arrays because it has the potential to
break the stack.
However, here we know that we aren’t going to get a value >8, so maybe in
this case a VLA would be ok?

Anyone else have an opinion here?


Alan.

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (4/4)

[Pedro Alves]

On 06/09/2017 11:31 AM, Alan Hayward wrote:

> I’ve avoided using variable-length arrays because it has the potential to
> break the stack.
> However, here we know that we aren’t going to get a value >8, so maybe in
> this case a VLA would be ok?
> 
> Anyone else have an opinion here?

VLAs are not standard C++, unfortunately.  Do we know whether all compilers
we care about support them?  It doesn't seem worth it to me to rely
on compiler extensions when we know we're always going to see a size <=8.

Thanks,
Pedro Alves

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (4/4)

[Maciej W. Rozycki]

On Fri, 9 Jun 2017, Pedro Alves wrote:

> > I’ve avoided using variable-length arrays because it has the potential to
> > break the stack.
> > However, here we know that we aren’t going to get a value >8, so maybe in
> > this case a VLA would be ok?
> > 
> > Anyone else have an opinion here?
> 
> VLAs are not standard C++, unfortunately.  Do we know whether all compilers
> we care about support them?  It doesn't seem worth it to me to rely
> on compiler extensions when we know we're always going to see a size <=8.

 Hmm, `alloca' then?  It used to be used here actually, up till commit 
d9d9c31f3130 ("MAX_REGISTER_RAW_SIZE -> MAX_REGISTER_SIZE"), 
<https://sourceware.org/ml/gdb-patches/2003-05/msg00127.html>.

  Maciej

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (4/4)

[Pedro Alves]

On 06/09/2017 12:48 PM, Maciej W. Rozycki wrote:
> On Fri, 9 Jun 2017, Pedro Alves wrote:
> 
>>> I’ve avoided using variable-length arrays because it has the potential to
>>> break the stack.
>>> However, here we know that we aren’t going to get a value >8, so maybe in
>>> this case a VLA would be ok?
>>>
>>> Anyone else have an opinion here?
>>
>> VLAs are not standard C++, unfortunately.  Do we know whether all compilers
>> we care about support them?  It doesn't seem worth it to me to rely
>> on compiler extensions when we know we're always going to see a size <=8.
> 
>  Hmm, `alloca' then?  It used to be used here actually, up till commit 
> d9d9c31f3130 ("MAX_REGISTER_RAW_SIZE -> MAX_REGISTER_SIZE"), 
> <https://sourceware.org/ml/gdb-patches/2003-05/msg00127.html>.

IMO, alloca calls are a red flag, which force you to reason about
whether you're dangerously calling it in a loop such as in
this case, because alloca memory is unwound on function exit, not
scope exit.    This is both a concern when an alloca call is
introduced, and later when code is moved around/refactored.  If we know
the hard upper bound, and it's just 8 bytes, I don't see the point
of making it variable.  Alignment and padding for following
variables plus the magic needed to handle variable length frames end up
negating the saving anyway.  Also, the function already hardcodes "8"
in several places.  IMO, here it'd be better to keep it simple
and use a static upper bound.

Thanks,
Pedro Alves

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (4/4)

[Maciej W. Rozycki]

On Fri, 9 Jun 2017, Pedro Alves wrote:

> >  Hmm, `alloca' then?  It used to be used here actually, up till commit 
> > d9d9c31f3130 ("MAX_REGISTER_RAW_SIZE -> MAX_REGISTER_SIZE"), 
> > <https://sourceware.org/ml/gdb-patches/2003-05/msg00127.html>.
> 
> IMO, alloca calls are a red flag, which force you to reason about
> whether you're dangerously calling it in a loop such as in
> this case, because alloca memory is unwound on function exit, not
> scope exit.    This is both a concern when an alloca call is
> introduced, and later when code is moved around/refactored.  If we know
> the hard upper bound, and it's just 8 bytes, I don't see the point
> of making it variable.  Alignment and padding for following
> variables plus the magic needed to handle variable length frames end up
> negating the saving anyway.  Also, the function already hardcodes "8"
> in several places.  IMO, here it'd be better to keep it simple
> and use a static upper bound.

 Contrariwise `mips_read_fp_register_single' already uses `alloca' for a 
similar purpose.  Good point about the loop though; moving the declaration 
and allocation outside the loop will easily solve the problem however.

 Hardcoding things, and especially with literals, causes a maintenance 
pain when things change.  Bad code elsewhere is not an excuse; besides, 
the other places where `8' is hardcoded are not (buffer) limits, but just 
handle specific register sizes, which are not going to change, and which 
are a completely different matter.  So if you find `alloca' unacceptable, 
then the limit has to be a macro, and an assertion check is also due (as 
already proposed), preferably using `sizeof', so that a future change does 
not break it by accident.

 NB the MSA ASE expands FPRs to 16 bytes and we'll soon have to accomodate 
that; patches have already been posted and are being worked on.

  Maciej

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (4/4)

[Pedro Alves]

On 06/09/2017 02:23 PM, Maciej W. Rozycki wrote:

>  Hardcoding things, and especially with literals, causes a maintenance 
> pain when things change.  Bad code elsewhere is not an excuse; besides, 
> the other places where `8' is hardcoded are not (buffer) limits, but just 
> handle specific register sizes, which are not going to change, and which 
> are a completely different matter. 

Perhaps you won't be surprised to learn that I subscribe to the same general
principles.  However, I believe that this isn't, in fact, a completely
different matter, because the buffer in question is used to hold the
contents of a structure's address, to either put in a general register,
or in a stack slot, and those (addresses and general registers), unlike FPRs,
aren't expected to grow for a given architecture.

> So if you find `alloca' unacceptable, 
> then the limit has to be a macro, and an assertion check is also due (as 
> already proposed), preferably using `sizeof', so that a future change does 
> not break it by accident.

I think a patch like the below is likely to clarify things.
(Builds, but is otherwise untested).

From 5bdf1c939d72bf2718a542164d99bb396f0526e3 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 9 Jun 2017 15:10:50 +0100
Subject: [PATCH] mips

---
 gdb/mips-tdep.c | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 82f91ba..7844c73 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -271,6 +271,9 @@ mips_isa_regsize (struct gdbarch *gdbarch)
 	  / gdbarch_bfd_arch_info (gdbarch)->bits_per_byte);
 }
 
+/* Max saved register size.  */
+#define MAX_MIPS_ABI_REGSIZE 8
+
 /* Return the currently configured (or set) saved register size.  */
 
 unsigned int
@@ -4476,7 +4479,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
   int stack_offset = 0;
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   CORE_ADDR func_addr = find_function_addr (function, NULL);
-  int regsize = mips_abi_regsize (gdbarch);
+  int abi_regsize = mips_abi_regsize (gdbarch);
 
   /* For shared libraries, "t9" needs to point at the function
      address.  */
@@ -4499,7 +4502,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
      than necessary for EABI, because the first few arguments are
      passed in registers, but that's OK.  */
   for (argnum = 0; argnum < nargs; argnum++)
-    len += align_up (TYPE_LENGTH (value_type (args[argnum])), regsize);
+    len += align_up (TYPE_LENGTH (value_type (args[argnum])), abi_regsize);
   sp -= align_up (len, 16);
 
   if (mips_debug)
@@ -4528,7 +4531,9 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
   for (argnum = 0; argnum < nargs; argnum++)
     {
       const gdb_byte *val;
-      gdb_byte valbuf[MAX_REGISTER_SIZE];
+      /* This holds the address of structures that are passed by
+	 reference.  */
+      gdb_byte ref_valbuf[MAX_MIPS_ABI_REGISTER_SIZE];
       struct value *arg = args[argnum];
       struct type *arg_type = check_typedef (value_type (arg));
       int len = TYPE_LENGTH (arg_type);
@@ -4541,13 +4546,14 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 
       /* The EABI passes structures that do not fit in a register by
          reference.  */
-      if (len > regsize
+      if (len > abi_regsize
 	  && (typecode == TYPE_CODE_STRUCT || typecode == TYPE_CODE_UNION))
 	{
-	  store_unsigned_integer (valbuf, regsize, byte_order,
+	  gdb_assert (ARRAY_SIZE (ref_valbuf) >= abi_regsize);
+	  store_unsigned_integer (ref_valbuf, abi_regsize, byte_order,
 				  value_address (arg));
 	  typecode = TYPE_CODE_PTR;
-	  len = regsize;
+	  len = abi_regsize;
 	  val = valbuf;
 	  if (mips_debug)
 	    fprintf_unfiltered (gdb_stdlog, " push");
@@ -4560,7 +4566,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
          up before the check to see if there are any FP registers
          left.  Non MIPS_EABI targets also pass the FP in the integer
          registers so also round up normal registers.  */
-      if (regsize < 8 && fp_register_arg_p (gdbarch, typecode, arg_type))
+      if (abi_regsize < 8 && fp_register_arg_p (gdbarch, typecode, arg_type))
 	{
 	  if ((float_argreg & 1))
 	    float_argreg++;
@@ -4626,12 +4632,12 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 	  /* Copy the argument to general registers or the stack in
 	     register-sized pieces.  Large arguments are split between
 	     registers and stack.  */
-	  /* Note: structs whose size is not a multiple of regsize
+	  /* Note: structs whose size is not a multiple of abi_regsize
 	     are treated specially: Irix cc passes
 	     them in registers where gcc sometimes puts them on the
 	     stack.  For maximum compatibility, we will put them in
 	     both places.  */
-	  int odd_sized_struct = (len > regsize && len % regsize != 0);
+	  int odd_sized_struct = (len > abi_regsize && len % abi_regsize != 0);
 
 	  /* Note: Floating-point values that didn't fit into an FP
 	     register are only written to memory.  */
@@ -4639,7 +4645,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 	    {
 	      /* Remember if the argument was written to the stack.  */
 	      int stack_used_p = 0;
-	      int partial_len = (len < regsize ? len : regsize);
+	      int partial_len = (len < abi_regsize ? len : abi_regsize);
 
 	      if (mips_debug)
 		fprintf_unfiltered (gdb_stdlog, " -- partial=%d",
@@ -4657,15 +4663,15 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 		  stack_used_p = 1;
 		  if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
 		    {
-		      if (regsize == 8
+		      if (abi_regsize == 8
 			  && (typecode == TYPE_CODE_INT
 			      || typecode == TYPE_CODE_PTR
 			      || typecode == TYPE_CODE_FLT) && len <= 4)
-			longword_offset = regsize - len;
+			longword_offset = abi_regsize - len;
 		      else if ((typecode == TYPE_CODE_STRUCT
 				|| typecode == TYPE_CODE_UNION)
-			       && TYPE_LENGTH (arg_type) < regsize)
-			longword_offset = regsize - len;
+			       && TYPE_LENGTH (arg_type) < abi_regsize)
+			longword_offset = abi_regsize - len;
 		    }
 
 		  if (mips_debug)
@@ -4706,7 +4712,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 		  if (mips_debug)
 		    fprintf_filtered (gdb_stdlog, " - reg=%d val=%s",
 				      argreg,
-				      phex (regval, regsize));
+				      phex (regval, abi_regsize));
 		  regcache_cooked_write_signed (regcache, argreg, regval);
 		  argreg++;
 		}
@@ -4721,7 +4727,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 	         only needs to be adjusted when it has been used.  */
 
 	      if (stack_used_p)
-		stack_offset += align_up (partial_len, regsize);
+		stack_offset += align_up (partial_len, abi_regsize);
 	    }
 	}
       if (mips_debug)
-- 
2.5.5

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (4/4)

[Alan Hayward]

> On 9 Jun 2017, at 15:29, Pedro Alves <palves@redhat.com> wrote:
> 
> On 06/09/2017 02:23 PM, Maciej W. Rozycki wrote:
> 
>> Hardcoding things, and especially with literals, causes a maintenance 
>> pain when things change.  Bad code elsewhere is not an excuse; besides, 
>> the other places where `8' is hardcoded are not (buffer) limits, but just 
>> handle specific register sizes, which are not going to change, and which 
>> are a completely different matter. 
> 
> Perhaps you won't be surprised to learn that I subscribe to the same general
> principles.  However, I believe that this isn't, in fact, a completely
> different matter, because the buffer in question is used to hold the
> contents of a structure's address, to either put in a general register,
> or in a stack slot, and those (addresses and general registers), unlike FPRs,
> aren't expected to grow for a given architecture.
> 
>> So if you find `alloca' unacceptable, 
>> then the limit has to be a macro, and an assertion check is also due (as 
>> already proposed), preferably using `sizeof', so that a future change does 
>> not break it by accident.
> 
> I think a patch like the below is likely to clarify things.
> (Builds, but is otherwise untested).
> 

I’d be happy with this patch. I like how the define refers just to the usage
of the register.

With the two minor changes below, it passes all my tests.


> From 5bdf1c939d72bf2718a542164d99bb396f0526e3 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Fri, 9 Jun 2017 15:10:50 +0100
> Subject: [PATCH] mips
> 
> ---
> gdb/mips-tdep.c | 38 ++++++++++++++++++++++----------------
> 1 file changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
> index 82f91ba..7844c73 100644
> --- a/gdb/mips-tdep.c
> +++ b/gdb/mips-tdep.c
> @@ -271,6 +271,9 @@ mips_isa_regsize (struct gdbarch *gdbarch)
> 	  / gdbarch_bfd_arch_info (gdbarch)->bits_per_byte);
> }
> 
> +/* Max saved register size.  */
> +#define MAX_MIPS_ABI_REGSIZE 8
> +
> /* Return the currently configured (or set) saved register size.  */
> 
> unsigned int
> @@ -4476,7 +4479,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>   int stack_offset = 0;
>   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>   CORE_ADDR func_addr = find_function_addr (function, NULL);
> -  int regsize = mips_abi_regsize (gdbarch);
> +  int abi_regsize = mips_abi_regsize (gdbarch);
> 
>   /* For shared libraries, "t9" needs to point at the function
>      address.  */
> @@ -4499,7 +4502,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>      than necessary for EABI, because the first few arguments are
>      passed in registers, but that's OK.  */
>   for (argnum = 0; argnum < nargs; argnum++)
> -    len += align_up (TYPE_LENGTH (value_type (args[argnum])), regsize);
> +    len += align_up (TYPE_LENGTH (value_type (args[argnum])), abi_regsize);
>   sp -= align_up (len, 16);
> 
>   if (mips_debug)
> @@ -4528,7 +4531,9 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>   for (argnum = 0; argnum < nargs; argnum++)
>     {
>       const gdb_byte *val;
> -      gdb_byte valbuf[MAX_REGISTER_SIZE];
> +      /* This holds the address of structures that are passed by
> +	 reference.  */
> +      gdb_byte ref_valbuf[MAX_MIPS_ABI_REGISTER_SIZE];

Name of the define is MAX_MIPS_ABI_REGSIZE, not MAX_MIPS_ABI_REGISTER_SIZE.


>       struct value *arg = args[argnum];
>       struct type *arg_type = check_typedef (value_type (arg));
>       int len = TYPE_LENGTH (arg_type);
> @@ -4541,13 +4546,14 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
> 
>       /* The EABI passes structures that do not fit in a register by
>          reference.  */
> -      if (len > regsize
> +      if (len > abi_regsize
> 	  && (typecode == TYPE_CODE_STRUCT || typecode == TYPE_CODE_UNION))
> 	{
> -	  store_unsigned_integer (valbuf, regsize, byte_order,
> +	  gdb_assert (ARRAY_SIZE (ref_valbuf) >= abi_regsize);
> +	  store_unsigned_integer (ref_valbuf, abi_regsize, byte_order,
> 				  value_address (arg));
> 	  typecode = TYPE_CODE_PTR;
> -	  len = regsize;
> +	  len = abi_regsize;
> 	  val = valbuf;

This line needs changing from valbuf to ref_valbuf.


> 	  if (mips_debug)
> 	    fprintf_unfiltered (gdb_stdlog, " push");
> @@ -4560,7 +4566,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>          up before the check to see if there are any FP registers
>          left.  Non MIPS_EABI targets also pass the FP in the integer
>          registers so also round up normal registers.  */
> -      if (regsize < 8 && fp_register_arg_p (gdbarch, typecode, arg_type))
> +      if (abi_regsize < 8 && fp_register_arg_p (gdbarch, typecode, arg_type))
> 	{
> 	  if ((float_argreg & 1))
> 	    float_argreg++;
> @@ -4626,12 +4632,12 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
> 	  /* Copy the argument to general registers or the stack in
> 	     register-sized pieces.  Large arguments are split between
> 	     registers and stack.  */
> -	  /* Note: structs whose size is not a multiple of regsize
> +	  /* Note: structs whose size is not a multiple of abi_regsize
> 	     are treated specially: Irix cc passes
> 	     them in registers where gcc sometimes puts them on the
> 	     stack.  For maximum compatibility, we will put them in
> 	     both places.  */
> -	  int odd_sized_struct = (len > regsize && len % regsize != 0);
> +	  int odd_sized_struct = (len > abi_regsize && len % abi_regsize != 0);
> 
> 	  /* Note: Floating-point values that didn't fit into an FP
> 	     register are only written to memory.  */
> @@ -4639,7 +4645,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
> 	    {
> 	      /* Remember if the argument was written to the stack.  */
> 	      int stack_used_p = 0;
> -	      int partial_len = (len < regsize ? len : regsize);
> +	      int partial_len = (len < abi_regsize ? len : abi_regsize);
> 
> 	      if (mips_debug)
> 		fprintf_unfiltered (gdb_stdlog, " -- partial=%d",
> @@ -4657,15 +4663,15 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
> 		  stack_used_p = 1;
> 		  if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
> 		    {
> -		      if (regsize == 8
> +		      if (abi_regsize == 8
> 			  && (typecode == TYPE_CODE_INT
> 			      || typecode == TYPE_CODE_PTR
> 			      || typecode == TYPE_CODE_FLT) && len <= 4)
> -			longword_offset = regsize - len;
> +			longword_offset = abi_regsize - len;
> 		      else if ((typecode == TYPE_CODE_STRUCT
> 				|| typecode == TYPE_CODE_UNION)
> -			       && TYPE_LENGTH (arg_type) < regsize)
> -			longword_offset = regsize - len;
> +			       && TYPE_LENGTH (arg_type) < abi_regsize)
> +			longword_offset = abi_regsize - len;
> 		    }
> 
> 		  if (mips_debug)
> @@ -4706,7 +4712,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
> 		  if (mips_debug)
> 		    fprintf_filtered (gdb_stdlog, " - reg=%d val=%s",
> 				      argreg,
> -				      phex (regval, regsize));
> +				      phex (regval, abi_regsize));
> 		  regcache_cooked_write_signed (regcache, argreg, regval);
> 		  argreg++;
> 		}
> @@ -4721,7 +4727,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
> 	         only needs to be adjusted when it has been used.  */
> 
> 	      if (stack_used_p)
> -		stack_offset += align_up (partial_len, regsize);
> +		stack_offset += align_up (partial_len, abi_regsize);
> 	    }
> 	}
>       if (mips_debug)
> -- 
> 2.5.5
> 
> 

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (4/4)

[Maciej W. Rozycki]

On Fri, 9 Jun 2017, Pedro Alves wrote:

> >  Hardcoding things, and especially with literals, causes a maintenance 
> > pain when things change.  Bad code elsewhere is not an excuse; besides, 
> > the other places where `8' is hardcoded are not (buffer) limits, but just 
> > handle specific register sizes, which are not going to change, and which 
> > are a completely different matter. 
> 
> Perhaps you won't be surprised to learn that I subscribe to the same general
> principles.  However, I believe that this isn't, in fact, a completely
> different matter, because the buffer in question is used to hold the
> contents of a structure's address, to either put in a general register,
> or in a stack slot, and those (addresses and general registers), unlike FPRs,
> aren't expected to grow for a given architecture.

 Well, apart from being a software bug a buffer overflow is a potential 
security concern, while an unhandled particular object kind is only a bug.

> > So if you find `alloca' unacceptable, 
> > then the limit has to be a macro, and an assertion check is also due (as 
> > already proposed), preferably using `sizeof', so that a future change does 
> > not break it by accident.
> 
> I think a patch like the below is likely to clarify things.
> (Builds, but is otherwise untested).

 LGTM; fairly mechanical.  Thanks!

  Maciej

Re: [PATCH 3/11] Add MIPS_MAX_REGISTER_SIZE (4/4)

[Pedro Alves]

On 06/12/2017 10:09 AM, Alan Hayward wrote:

> IÂ’d be happy with this patch. I like how the define refers just to the usage
> of the register.

Great.

> With the two minor changes below, it passes all my tests.

Whoops, looks like I posted a slightly outdated patch.  Sorry
about that.

> Name of the define is MAX_MIPS_ABI_REGSIZE, not MAX_MIPS_ABI_REGISTER_SIZE.

Indeed.

> This line needs changing from valbuf to ref_valbuf.

*nod*

On 06/12/2017 03:29 PM, Maciej W. Rozycki wrote:

>  LGTM; fairly mechanical.  Thanks!
> 

Great, thanks.  I've pushed this in now.

From b3464d0316235899d9facf81896d7a427d5cd6d0 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 12 Jun 2017 19:04:52 +0100
Subject: [PATCH] mips-tdep.c: Remove MAX_REGISTER_SIZE usage

mips_eabi_push_dummy_call is storing the address of a struct in a
buffer that must have the same of the configured/set ABI register size.
Add a define for the maximum ABI size and use it to size the local
buffer.  Also rename the 'regsize' local to 'abi_regsize' for clarity.

Tested that --enable-targets=all still builds.

gdb/ChangeLog:
2017-06-12  Pedro Alves  <palves@redhat.com>
	    Alan Hayward  <alan.hayward@arm.com>

	* mips-tdep.c (MAX_MIPS_ABI_REGSIZE): New.
	(mips_eabi_push_dummy_call): Rename local 'regsize' to
	'abi_regsize'.  Rename local array 'valbuf' to 'ref_valbuf', and
	use MAX_MIPS_ABI_REGSIZE instead of MAX_REGISTER_SIZE to size it.
	Assert that abi_regsize bytes fit in 'ref_valbuf'.
---
 gdb/ChangeLog   |  9 +++++++++
 gdb/mips-tdep.c | 40 +++++++++++++++++++++++-----------------
 2 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 82f972a..0298a15 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,4 +1,13 @@
 2017-06-12  Pedro Alves  <palves@redhat.com>
+	    Alan Hayward  <alan.hayward@arm.com>
+
+	* mips-tdep.c (MAX_MIPS_ABI_REGSIZE): New.
+	(mips_eabi_push_dummy_call): Rename local 'regsize' to
+	'abi_regsize'.  Rename local array 'valbuf' to 'ref_valbuf', and
+	use MAX_MIPS_ABI_REGSIZE instead of MAX_REGISTER_SIZE to size it.
+	Assert that abi_regsize bytes fit in 'ref_valbuf'.
+
+2017-06-12  Pedro Alves  <palves@redhat.com>
 
 	* dwarf2read.c (mapped_symtab::data): Now a vector of
 	symtab_index_entry instead of vector of
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 82f91ba..dcd9ef2 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -271,6 +271,9 @@ mips_isa_regsize (struct gdbarch *gdbarch)
 	  / gdbarch_bfd_arch_info (gdbarch)->bits_per_byte);
 }
 
+/* Max saved register size.  */
+#define MAX_MIPS_ABI_REGSIZE 8
+
 /* Return the currently configured (or set) saved register size.  */
 
 unsigned int
@@ -4476,7 +4479,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
   int stack_offset = 0;
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   CORE_ADDR func_addr = find_function_addr (function, NULL);
-  int regsize = mips_abi_regsize (gdbarch);
+  int abi_regsize = mips_abi_regsize (gdbarch);
 
   /* For shared libraries, "t9" needs to point at the function
      address.  */
@@ -4499,7 +4502,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
      than necessary for EABI, because the first few arguments are
      passed in registers, but that's OK.  */
   for (argnum = 0; argnum < nargs; argnum++)
-    len += align_up (TYPE_LENGTH (value_type (args[argnum])), regsize);
+    len += align_up (TYPE_LENGTH (value_type (args[argnum])), abi_regsize);
   sp -= align_up (len, 16);
 
   if (mips_debug)
@@ -4528,7 +4531,9 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
   for (argnum = 0; argnum < nargs; argnum++)
     {
       const gdb_byte *val;
-      gdb_byte valbuf[MAX_REGISTER_SIZE];
+      /* This holds the address of structures that are passed by
+	 reference.  */
+      gdb_byte ref_valbuf[MAX_MIPS_ABI_REGSIZE];
       struct value *arg = args[argnum];
       struct type *arg_type = check_typedef (value_type (arg));
       int len = TYPE_LENGTH (arg_type);
@@ -4541,14 +4546,15 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 
       /* The EABI passes structures that do not fit in a register by
          reference.  */
-      if (len > regsize
+      if (len > abi_regsize
 	  && (typecode == TYPE_CODE_STRUCT || typecode == TYPE_CODE_UNION))
 	{
-	  store_unsigned_integer (valbuf, regsize, byte_order,
+	  gdb_assert (abi_regsize <= ARRAY_SIZE (ref_valbuf));
+	  store_unsigned_integer (ref_valbuf, abi_regsize, byte_order,
 				  value_address (arg));
 	  typecode = TYPE_CODE_PTR;
-	  len = regsize;
-	  val = valbuf;
+	  len = abi_regsize;
+	  val = ref_valbuf;
 	  if (mips_debug)
 	    fprintf_unfiltered (gdb_stdlog, " push");
 	}
@@ -4560,7 +4566,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
          up before the check to see if there are any FP registers
          left.  Non MIPS_EABI targets also pass the FP in the integer
          registers so also round up normal registers.  */
-      if (regsize < 8 && fp_register_arg_p (gdbarch, typecode, arg_type))
+      if (abi_regsize < 8 && fp_register_arg_p (gdbarch, typecode, arg_type))
 	{
 	  if ((float_argreg & 1))
 	    float_argreg++;
@@ -4626,12 +4632,12 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 	  /* Copy the argument to general registers or the stack in
 	     register-sized pieces.  Large arguments are split between
 	     registers and stack.  */
-	  /* Note: structs whose size is not a multiple of regsize
+	  /* Note: structs whose size is not a multiple of abi_regsize
 	     are treated specially: Irix cc passes
 	     them in registers where gcc sometimes puts them on the
 	     stack.  For maximum compatibility, we will put them in
 	     both places.  */
-	  int odd_sized_struct = (len > regsize && len % regsize != 0);
+	  int odd_sized_struct = (len > abi_regsize && len % abi_regsize != 0);
 
 	  /* Note: Floating-point values that didn't fit into an FP
 	     register are only written to memory.  */
@@ -4639,7 +4645,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 	    {
 	      /* Remember if the argument was written to the stack.  */
 	      int stack_used_p = 0;
-	      int partial_len = (len < regsize ? len : regsize);
+	      int partial_len = (len < abi_regsize ? len : abi_regsize);
 
 	      if (mips_debug)
 		fprintf_unfiltered (gdb_stdlog, " -- partial=%d",
@@ -4657,15 +4663,15 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 		  stack_used_p = 1;
 		  if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
 		    {
-		      if (regsize == 8
+		      if (abi_regsize == 8
 			  && (typecode == TYPE_CODE_INT
 			      || typecode == TYPE_CODE_PTR
 			      || typecode == TYPE_CODE_FLT) && len <= 4)
-			longword_offset = regsize - len;
+			longword_offset = abi_regsize - len;
 		      else if ((typecode == TYPE_CODE_STRUCT
 				|| typecode == TYPE_CODE_UNION)
-			       && TYPE_LENGTH (arg_type) < regsize)
-			longword_offset = regsize - len;
+			       && TYPE_LENGTH (arg_type) < abi_regsize)
+			longword_offset = abi_regsize - len;
 		    }
 
 		  if (mips_debug)
@@ -4706,7 +4712,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 		  if (mips_debug)
 		    fprintf_filtered (gdb_stdlog, " - reg=%d val=%s",
 				      argreg,
-				      phex (regval, regsize));
+				      phex (regval, abi_regsize));
 		  regcache_cooked_write_signed (regcache, argreg, regval);
 		  argreg++;
 		}
@@ -4721,7 +4727,7 @@ mips_eabi_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
 	         only needs to be adjusted when it has been used.  */
 
 	      if (stack_used_p)
-		stack_offset += align_up (partial_len, regsize);
+		stack_offset += align_up (partial_len, abi_regsize);
 	    }
 	}
       if (mips_debug)
-- 
2.5.5