Re: Two problems with current_function_pretend_args_size

[Jim Wilson <wilson at specifixinc dot com>]

Richard Sandiford wrote:
> 1) If an argument is passed partially in registers, there will be
>    pretend_args_size bytes between virtual_incoming_args and the first
>    real stack argument.  The problem is that pretend_args_size is only
>    rounded to PARM_BOUNDARY bits, not STACK_BOUNDARY bits.  This can
>    throw off the location of later stack arguments.

The 2) case with SETUP_INCOMING_VARARGS looks clearly correct.

However, this first case isn't clear to me.

It isn't clear what bug you are fixing.  Is gcc incompatible with the 
SGI compiler here?  Does the testcase fail without the patch?  Does the 
testcase fail because the caller/callee think arguments are in different 
places.  Does it fail for some other reason?  I don't think it can fail 
due to unaligned memory accesses, because there are no normal mips 
load/store instructions that require 16-byte alignment.

Another issue here is whether we really have to align the argument 
pointer, or whether we can just fix locate_and_pad_parm to add the extra 
word of padding between pb and pc.  If we don't have to align the 
argument pointer to STACK_BOUNDARY, then we don't have to waste some 
stack space.

If gcc is incompatible with the SGI compiler, and the arg pointer needs 
16-byte alignment, then this patch does make sense.

It isn't clear that STACK_BOUNDARY is exactly what we want.  Does the 
ABI impose alignment requirements on the arg pointer?  If not, then all 
we need is MAX_PARM_BOUNDARY, but we don't have such a macro.  The 
question then is which macro comes closest without going under.  I think 
STACK_BOUNDARY and BIGGEST_ALIGNMENT are the only two reasonable 
choices.  Looking through the ports, it looks like some have 
BIGGEST_ALIGNMENT larger than STACK_BOUNDARY, but none vice versa, so 
STACK_BOUNDARY looks like the best compromise.  I think it would be 
useful to explain some of this in a comment.

Another thing unclear about the patch is that you add to 
stack_args_size.constant before calling locate_and_pad_parm.  This 
ensures that the extra padding goes before the first argument.  It took 
me a little while to figure this out, because locate_and_pad_parm is 
invisible in the patch.  Also, couldn't this just be an = instead of a 
+=?  Only the first argument on the stack can be partially in regs and 
partially on the stack, so I would think that the stack_size should 
always be zero here.

This patch is a potential silent ABI change for a number of targets. 
Any target which has arguments that require more alignment than 
PARM_BOUNDARY and which don't use REG_PARM_STACK_SPACE will be affected 
I think.  This kind of thing should be documented in release notes. 
Also, for targets where the ABI is defined by gcc, it could be argued 
that this is wrong, since we are breaking the existing "documented" ABI. 
 However, I think this is useful breakage in that we get something more 
sensible than what we had before.  It might be useful to try to identify 
affected targets and make sure the ABI change is OK for them.  I believe 
the plan is to change the C++ ABI from 3.3 to 3.4, so changing C ABIs at 
this time should be tolerable.
--
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com