diff mbox series

Follow-up-fix to "[PATCH] Move PR84877 fix elsewhere (PR bootstrap/88450)"

Message ID 201902110105.x1B15BPC021610@ignucius.se.axis.com
State New
Headers show
Series Follow-up-fix to "[PATCH] Move PR84877 fix elsewhere (PR bootstrap/88450)" | expand

Commit Message

Hans-Peter Nilsson Feb. 11, 2019, 1:05 a.m. UTC 
> Date: Thu, 10 Jan 2019 00:06:01 +0100
> From: Jakub Jelinek <jakub@redhat.com>

> 2019-01-09  Jakub Jelinek  <jakub@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".)

brgds, H-P

Comments

Eric Botcazou Feb. 11, 2019, 8:53 a.m. UTC | #1 
> 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.

Right, thanks for catching it.

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

OK, thanks.
diff mbox series

Patch

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