diff mbox

[v3] PR77359: Properly align local variables in functions calling alloca.

Message ID 20161111093316.GA24679@linux.vnet.ibm.com
State New
Headers show

Commit Message

Dominik Vogt Nov. 11, 2016, 9:33 a.m. UTC 
On Fri, Nov 11, 2016 at 12:11:49AM -0500, David Edelsohn wrote:
> On Thu, Nov 10, 2016 at 6:47 PM, Dominik Vogt <vogt@linux.vnet.ibm.com> wrote:
> > On Thu, Nov 03, 2016 at 11:40:44AM +0100, Dominik Vogt wrote:
> >> The attached patch fixes the stack layout problems on AIX and
> >> Power as described here:
> >>
> >>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77359
> >>
> >> The patch has been bootstrapped on AIX (32 Bit) and bootstrappend
> >> and regression tested on Power (biarch).  It needs more testing
> >> that I cannot do with the hardware available to me.
> >>
> >> If the patch is good, this one can be re-applied:
> >>
> >>   https://gcc.gnu.org/ml/gcc-patches/2016-07/msg01730.html
> >>   https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01616.html
> >
> > So, is this patch in order to be committed?  (Assuming that a
> > followup patch will clean up the rs6000.h+aix.h quirks.)
> 
> Please also update the ASCII pictures above the rs6000_stack_info()
> function in rs6000.c to show / describe the new padding for alignment.

Like in the new patch?  (Please double check that I got this right
in all three cases).

Ciao

Dominik ^_^  ^_^

Comments

Segher Boessenkool Nov. 11, 2016, 8:20 p.m. UTC | #1 
On Fri, Nov 11, 2016 at 10:33:16AM +0100, Dominik Vogt wrote:
> > > So, is this patch in order to be committed?  (Assuming that a
> > > followup patch will clean up the rs6000.h+aix.h quirks.)
> > 
> > Please also update the ASCII pictures above the rs6000_stack_info()
> > function in rs6000.c to show / describe the new padding for alignment.
> 
> Like in the new patch?  (Please double check that I got this right
> in all three cases).

Looks good to me.


Segher
Andreas Krebbel Nov. 18, 2016, 2:29 p.m. UTC | #2 
On Fri, Nov 11, 2016 at 10:33:16AM +0100, Dominik Vogt wrote:
> gcc/ChangeLog
> 
> 	* config/rs6000/rs6000.c (rs6000_stack_info): PR/77359: Properly align
> 	local variables in functions calling alloca.  Also update the ASCII
> 	drawings
> 	* config/rs6000/rs6000.h (STARTING_FRAME_OFFSET, STACK_DYNAMIC_OFFSET):
> 	PR/77359: Likewise.
> 	* config/rs6000/aix.h (STARTING_FRAME_OFFSET, STACK_DYNAMIC_OFFSET):
> 	PR/77359: Copy AIX specific versions of the rs6000.h macros to aix.h.

Applied. Thanks!

-Andreas-
diff mbox

Patch

diff --git a/gcc/config/rs6000/aix.h b/gcc/config/rs6000/aix.h
index b254236..f6eb122 100644
--- a/gcc/config/rs6000/aix.h
+++ b/gcc/config/rs6000/aix.h
@@ -40,6 +40,41 @@ 
 #undef  STACK_BOUNDARY
 #define STACK_BOUNDARY 128
 
