> Date: Thu, 10 Jan 2019 00:06:01 +0100
> From: Jakub Jelinek <ja...@redhat.com>
> 2019-01-09 Jakub Jelinek <ja...@redhat.com>
>
> PR middle-end/84877
> PR bootstrap/88450
> * function.c (assign_stack_local_1): Revert the 2018-11-21 changes.
> (assign_parm_setup_block): Do the argument slot realignment here
> instead.
When this was committed as r267812, results for cris-elf went
from regress-8 to regress-160 (now at r268749, regress-154).
<TL;DR>
I analyzed one of the simpler cases,
gcc.c-torture/execute/930126-1.c at -O0.
It looks like the code added in r267812 doesn't handle targets
where MAX_SUPPORTED_STACK_ALIGNMENT is less than BITS_PER_WORD
(by necessity, non-STRICT_ALIGNMENT targets); the bug is in not
adding necessary alignment to the allocated space.
cris-elf is a non-STRICT_ALIGNMENT target and
MAX_SUPPORTED_STACK_ALIGNMENT is defaulted from STACK_BOUNDARY,
16 (bits).
The only difference with r267812 is that before, 10 bytes
(excluding 4 for the saved frame-pointer-address) was allocated
for the stack-frame of the function f and at r267812, 8 bytes;
the literally only differences are two instructions, one when
allocating stack and one when forming the pointer to the
stack-parameter. The sizeof the actual object is 5 bytes, but
padded to 8 bytes due to copying from incoming registers (which
is IMO a valid reason, maybe not worthwhile to improve). Please
remember that size-padding is different from alignment-padding.
The setting of DECL_ALIGN (parm) to BITS_PER_WORD (originating
at r94104) is wrong too, but not fatal. I'll fix that in a
follow-up change and let's pretend for the moment that there's a
valid reason for aligning "parm" in this case (for example, a
user-provided overalignment).
At function entry, after saving the frame-pointer-register, the
stack-pointer is 0x3dfffece (both versions). The parameter-
pointer-align instructions align this to a multiple of 4 for
both versions, and this of course fails as there's no padding
with r267812. The actual failure is due to overwriting the
saved frame-pointer-register, with the wrong value then used in
main to verify the result.
</TL;DR>
To fix the r267812 incorrectness we need to use the *stored*
size, i.e. that which will be stored into using register-sized
writes. It's seems like the bug is just a typo, so the fix is
as simple as follows. Note the usage of "diff -U 10" to show
that size_stored is used in the "then"-arm.
Regtested on cris-elf, where it "introduces" gcc.dg/pr84877.c
but on inspection that's a known bug with a bit of hairy churn,
reverts and all. See the PR. This patch has no bearing on that
PR; this is for incoming parameters, and PR84877 is for (not
doing) alignment of pass-by-reference parameters for outgoing
parameters. Still, maybe the solution to that PR should have
code like in that the context below...which I see you already
noted, in the patch-message to which this is a reply.
Also regtested on x86_64-pc-linux-gnu (without regressions)
together with the followup-change.
Ok to commit?
gcc/ChangeLog:
* function.c (assign_parm_setup_block): Use the stored
size, not the passed size, when allocating stack-space,
also for a parameter with alignment larger than
MAX_SUPPORTED_STACK_ALIGNMENT.
both arms of the containing "if".)
--- gcc/function.c.orig Sun Jan 20 19:20:16 2019
+++ gcc/function.c Sat Feb 9 00:53:17 2019
@@ -2907,23 +2907,23 @@ assign_parm_setup_block (struct assign_p
}
data->stack_parm = NULL;
}
size = int_size_in_bytes (data->passed_type);
size_stored = CEIL_ROUND (size, UNITS_PER_WORD);
if (stack_parm == 0)
{
SET_DECL_ALIGN (parm, MAX (DECL_ALIGN (parm), BITS_PER_WORD));
if (DECL_ALIGN (parm) > MAX_SUPPORTED_STACK_ALIGNMENT)
{
- rtx allocsize = gen_int_mode (size, Pmode);
+ rtx allocsize = gen_int_mode (size_stored, Pmode);
get_dynamic_stack_size (&allocsize, 0, DECL_ALIGN (parm), NULL);
stack_parm = assign_stack_local (BLKmode, UINTVAL (allocsize),
MAX_SUPPORTED_STACK_ALIGNMENT);
rtx addr = align_dynamic_address (XEXP (stack_parm, 0),
DECL_ALIGN (parm));
mark_reg_pointer (addr, DECL_ALIGN (parm));
stack_parm = gen_rtx_MEM (GET_MODE (stack_parm), addr);
MEM_NOTRAP_P (stack_parm) = 1;
}
else
stack_parm = assign_stack_local (BLKmode, size_stored,
brgds, H-P
