[3/11] Add MIPS_MAX_REGISTER_SIZE (4/4)

  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(-)
  

> 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

> 

>
  

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