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

["Maciej W. Rozycki" <macro@imgtec.com>]

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