diff mbox

[3/11] Add MIPS_MAX_REGISTER_SIZE

Message ID 3C00280E-37C9-4C0A-9DA6-F3B9DB1A6E8F@arm.com
State New
Headers show

Commit Message

Alan Hayward April 4, 2017, 10:12 a.m. UTC
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

Comments

John Baldwin April 4, 2017, 3:59 p.m. UTC | #1
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.
Alan Hayward April 5, 2017, 10:27 a.m. UTC | #2
> 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.
Yao Qi April 11, 2017, 3:37 p.m. UTC | #3
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.
diff mbox

Patch

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;