[PATCH] Use ECF_MAY_BE_ALLOCA for __builtin_alloca_with_align (PR tree-optimization/68680)

[PATCH] Use ECF_MAY_BE_ALLOCA for __builtin_alloca_with_align (PR tree-optimization/68680)

[Jakub Jelinek]

Hi!

As mentioned in the PR, GCC 4.7+ seems to have regressed for
-fstack-protector*, functions containing VLAs and no other arrays are not
protected anymore.  Before 4.7, VLAs were gimplified as __builtin_alloca
call, which sets ECF_MAY_BE_ALLOCA and in turn cfun->calls_alloca.
These two are used in various places:
1) for stack protector purposes (this issue), early during expansion
2) in the inliner
3) for tail call optimization
4) for some non-NULL optimizations
and tons of places in RTL.  As 4.7+ emits __builtin_alloca_with_align
instead and special_function_p has not been adjusted, this does not happen
any longer, though cfun->calls_alloca gets set during the expansion of
__builtin_alloca_with_align, so for RTL optimizers it is already set.

The following patch restores the previous behavior, making VLAs be
ECF_MAY_BE_ALLOCA and cfun->calls_alloca already during GIMPLE passes.
It could be also done by testing the name, but I thought that it would be
too ugly (would need another case anyway, as the current tests are for
names with length <= 16).

1) and 4) surely want to treat the VLAs like the patch does, I'm not 100%
sure about 2) and 3), as VLAs are slightly different, they release
the stack afterwards at the end of scope of the VLA var.  If we wanted to
treat the two differently, maybe we'd need another ECF* flag and another
cfun bitfield for VLAs.

The following patch has been bootstrapped/regtested on x86_64-linux and
i686-linux.

2015-12-03  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/68680
	* calls.c (special_function_p): Return ECF_MAY_BE_ALLOCA for
	BUILT_IN_ALLOCA{,_WITH_ALIGN}.

	* gcc.target/i386/pr68680.c: New test.

--- gcc/calls.c.jj	2015-11-26 11:17:25.000000000 +0100
+++ gcc/calls.c	2015-12-03 19:03:59.342306457 +0100
@@ -553,6 +553,17 @@ special_function_p (const_tree fndecl, i
 	flags |= ECF_NORETURN;
     }
 
+  if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
+    switch (DECL_FUNCTION_CODE (fndecl))
+      {
+      case BUILT_IN_ALLOCA:
+      case BUILT_IN_ALLOCA_WITH_ALIGN:
+	flags |= ECF_MAY_BE_ALLOCA;
+	break;
+      default:
+	break;
+      }
+
   return flags;
 }
 
--- gcc/testsuite/gcc.target/i386/pr68680.c.jj	2015-12-03 19:10:14.836037923 +0100
+++ gcc/testsuite/gcc.target/i386/pr68680.c	2015-12-03 19:09:57.000000000 +0100
@@ -0,0 +1,15 @@
+/* PR tree-optimization/68680 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -fstack-protector-strong" } */
+
+int foo (char *);
+
+int
+bar (unsigned long x)
+{
+  char a[x];
+  return foo (a);
+}
+
+/* Verify that this function is stack protected.  */
+/* { dg-final { scan-assembler "stack_chk_fail" } } */

	Jakub

Re: [PATCH] Use ECF_MAY_BE_ALLOCA for __builtin_alloca_with_align (PR tree-optimization/68680)

[Richard Biener]

On Thu, 3 Dec 2015, Jakub Jelinek wrote:

> Hi!
> 
> As mentioned in the PR, GCC 4.7+ seems to have regressed for
> -fstack-protector*, functions containing VLAs and no other arrays are not
> protected anymore.  Before 4.7, VLAs were gimplified as __builtin_alloca
> call, which sets ECF_MAY_BE_ALLOCA and in turn cfun->calls_alloca.
> These two are used in various places:
> 1) for stack protector purposes (this issue), early during expansion
> 2) in the inliner
> 3) for tail call optimization
> 4) for some non-NULL optimizations
> and tons of places in RTL.  As 4.7+ emits __builtin_alloca_with_align
> instead and special_function_p has not been adjusted, this does not happen
> any longer, though cfun->calls_alloca gets set during the expansion of
> __builtin_alloca_with_align, so for RTL optimizers it is already set.
> 
> The following patch restores the previous behavior, making VLAs be
> ECF_MAY_BE_ALLOCA and cfun->calls_alloca already during GIMPLE passes.
> It could be also done by testing the name, but I thought that it would be
> too ugly (would need another case anyway, as the current tests are for
> names with length <= 16).
> 
> 1) and 4) surely want to treat the VLAs like the patch does, I'm not 100%
> sure about 2) and 3), as VLAs are slightly different, they release
> the stack afterwards at the end of scope of the VLA var.  If we wanted to
> treat the two differently, maybe we'd need another ECF* flag and another
> cfun bitfield for VLAs.
> 
> The following patch has been bootstrapped/regtested on x86_64-linux and
> i686-linux.

