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] 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] PR77359: Properly align local variables in functions calling alloca.

[Dominik Vogt]

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

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

[David Edelsohn]

On Thu, Nov 10, 2016 at 6:47 PM, 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.)

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.

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

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

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

[David Edelsohn]

[Please cc me and Segher on PowerPC / AIX patches.]

I bootstrapped successfully with the patch and ran the testsuite.

Without patch:
https://gcc.gnu.org/ml/gcc-testresults/2016-11/msg00186.html

With patch:
https://gcc.gnu.org/ml/gcc-testresults/2016-11/msg00233.html

Not exactly the same revision, but close enough and no differences.

The patch doesn't break anything, but we still need to understand if
it is correct.

Thanks, David