[3/11] Add MIPS_MAX_REGISTER_SIZE
Commit Message
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
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.
> 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.
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.
@@ -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);
@@ -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);
@@ -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. */
@@ -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;