The patch is ok - it looks like you could have removed the
__builtin_alloca strcmp with it though.

Does the patch mean we inlined __builtin_alloca_with_align ()
functions?  We might run into the issue Eric fixed lately with
mixing alloca and VLAs (don't see the patch being committed though).

Richard.

> 2015-12-03  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/68680
> 	* calls.c (special_function_p): Return ECF_MAY_BE_ALLOCA for
> 	BUILT_IN_ALLOCA{,_WITH_ALIGN}.
> 
> 	* gcc.target/i386/pr68680.c: New test.
> 
> --- gcc/calls.c.jj	2015-11-26 11:17:25.000000000 +0100
> +++ gcc/calls.c	2015-12-03 19:03:59.342306457 +0100
> @@ -553,6 +553,17 @@ special_function_p (const_tree fndecl, i
>  	flags |= ECF_NORETURN;
>      }
>  
> +  if (DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL)
> +    switch (DECL_FUNCTION_CODE (fndecl))
> +      {
> +      case BUILT_IN_ALLOCA:
> +      case BUILT_IN_ALLOCA_WITH_ALIGN:
> +	flags |= ECF_MAY_BE_ALLOCA;
> +	break;
> +      default:
> +	break;
> +      }
> +
>    return flags;
>  }
>  
> --- gcc/testsuite/gcc.target/i386/pr68680.c.jj	2015-12-03 19:10:14.836037923 +0100
> +++ gcc/testsuite/gcc.target/i386/pr68680.c	2015-12-03 19:09:57.000000000 +0100
> @@ -0,0 +1,15 @@
> +/* PR tree-optimization/68680 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fstack-protector-strong" } */
> +
> +int foo (char *);
> +
> +int
> +bar (unsigned long x)
> +{
> +  char a[x];
> +  return foo (a);
> +}
> +
> +/* Verify that this function is stack protected.  */
> +/* { dg-final { scan-assembler "stack_chk_fail" } } */
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [PATCH] Use ECF_MAY_BE_ALLOCA for __builtin_alloca_with_align (PR tree-optimization/68680)

[Eric Botcazou]

> Does the patch mean we inlined __builtin_alloca_with_align ()
> functions?  We might run into the issue Eric fixed lately with
> mixing alloca and VLAs (don't see the patch being committed though).

But I'm about to do it (I was waiting for the approval of the aarch64 
specific part).  Note that I'm not really sure if we want it for 5.x too.

-- 
Eric Botcazou

Re: [PATCH] Use ECF_MAY_BE_ALLOCA for __builtin_alloca_with_align (PR tree-optimization/68680)

[Jakub Jelinek]

On Fri, Dec 04, 2015 at 10:30:38AM +0100, Richard Biener wrote:
> > The following patch has been bootstrapped/regtested on x86_64-linux and
> > i686-linux.
> 
> The patch is ok - it looks like you could have removed the
> __builtin_alloca strcmp with it though.

Ok, will remove the strcmp then.

> Does the patch mean we inlined __builtin_alloca_with_align ()
> functions?  We might run into the issue Eric fixed lately with

Yes, see testcase below.  4.7+ inlines it.  As for tail call optimization,
seems we are just lucky there (f4), as fab pass which is quite late
turns the __builtin_stack_restore into GIMPLE_NOP and tailc pass does not
ignore nops.  Shall I commit following patch to trunk to fix that up
(after committing this VLA fix of course)?

int f1 (char *);

static inline void
f2 (int x)
{
  char a[x];
  f1 (a);
}

void
f3 (int x)
{
  f2 (x);
  f2 (x);
  f2 (x);
  f2 (x);
}

int
f4 (int x)
{
  char a[x];
  return f1 (a);
}

