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

  > 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.
  

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
  

> 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.
  

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
  

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
  

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
  

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