[patch] Fix PR middle-end/65958

[patch] Fix PR middle-end/65958

[Eric Botcazou]

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

Hi,

this is the underlying issue of PR middle-end/65958: the compiler generates 
wrong code when alloca is used in conjunction with a VLA since the latter 
causes the stack space allocated to the former to be wrongly reclaimed.
So the proposed fix is not to reclaim any stack space in this case.

Bootstrapped/regtested on x86_64-suse-linux, OK for mainline and 5.x branch?


2015-11-18  Eric Botcazou  <ebotcazou@adacore.com>

	PR middle-end/65958
	* gimplify.c (struct gimplify_ctx): Turn boolean fields into 1-bit
	fields, add keep_stack and reorder them.
	(gimplify_bind_expr): Save gimplify_ctxp->keep_stack on entry then
	set it to false.  Do not insert a stack save/restore pair if it has
	been set to true by the gimplification of the statements.
	Restore it to the saved value on exit if it is still false.
	(gimplify_vla_decl): Do not set gimplify_ctxp->save_stack here.
	(gimplify_call_expr) <BUILT_IN_ALLOCA[_WITH_ALIGN]>: New case.  Set
	either save_stack or keep_stack depending on CALL_ALLOCA_FOR_VAR_P.


2015-11-18  Eric Botcazou  <ebotcazou@adacore.com>

	* gcc.dg/vla-24.c: New test.

-- 
Eric Botcazou

[-- Attachment #2: vla-24.c --]
[-- Type: text/x-csrc, Size: 378 bytes --]

/* PR middle-end/65958 */

/* { dg-do run } */
/* { dg-options "-std=gnu99" } */

extern void abort (void);

int foo (int n)
{
  char *p, *q;

  if (1)
    {
      char i[n];
      p = __builtin_alloca (8);
      p[0] = 1;
    }

  q = __builtin_alloca (64);
  __builtin_memset (q, 0, 64);

  return !p[0];
}

int main (void)
{
  if (foo (48) != 0)
    abort ();

  return 0;
}

[-- Attachment #3: pr65958-3.diff --]
[-- Type: text/x-patch, Size: 3122 bytes --]

Index: gimplify.c
===================================================================
--- gimplify.c	(revision 230453)
+++ gimplify.c	(working copy)
@@ -148,10 +148,11 @@ struct gimplify_ctx
   hash_table<gimplify_hasher> *temp_htab;
 
   int conditions;
-  bool save_stack;
-  bool into_ssa;
-  bool allow_rhs_cond_expr;
-  bool in_cleanup_point_expr;
+  unsigned into_ssa : 1;
+  unsigned allow_rhs_cond_expr : 1;
+  unsigned in_cleanup_point_expr : 1;
+  unsigned keep_stack : 1;
+  unsigned save_stack : 1;
 };
 
 struct gimplify_omp_ctx
@@ -1073,6 +1074,7 @@ static enum gimplify_status
 gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p)
 {
   tree bind_expr = *expr_p;
+  bool old_keep_stack = gimplify_ctxp->keep_stack;
   bool old_save_stack = gimplify_ctxp->save_stack;
   tree t;
   gbind *bind_stmt;
@@ -1122,9 +1124,10 @@ gimplify_bind_expr (tree *expr_p, gimple
     }
 
   bind_stmt = gimple_build_bind (BIND_EXPR_VARS (bind_expr), NULL,
-                                   BIND_EXPR_BLOCK (bind_expr));
+				 BIND_EXPR_BLOCK (bind_expr));
   gimple_push_bind_expr (bind_stmt);
 
+  gimplify_ctxp->keep_stack = false;
   gimplify_ctxp->save_stack = false;
 
   /* Gimplify the body into the GIMPLE_BIND tuple's body.  */