2015-12-04  Jakub Jelinek  <jakub@redhat.com>

	* tree-tailcall.c (find_tail_calls): Ignore GIMPLE_NOPs.

--- gcc/tree-tailcall.c.jj	2015-11-04 11:12:17.000000000 +0100
+++ gcc/tree-tailcall.c	2015-12-04 11:43:01.296110941 +0100
@@ -412,9 +412,10 @@ find_tail_calls (basic_block bb, struct
     {
       stmt = gsi_stmt (gsi);
 
-      /* Ignore labels, returns, clobbers and debug stmts.  */
+      /* Ignore labels, returns, nops, clobbers and debug stmts.  */
       if (gimple_code (stmt) == GIMPLE_LABEL
 	  || gimple_code (stmt) == GIMPLE_RETURN
+	  || gimple_code (stmt) == GIMPLE_NOP
 	  || gimple_clobber_p (stmt)
 	  || is_gimple_debug (stmt))
 	continue;
@@ -532,7 +533,8 @@ find_tail_calls (basic_block bb, struct
 
       stmt = gsi_stmt (agsi);
 
-      if (gimple_code (stmt) == GIMPLE_LABEL)
+      if (gimple_code (stmt) == GIMPLE_LABEL
+	  || gimple_code (stmt) == GIMPLE_NOP)
 	continue;
 
       if (gimple_code (stmt) == GIMPLE_RETURN)


	Jakub

Re: [PATCH] Use ECF_MAY_BE_ALLOCA for __builtin_alloca_with_align (PR tree-optimization/68680)

[Richard Biener]

On Fri, 4 Dec 2015, Jakub Jelinek wrote:

> On Fri, Dec 04, 2015 at 10:30:38AM +0100, Richard Biener wrote:
> > > The following patch has been bootstrapped/regtested on x86_64-linux and
> > > i686-linux.
> > 
> > The patch is ok - it looks like you could have removed the
> > __builtin_alloca strcmp with it though.
> 
> Ok, will remove the strcmp then.
> 
> > Does the patch mean we inlined __builtin_alloca_with_align ()
> > functions?  We might run into the issue Eric fixed lately with
> 
> Yes, see testcase below.  4.7+ inlines it.  As for tail call optimization,
> seems we are just lucky there (f4), as fab pass which is quite late
> turns the __builtin_stack_restore into GIMPLE_NOP and tailc pass does not
> ignore nops.  Shall I commit following patch to trunk to fix that up
> (after committing this VLA fix of course)?

Yes please.

Thanks,
Richard.

> int f1 (char *);
> 
> static inline void
> f2 (int x)
> {
>   char a[x];
>   f1 (a);
> }
> 
> void
> f3 (int x)
> {
>   f2 (x);
>   f2 (x);
>   f2 (x);
>   f2 (x);
> }
> 
> int
> f4 (int x)
> {
>   char a[x];
>   return f1 (a);
> }
> 
> 2015-12-04  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* tree-tailcall.c (find_tail_calls): Ignore GIMPLE_NOPs.
> 
> --- gcc/tree-tailcall.c.jj	2015-11-04 11:12:17.000000000 +0100
> +++ gcc/tree-tailcall.c	2015-12-04 11:43:01.296110941 +0100
> @@ -412,9 +412,10 @@ find_tail_calls (basic_block bb, struct
>      {
>        stmt = gsi_stmt (gsi);
>  
> -      /* Ignore labels, returns, clobbers and debug stmts.  */
> +      /* Ignore labels, returns, nops, clobbers and debug stmts.  */
>        if (gimple_code (stmt) == GIMPLE_LABEL
>  	  || gimple_code (stmt) == GIMPLE_RETURN
> +	  || gimple_code (stmt) == GIMPLE_NOP
>  	  || gimple_clobber_p (stmt)
>  	  || is_gimple_debug (stmt))
>  	continue;
> @@ -532,7 +533,8 @@ find_tail_calls (basic_block bb, struct
>  
>        stmt = gsi_stmt (agsi);
>  
> -      if (gimple_code (stmt) == GIMPLE_LABEL)
> +      if (gimple_code (stmt) == GIMPLE_LABEL
> +	  || gimple_code (stmt) == GIMPLE_NOP)
>  	continue;
>  
>        if (gimple_code (stmt) == GIMPLE_RETURN)
> 
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)