+/* Offset within stack frame to start allocating local variables at.
+   If FRAME_GROWS_DOWNWARD, this is the offset to the END of the
+   first local allocated.  Otherwise, it is the offset to the BEGINNING
+   of the first local allocated.
+
+   On the RS/6000, the frame pointer is the same as the stack pointer,
+   except for dynamic allocations.  So we start after the fixed area and
+   outgoing parameter area.
+
+   If the function uses dynamic stack space (CALLS_ALLOCA is set), that
+   space needs to be aligned to STACK_BOUNDARY, i.e. the sum of the
+   sizes of the fixed area and the parameter area must be a multiple of
+   STACK_BOUNDARY.  */
+
+#undef STARTING_FRAME_OFFSET
+#define STARTING_FRAME_OFFSET						\
+  (FRAME_GROWS_DOWNWARD							\
+   ? 0									\
+   : (cfun->calls_alloca						\
+      ? RS6000_ALIGN (crtl->outgoing_args_size + RS6000_SAVE_AREA, 16)	\
+      : (RS6000_ALIGN (crtl->outgoing_args_size, 16) + RS6000_SAVE_AREA)))
+
+/* Offset from the stack pointer register to an item dynamically
+   allocated on the stack, e.g., by `alloca'.
+
+   The default value for this macro is `STACK_POINTER_OFFSET' plus the
+   length of the outgoing arguments.  The default is correct for most
+   machines.  See `function.c' for details.
+
+   This value must be a multiple of STACK_BOUNDARY (hard coded in
+   `emit-rtl.c').  */
+#undef STACK_DYNAMIC_OFFSET
+#define STACK_DYNAMIC_OFFSET(FUNDECL)					\
+   RS6000_ALIGN (crtl->outgoing_args_size + STACK_POINTER_OFFSET, 16)
+
 #undef  TARGET_IEEEQUAD
 #define TARGET_IEEEQUAD 0
 
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index f9e4739..4f3b886 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -25781,7 +25781,7 @@  rs6000_savres_strategy (rs6000_stack_t *info,
 		+---------------------------------------+
 		| saved TOC pointer			| 20      40
 		+---------------------------------------+
-		| Parameter save area (P)		| 24      48
+		| Parameter save area (+padding*) (P)	| 24      48
 		+---------------------------------------+
 		| Alloca space (A)			| 24+P    etc.
 		+---------------------------------------+
@@ -25802,6 +25802,9 @@  rs6000_savres_strategy (rs6000_stack_t *info,
 	old SP->| back chain to caller's caller		|
 		+---------------------------------------+
 
+     * If the alloca area is present, the parameter save area is
+       padded so that the former starts 16-byte aligned.
+
    The required alignment for AIX configurations is two words (i.e., 8
    or 16 bytes).
 
@@ -25816,7 +25819,7 @@  rs6000_savres_strategy (rs6000_stack_t *info,
 		+---------------------------------------+
 		| Saved TOC pointer			|  24
 		+---------------------------------------+
-		| Parameter save area (P)		|  32
+		| Parameter save area (+padding*) (P)	|  32
 		+---------------------------------------+
 		| Alloca space (A)			|  32+P
 		+---------------------------------------+
@@ -25833,6 +25836,8 @@  rs6000_savres_strategy (rs6000_stack_t *info,
 	old SP->| back chain to caller's caller		|  32+P+A+L+W+Y+G+F
 		+---------------------------------------+
 
+     * If the alloca area is present, the parameter save area is
+       padded so that the former starts 16-byte aligned.
 
    V.4 stack frames look like:
 
@@ -25841,7 +25846,7 @@  rs6000_savres_strategy (rs6000_stack_t *info,
 		+---------------------------------------+
 		| caller's saved LR			| 4
 		+---------------------------------------+
-		| Parameter save area (P)		| 8
+		| Parameter save area (+padding*) (P)	| 8
 		+---------------------------------------+
 		| Alloca space (A)			| 8+P
 		+---------------------------------------+
@@ -25870,6 +25875,10 @@  rs6000_savres_strategy (rs6000_stack_t *info,
 	old SP->| back chain to caller's caller		|
 		+---------------------------------------+
 
+     * If the alloca area is present and the required alignment is
+       16 bytes, the parameter save area is padded so that the
+       alloca area starts 16-byte aligned.
+
    The required alignment for V.4 is 16 bytes, or 8 bytes if -meabi is
    given.  (But note below and in sysv4.h that we require only 8 and
    may round up the size of our stack frame anyways.  The historical
@@ -26004,8 +26013,13 @@  rs6000_stack_info (void)
   info->reg_size     = reg_size;
   info->fixed_size   = RS6000_SAVE_AREA;
   info->vars_size    = RS6000_ALIGN (get_frame_size (), 8);
-  info->parm_size    = RS6000_ALIGN (crtl->outgoing_args_size,
-					 TARGET_ALTIVEC ? 16 : 8);
+  if (cfun->calls_alloca)
+    info->parm_size  =
+      RS6000_ALIGN (crtl->outgoing_args_size + info->fixed_size,
+		    STACK_BOUNDARY / BITS_PER_UNIT) - info->fixed_size;
+  else
+    info->parm_size  = RS6000_ALIGN (crtl->outgoing_args_size,
+				     TARGET_ALTIVEC ? 16 : 8);
   if (FRAME_GROWS_DOWNWARD)
     info->vars_size
       += RS6000_ALIGN (info->fixed_size + info->vars_size + info->parm_size,
diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h
index 4b83abd..afda416 100644
--- a/gcc/config/rs6000/rs6000.h
+++ b/gcc/config/rs6000/rs6000.h
@@ -1723,25 +1723,35 @@  extern enum reg_class rs6000_constraints[RS6000_CONSTRAINT_MAX];
 
    On the RS/6000, the frame pointer is the same as the stack pointer,
    except for dynamic allocations.  So we start after the fixed area and
-   outgoing parameter area.  */
+   outgoing parameter area.
+
+   If the function uses dynamic stack space (CALLS_ALLOCA is set), that
+   space needs to be aligned to STACK_BOUNDARY, i.e. the sum of the
+   sizes of the fixed area and the parameter area must be a multiple of
+   STACK_BOUNDARY.  */
 
 #define STARTING_FRAME_OFFSET						\
   (FRAME_GROWS_DOWNWARD							\
    ? 0									\
-   : (RS6000_ALIGN (crtl->outgoing_args_size,				\
-		    (TARGET_ALTIVEC || TARGET_VSX) ? 16 : 8)		\
-      + RS6000_SAVE_AREA))
+   : (cfun->calls_alloca						\
+      ? (RS6000_ALIGN (crtl->outgoing_args_size + RS6000_SAVE_AREA,	\
+		       (TARGET_ALTIVEC || TARGET_VSX) ? 16 : 8 ))	\
+      : (RS6000_ALIGN (crtl->outgoing_args_size,			\
+		       (TARGET_ALTIVEC || TARGET_VSX) ? 16 : 8)		\
+	 + RS6000_SAVE_AREA)))
 
 /* Offset from the stack pointer register to an item dynamically
    allocated on the stack, e.g., by `alloca'.
 
    The default value for this macro is `STACK_POINTER_OFFSET' plus the
    length of the outgoing arguments.  The default is correct for most
-   machines.  See `function.c' for details.  */
+   machines.  See `function.c' for details.
+
+   This value must be a multiple of STACK_BOUNDARY (hard coded in
+   `emit-rtl.c').  */
 #define STACK_DYNAMIC_OFFSET(FUNDECL)					\
-  (RS6000_ALIGN (crtl->outgoing_args_size,				\
-		 (TARGET_ALTIVEC || TARGET_VSX) ? 16 : 8)		\
-   + (STACK_POINTER_OFFSET))
+  RS6000_ALIGN (crtl->outgoing_args_size + STACK_POINTER_OFFSET,	\
+		(TARGET_ALTIVEC || TARGET_VSX) ? 16 : 8)
 
 /* If we generate an insn to push BYTES bytes,
    this says how many the stack pointer really advances by.