@@ -1147,7 +1150,10 @@ gimplify_bind_expr (tree *expr_p, gimple
 
   cleanup = NULL;
   stack_save = NULL;
-  if (gimplify_ctxp->save_stack)
+
+  /* If the code both contains VLAs and calls alloca, then we cannot reclaim
+     the stack space allocated to the VLAs.  */
+  if (gimplify_ctxp->save_stack && !gimplify_ctxp->keep_stack)
     {
       gcall *stack_restore;
 
@@ -1229,7 +1235,11 @@ gimplify_bind_expr (tree *expr_p, gimple
       gimple_bind_set_body (bind_stmt, new_body);
     }
 
+  /* keep_stack propagates all the way up to the outermost BIND_EXPR.  */
+  if (!gimplify_ctxp->keep_stack)
+    gimplify_ctxp->keep_stack = old_keep_stack;
   gimplify_ctxp->save_stack = old_save_stack;
+
   gimple_pop_bind_expr ();
 
   gimplify_seq_add_stmt (pre_p, bind_stmt);
@@ -1386,10 +1396,6 @@ gimplify_vla_decl (tree decl, gimple_seq
   t = build2 (MODIFY_EXPR, TREE_TYPE (addr), addr, t);
 
   gimplify_and_add (t, seq_p);
-
-  /* Indicate that we need to restore the stack level when the
-     enclosing BIND_EXPR is exited.  */
-  gimplify_ctxp->save_stack = true;
 }
 
 /* A helper function to be called via walk_tree.  Mark all labels under *TP
@@ -2370,6 +2376,18 @@ gimplify_call_expr (tree *expr_p, gimple
       && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
     switch (DECL_FUNCTION_CODE (fndecl))
       {
+      case BUILT_IN_ALLOCA:
+      case BUILT_IN_ALLOCA_WITH_ALIGN:
+	/* If the call has been built for a variable-sized object, then we
+	   want to restore the stack level when the enclosing BIND_EXPR is
+	   exited to reclaim the allocated space; otherwise, we precisely
+	   need to do the opposite and preserve the latest stack level.  */
+	if (CALL_ALLOCA_FOR_VAR_P (*expr_p))
+	  gimplify_ctxp->save_stack = true;
+	else
+	  gimplify_ctxp->keep_stack = true;
+	break;
+
       case BUILT_IN_VA_START:
         {
 	  builtin_va_start_p = TRUE;

Re: [patch] Fix PR middle-end/65958

[Richard Biener]

On Wed, Nov 18, 2015 at 11:06 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> this is the underlying issue of PR middle-end/65958: the compiler generates
> wrong code when alloca is used in conjunction with a VLA since the latter
> causes the stack space allocated to the former to be wrongly reclaimed.
> So the proposed fix is not to reclaim any stack space in this case.
>
> Bootstrapped/regtested on x86_64-suse-linux, OK for mainline and 5.x branch?

Ok.  I wonder if we document GCCs VLA implementation somewhere so we can add
a note on the interaction with alloca.

Thanks,
Richard.

>
> 2015-11-18  Eric Botcazou  <ebotcazou@adacore.com>
>
>         PR middle-end/65958
>         * gimplify.c (struct gimplify_ctx): Turn boolean fields into 1-bit
>         fields, add keep_stack and reorder them.
>         (gimplify_bind_expr): Save gimplify_ctxp->keep_stack on entry then
>         set it to false.  Do not insert a stack save/restore pair if it has
>         been set to true by the gimplification of the statements.
>         Restore it to the saved value on exit if it is still false.
>         (gimplify_vla_decl): Do not set gimplify_ctxp->save_stack here.
>         (gimplify_call_expr) <BUILT_IN_ALLOCA[_WITH_ALIGN]>: New case.  Set
>         either save_stack or keep_stack depending on CALL_ALLOCA_FOR_VAR_P.
>
>
> 2015-11-18  Eric Botcazou  <ebotcazou@adacore.com>
>
>         * gcc.dg/vla-24.c: New test.
>
> --
> Eric Botcazou

Re: [patch] Fix PR middle-end/65958

[Eric Botcazou]

> Ok.  I wonder if we document GCCs VLA implementation somewhere so we can add
> a note on the interaction with alloca.

I found this in 10.7.2 Blocks:

"Variable-length arrays (VLAs) complicate this process, as their size
often refers to variables initialized earlier in the block.  To handle
this, we currently split the block at that point, and move the VLA into
a new, inner `BIND_EXPR'.  This strategy may change in the future."

which sounds totally obsolete to me.  Proposed change:

Index: doc/generic.texi
===================================================================
--- doc/generic.texi    (revision 230453)
+++ doc/generic.texi    (working copy)
@@ -1950,11 +1950,15 @@ this initialization replaces the @code{D
 will never require cleanups.  The scope of these variables is just the
 body
 
-Variable-length arrays (VLAs) complicate this process, as their
-size often refers to variables initialized earlier in the block.
-To handle this, we currently split the block at that point, and
-move the VLA into a new, inner @code{BIND_EXPR}.  This strategy
-may change in the future.
+Variable-length arrays (VLAs) complicate this process, as their size
+often refers to variables initialized earlier in the block and their
+initialization involves an explicit stack allocation.  To handle this,
+we add an indirection and replace them with a pointer to stack space
+allocated by means of @code{alloca}.  In most cases, we also arrange
+for this space to be reclaimed when the enclosing @code{BIND_EXPR} is
+exited, the exception to this being when there is an explicit call to
+@code{alloca} in the source code, in which case the stack is left
+depressed on exit of the @code{BIND_EXPR}.
 
 A C++ program will usually contain more @code{BIND_EXPR}s than
 there are syntactic blocks in the source code, since several C++


-- 
Eric Botcazou

Re: [patch] Fix PR middle-end/65958

[Richard Biener]

On Wed, Nov 18, 2015 at 12:12 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Ok.  I wonder if we document GCCs VLA implementation somewhere so we can add
>> a note on the interaction with alloca.
>
> I found this in 10.7.2 Blocks:
>
> "Variable-length arrays (VLAs) complicate this process, as their size
> often refers to variables initialized earlier in the block.  To handle
> this, we currently split the block at that point, and move the VLA into
> a new, inner `BIND_EXPR'.  This strategy may change in the future."
>
> which sounds totally obsolete to me.  Proposed change:

Looks good to me.  I also found the Arrays of Variable Length section
in extend.texi which also refers to alloca as doing the same.  We may
want to add a note there that you should not mix both and that only
VLAs (when not mixed with alloca) are freed at scope boundary.

Richard.

> Index: doc/generic.texi
> ===================================================================
> --- doc/generic.texi    (revision 230453)
> +++ doc/generic.texi    (working copy)
> @@ -1950,11 +1950,15 @@ this initialization replaces the @code{D
>  will never require cleanups.  The scope of these variables is just the
>  body
>
> -Variable-length arrays (VLAs) complicate this process, as their
> -size often refers to variables initialized earlier in the block.
> -To handle this, we currently split the block at that point, and
> -move the VLA into a new, inner @code{BIND_EXPR}.  This strategy
> -may change in the future.
> +Variable-length arrays (VLAs) complicate this process, as their size
> +often refers to variables initialized earlier in the block and their
> +initialization involves an explicit stack allocation.  To handle this,
> +we add an indirection and replace them with a pointer to stack space
> +allocated by means of @code{alloca}.  In most cases, we also arrange
> +for this space to be reclaimed when the enclosing @code{BIND_EXPR} is
> +exited, the exception to this being when there is an explicit call to
> +@code{alloca} in the source code, in which case the stack is left
> +depressed on exit of the @code{BIND_EXPR}.
>
>  A C++ program will usually contain more @code{BIND_EXPR}s than
>  there are syntactic blocks in the source code, since several C++
>
>
> --
> Eric Botcazou

Re: [patch] Fix PR middle-end/65958

[Eric Botcazou]

> Looks good to me.  I also found the Arrays of Variable Length section
> in extend.texi which also refers to alloca as doing the same.  We may
> want to add a note there that you should not mix both and that only
> VLAs (when not mixed with alloca) are freed at scope boundary.

It's already there and in fact the current behavior is documented:

 There are other differences between these two methods.  Space allocated
with `alloca' exists until the containing _function_ returns.  The
space for a variable-length array is deallocated as soon as the array
name's scope ends.  (If you use both variable-length arrays and
`alloca' in the same function, deallocation of a variable-length array
also deallocates anything more recently allocated with `alloca'.)

so we need to amend the documentation if we go for the patch.

-- 
Eric Botcazou

Re: [patch] Fix PR middle-end/65958

[Richard Biener]

On Wed, Nov 18, 2015 at 3:28 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Looks good to me.  I also found the Arrays of Variable Length section
>> in extend.texi which also refers to alloca as doing the same.  We may
>> want to add a note there that you should not mix both and that only
>> VLAs (when not mixed with alloca) are freed at scope boundary.
>
> It's already there and in fact the current behavior is documented:
>
>  There are other differences between these two methods.  Space allocated
> with `alloca' exists until the containing _function_ returns.  The
> space for a variable-length array is deallocated as soon as the array
> name's scope ends.  (If you use both variable-length arrays and
> `alloca' in the same function, deallocation of a variable-length array
> also deallocates anything more recently allocated with `alloca'.)
>
> so we need to amend the documentation if we go for the patch.

Ah.  I think the patch is good and we should reflect this in the
documentation.

Richard.

> --
> Eric Botcazou

Re: [patch] Fix PR middle-end/65958

[Eric Botcazou]

> Ah.  I think the patch is good and we should reflect this in the
> documentation.

OK, will do, thanks.

-- 
Eric Botcazou