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

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

[Dominik Vogt]

[-- Attachment #1: Type: text/plain, Size: 549 bytes --]

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

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

[-- Attachment #2: 0001-ChangeLog --]
[-- Type: text/plain, Size: 338 bytes --]

gcc/ChangeLog

	* config/rs6000/rs6000.c (rs6000_stack_info): Properly align local
	variables in functions calling alloca.
	* config/rs6000/rs6000.h (STARTING_FRAME_OFFSET, STACK_DYNAMIC_OFFSET):
	Likewise.
	* config/rs6000/aix.h (STARTING_FRAME_OFFSET, STACK_DYNAMIC_OFFSET):
	Copy AIX specific versions of the rs6000.h macros to aix.h.

[-- Attachment #3: 0001-PR77359-Properly-align-local-variables-in-functions-.patch --]
[-- Type: text/plain, Size: 4372 bytes --]

From bd36042fd82e29204d2f10c180b9e7c27281eef2 Mon Sep 17 00:00:00 2001
From: Dominik Vogt <vogt@linux.vnet.ibm.com>
Date: Fri, 28 Oct 2016 12:59:55 +0100
Subject: [PATCH] PR77359: Properly align local variables in functions
 calling alloca.

See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77359 for a discussion of the
problem and the fix.
---
 gcc/config/rs6000/aix.h    | 27 +++++++++++++++++++++++++++
 gcc/config/rs6000/rs6000.c |  9 +++++++--
 gcc/config/rs6000/rs6000.h | 14 ++++++++------
 3 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/gcc/config/rs6000/aix.h b/gcc/config/rs6000/aix.h
index b254236..7773517 100644
--- a/gcc/config/rs6000/aix.h
+++ b/gcc/config/rs6000/aix.h
@@ -40,6 +40,33 @@
 #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.  */
+
+#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.  */
+#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..02ed9c1 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -26004,8 +26004,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..c11dc1b 100644
--- a/gcc/config/rs6000/rs6000.h
+++ b/gcc/config/rs6000/rs6000.h
@@ -1728,9 +1728,12 @@ extern enum reg_class rs6000_constraints[RS6000_CONSTRAINT_MAX];
 #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'.
@@ -1739,9 +1742,8 @@ extern enum reg_class rs6000_constraints[RS6000_CONSTRAINT_MAX];
    length of the outgoing arguments.  The default is correct for most
    machines.  See `function.c' for details.  */
 #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.
-- 
2.3.0

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

[Dominik Vogt]

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

> gcc/ChangeLog
> 
> 	* config/rs6000/rs6000.c (rs6000_stack_info): Properly align local
> 	variables in functions calling alloca.
> 	* config/rs6000/rs6000.h (STARTING_FRAME_OFFSET, STACK_DYNAMIC_OFFSET):
> 	Likewise.
> 	* config/rs6000/aix.h (STARTING_FRAME_OFFSET, STACK_DYNAMIC_OFFSET):
> 	Copy AIX specific versions of the rs6000.h macros to aix.h.

> >From bd36042fd82e29204d2f10c180b9e7c27281eef2 Mon Sep 17 00:00:00 2001
> From: Dominik Vogt <vogt@linux.vnet.ibm.com>
> Date: Fri, 28 Oct 2016 12:59:55 +0100
> Subject: [PATCH] PR77359: Properly align local variables in functions
>  calling alloca.
> 
> See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77359 for a discussion of the
> problem and the fix.
> ---
>  gcc/config/rs6000/aix.h    | 27 +++++++++++++++++++++++++++
>  gcc/config/rs6000/rs6000.c |  9 +++++++--
>  gcc/config/rs6000/rs6000.h | 14 ++++++++------
>  3 files changed, 42 insertions(+), 8 deletions(-)
> 
> diff --git a/gcc/config/rs6000/aix.h b/gcc/config/rs6000/aix.h
> index b254236..7773517 100644
> --- a/gcc/config/rs6000/aix.h
> +++ b/gcc/config/rs6000/aix.h
> @@ -40,6 +40,33 @@
>  #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.  */
> +
> +#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.  */
> +#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..02ed9c1 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -26004,8 +26004,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..c11dc1b 100644
> --- a/gcc/config/rs6000/rs6000.h
> +++ b/gcc/config/rs6000/rs6000.h
> @@ -1728,9 +1728,12 @@ extern enum reg_class rs6000_constraints[RS6000_CONSTRAINT_MAX];
>  #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'.
> @@ -1739,9 +1742,8 @@ extern enum reg_class rs6000_constraints[RS6000_CONSTRAINT_MAX];
>     length of the outgoing arguments.  The default is correct for most
>     machines.  See `function.c' for details.  */
>  #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.
> -- 
> 2.3.0
> 



Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

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

[Segher Boessenkool]

On Fri, Nov 11, 2016 at 12:47:02AM +0100, Dominik Vogt 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.)

You say it needs more testing -- what testing?

(And it needs to be posted to gcc-patches@ of course).

> > +#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)))

Maybe you can make the comment explain these last two lines as well...  It
seems to me you want to align STARTING_FRAME_OFFSET if calls_alloca?

Also add a comment for the one in rs6000.h?

> > +/* 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.  */
> > +#undef STACK_DYNAMIC_OFFSET
> > +#define STACK_DYNAMIC_OFFSET(FUNDECL)					\
> > +   RS6000_ALIGN (crtl->outgoing_args_size + (STACK_POINTER_OFFSET), 16)

You don't need parens around STACK_POINTER_OFFSET.

Looks fine to me except for those nits,


Segher

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

[David Edelsohn]

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.

Thanks, David

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

[Dominik Vogt]

[-- Attachment #1: Type: text/plain, Size: 2011 bytes --]

On Thu, Nov 10, 2016 at 06:17:57PM -0600, Segher Boessenkool wrote:
> On Fri, Nov 11, 2016 at 12:47:02AM +0100, Dominik Vogt 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.)
> 
> You say it needs more testing -- what testing?

Regression testing on AIX (David has done this in reply to the
original message), possibly also on 32-Bit Power, if that is a
reasonable target.

> (And it needs to be posted to gcc-patches@ of course).

This discussion was originally on gcc-patches, but somehow it got
removed from the recipient list.

> > > +   : (cfun->calls_alloca						\
> > > +      ? RS6000_ALIGN (crtl->outgoing_args_size + RS6000_SAVE_AREA, 16)	\
> > > +      : (RS6000_ALIGN (crtl->outgoing_args_size, 16) + RS6000_SAVE_AREA)))
> 
> Maybe you can make the comment explain these last two lines as well...  It
> seems to me you want to align STARTING_FRAME_OFFSET if calls_alloca?

Done.

> Also add a comment for the one in rs6000.h?

Done.

> > > +   RS6000_ALIGN (crtl->outgoing_args_size + (STACK_POINTER_OFFSET), 16)
> 
> You don't need parens around STACK_POINTER_OFFSET.

Done.

New patch attached (added more comments and removed parentheses;
not re-tested).  (The first attachment is a diff between the two
patches.)

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

[-- Attachment #2: v1-v2.diff --]
[-- Type: text/plain, Size: 4615 bytes --]

--- old/0001-PR77359-Properly-align-local-variables-in-functions-.patch	2016-11-11 09:52:16.535439142 +0100
+++ new/0001-PR77359-Properly-align-local-variables-in-functions-.patch	2016-11-11 09:50:03.715439142 +0100
@@ -1,4 +1,4 @@
-From bd36042fd82e29204d2f10c180b9e7c27281eef2 Mon Sep 17 00:00:00 2001
+From faae30210f584bba92ab96aac479ae8f253e59b7 Mon Sep 17 00:00:00 2001
 From: Dominik Vogt <vogt@linux.vnet.ibm.com>
 Date: Fri, 28 Oct 2016 12:59:55 +0100
 Subject: [PATCH 1/2] PR77359: Properly align local variables in functions
@@ -7,16 +7,16 @@
 See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77359 for a discussion of the
 problem and the fix.
 ---
- gcc/config/rs6000/aix.h    | 27 +++++++++++++++++++++++++++
+ gcc/config/rs6000/aix.h    | 35 +++++++++++++++++++++++++++++++++++
  gcc/config/rs6000/rs6000.c |  9 +++++++--
- gcc/config/rs6000/rs6000.h | 14 ++++++++------
- 3 files changed, 42 insertions(+), 8 deletions(-)
+ gcc/config/rs6000/rs6000.h | 26 ++++++++++++++++++--------
+ 3 files changed, 60 insertions(+), 10 deletions(-)
 
 diff --git a/gcc/config/rs6000/aix.h b/gcc/config/rs6000/aix.h
-index b254236..7773517 100644
+index b254236..f6eb122 100644
 --- a/gcc/config/rs6000/aix.h
 +++ b/gcc/config/rs6000/aix.h
-@@ -40,6 +40,33 @@
+@@ -40,6 +40,41 @@
  #undef  STACK_BOUNDARY
  #define STACK_BOUNDARY 128
  
@@ -27,7 +27,12 @@
 +
 +   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.  */
 +
 +#undef STARTING_FRAME_OFFSET
 +#define STARTING_FRAME_OFFSET						\
@@ -42,10 +47,13 @@
 +
 +   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').  */
 +#undef STACK_DYNAMIC_OFFSET
 +#define STACK_DYNAMIC_OFFSET(FUNDECL)					\
-+   RS6000_ALIGN (crtl->outgoing_args_size + (STACK_POINTER_OFFSET), 16)
++   RS6000_ALIGN (crtl->outgoing_args_size + STACK_POINTER_OFFSET, 16)
 +
  #undef  TARGET_IEEEQUAD
  #define TARGET_IEEEQUAD 0
@@ -71,10 +79,21 @@
      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..c11dc1b 100644
+index 4b83abd..afda416 100644
 --- a/gcc/config/rs6000/rs6000.h
 +++ b/gcc/config/rs6000/rs6000.h
-@@ -1728,9 +1728,12 @@ extern enum reg_class rs6000_constraints[RS6000_CONSTRAINT_MAX];
+@@ -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									\
@@ -90,14 +109,19 @@
  
  /* Offset from the stack pointer register to an item dynamically
     allocated on the stack, e.g., by `alloca'.
-@@ -1739,9 +1742,8 @@ extern enum reg_class rs6000_constraints[RS6000_CONSTRAINT_MAX];
+ 
+    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.  */
++   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),	\
++  RS6000_ALIGN (crtl->outgoing_args_size + STACK_POINTER_OFFSET,	\
 +		(TARGET_ALTIVEC || TARGET_VSX) ? 16 : 8)
  
  /* If we generate an insn to push BYTES bytes,

[-- Attachment #3: 0001-v2-ChangeLog --]
[-- Type: text/plain, Size: 338 bytes --]

gcc/ChangeLog

	* config/rs6000/rs6000.c (rs6000_stack_info): Properly align local
	variables in functions calling alloca.
	* config/rs6000/rs6000.h (STARTING_FRAME_OFFSET, STACK_DYNAMIC_OFFSET):
	Likewise.
	* config/rs6000/aix.h (STARTING_FRAME_OFFSET, STACK_DYNAMIC_OFFSET):
	Copy AIX specific versions of the rs6000.h macros to aix.h.

[-- Attachment #4: 0001-v2-PR77359-Properly-align-local-variables-in-functions-.patch --]
[-- Type: text/plain, Size: 5292 bytes --]

From faae30210f584bba92ab96aac479ae8f253e59b7 Mon Sep 17 00:00:00 2001
From: Dominik Vogt <vogt@linux.vnet.ibm.com>
Date: Fri, 28 Oct 2016 12:59:55 +0100
Subject: [PATCH] PR77359: Properly align local variables in functions
 calling alloca.

See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77359 for a discussion of the
problem and the fix.
---
 gcc/config/rs6000/aix.h    | 35 +++++++++++++++++++++++++++++++++++
 gcc/config/rs6000/rs6000.c |  9 +++++++--
 gcc/config/rs6000/rs6000.h | 26 ++++++++++++++++++--------
 3 files changed, 60 insertions(+), 10 deletions(-)

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..02ed9c1 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -26004,8 +26004,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.
-- 
2.3.0

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

[Dominik Vogt]

[-- Attachment #1: Type: text/plain, Size: 1187 bytes --]

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 ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

[-- Attachment #2: 0001-v3-ChangeLog --]
[-- Type: text/plain, Size: 401 bytes --]

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.

[-- Attachment #3: 0001-v3-PR-77359-Properly-align-local-variables-in-functions.patch --]
[-- Type: text/plain, Size: 7789 bytes --]

From a80234c45173bebbfa07e810ff534c60591a8b76 Mon Sep 17 00:00:00 2001
From: Dominik Vogt <vogt@linux.vnet.ibm.com>
Date: Fri, 28 Oct 2016 12:59:55 +0100
Subject: [PATCH] PR/77359: Properly align local variables in functions
 calling alloca.

See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77359 for a discussion of the
problem and the fix.
---
 gcc/config/rs6000/aix.h    | 35 +++++++++++++++++++++++++++++++++++
 gcc/config/rs6000/rs6000.c | 24 +++++++++++++++++++-----
 gcc/config/rs6000/rs6000.h | 26 ++++++++++++++++++--------
 3 files changed, 72 insertions(+), 13 deletions(-)

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

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

[Segher Boessenkool]

On Fri, Nov 11, 2016 at 09:58:21AM +0100, Dominik Vogt wrote:
> > You say it needs more testing -- what testing?
> 
> Regression testing on AIX (David has done this in reply to the
> original message), possibly also on 32-Bit Power, if that is a
> reasonable target.

It is.  I'll test powerpc64-linux -m32, and a crosscompiler for
powerpc-linux.

Thanks for the patch.  Please apply to trunk.  Does it need backports
later?


Segher

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

[Segher Boessenkool]

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

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

[Dominik Vogt]

On Fri, Nov 11, 2016 at 02:17:58PM -0600, Segher Boessenkool wrote:
> On Fri, Nov 11, 2016 at 09:58:21AM +0100, Dominik Vogt wrote:
> > > You say it needs more testing -- what testing?
> > 
> > Regression testing on AIX (David has done this in reply to the
> > original message), possibly also on 32-Bit Power, if that is a
> > reasonable target.
> 
> It is.  I'll test powerpc64-linux -m32, and a crosscompiler for
> powerpc-linux.
> 
> Thanks for the patch.  Please apply to trunk.  Does it need backports
> later?

Depends on whether the Power/AIX folks think backports are
necessary.  We'll apply the patch to trunk; if a backport is
desired later, David or someone else from IBM could do that (i.e.
it's not necessary that our department commits further patches).

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

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

[Andreas Krebbel]

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-