Fix ICE with __builtin_stack_save (PR c/90898)
diff mbox series

Message ID 20191119235827.GS4650@tucnak
State New
Headers show
Series
  • Fix ICE with __builtin_stack_save (PR c/90898)
Related show

Commit Message

Jakub Jelinek Nov. 19, 2019, 11:58 p.m. UTC
Hi!

I agree that we shouldn't have made __builtin_stack_{save,restore} public,
but I'd fear it is too late to remove it now (and replace by internal fn),
I'd think some code might use it to control when e.g. alloca will be
released.  As the comment on insert_clobber* says, the addition of the
clobbers is a best effort, we could end up not finding the right spot and
not adding the CLOBBER in there even without user __builtin_* calls, so
this patch just removes an unnecessary assertion and just will not find
__builtin_stack_restore if something weird is seen, such as in the testcase
storing of the __builtin_stack_save result into memory, or could be
a cast or whatever else too.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2019-11-19  Jakub Jelinek  <jakub@redhat.com>

	PR c/90898
	* tree-ssa-ccp.c (insert_clobber_before_stack_restore): Remove
	assertion.
	(insert_clobbers_for_var): Fix a typo in function comment.

	* gcc.dg/pr90898.c: New test.


	Jakub

Comments

Jeff Law Nov. 20, 2019, 2:33 a.m. UTC | #1
On 11/19/19 4:58 PM, Jakub Jelinek wrote:
> Hi!
> 
> I agree that we shouldn't have made __builtin_stack_{save,restore} public,
> but I'd fear it is too late to remove it now (and replace by internal fn),
> I'd think some code might use it to control when e.g. alloca will be
> released.  As the comment on insert_clobber* says, the addition of the
> clobbers is a best effort, we could end up not finding the right spot and
> not adding the CLOBBER in there even without user __builtin_* calls, so
> this patch just removes an unnecessary assertion and just will not find
> __builtin_stack_restore if something weird is seen, such as in the testcase
> storing of the __builtin_stack_save result into memory, or could be
> a cast or whatever else too.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2019-11-19  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c/90898
> 	* tree-ssa-ccp.c (insert_clobber_before_stack_restore): Remove
> 	assertion.
> 	(insert_clobbers_for_var): Fix a typo in function comment.
> 
> 	* gcc.dg/pr90898.c: New test.
OK
jeff
Richard Biener Nov. 20, 2019, 7:30 a.m. UTC | #2
On Wed, 20 Nov 2019, Jakub Jelinek wrote:

> Hi!
> 
> I agree that we shouldn't have made __builtin_stack_{save,restore} public,
> but I'd fear it is too late to remove it now (and replace by internal fn),
> I'd think some code might use it to control when e.g. alloca will be
> released.  As the comment on insert_clobber* says, the addition of the
> clobbers is a best effort, we could end up not finding the right spot and
> not adding the CLOBBER in there even without user __builtin_* calls, so
> this patch just removes an unnecessary assertion and just will not find
> __builtin_stack_restore if something weird is seen, such as in the testcase
> storing of the __builtin_stack_save result into memory, or could be
> a cast or whatever else too.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.

Thanks,
Richard.

> 2019-11-19  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c/90898
> 	* tree-ssa-ccp.c (insert_clobber_before_stack_restore): Remove
> 	assertion.
> 	(insert_clobbers_for_var): Fix a typo in function comment.
> 
> 	* gcc.dg/pr90898.c: New test.
> 
> --- gcc/tree-ssa-ccp.c.jj	2019-11-13 10:54:47.093020613 +0100
> +++ gcc/tree-ssa-ccp.c	2019-11-19 19:13:35.332705130 +0100
> @@ -2114,8 +2114,6 @@ insert_clobber_before_stack_restore (tre
>      else if (gimple_assign_ssa_name_copy_p (stmt))
>        insert_clobber_before_stack_restore (gimple_assign_lhs (stmt), var,
>  					   visited);
> -    else
> -      gcc_assert (is_gimple_debug (stmt));
>  }
>  
>  /* Advance the iterator to the previous non-debug gimple statement in the same
> @@ -2140,9 +2138,9 @@ gsi_prev_dom_bb_nondebug (gimple_stmt_it
>  /* Find a BUILT_IN_STACK_SAVE dominating gsi_stmt (I), and insert
>     a clobber of VAR before each matching BUILT_IN_STACK_RESTORE.
>  
> -   It is possible that BUILT_IN_STACK_SAVE cannot be find in a dominator when a
> -   previous pass (such as DOM) duplicated it along multiple paths to a BB.  In
> -   that case the function gives up without inserting the clobbers.  */
> +   It is possible that BUILT_IN_STACK_SAVE cannot be found in a dominator when
> +   a previous pass (such as DOM) duplicated it along multiple paths to a BB.
> +   In that case the function gives up without inserting the clobbers.  */
>  
>  static void
>  insert_clobbers_for_var (gimple_stmt_iterator i, tree var)
> --- gcc/testsuite/gcc.dg/pr90898.c.jj	2019-11-19 19:18:02.277712801 +0100
> +++ gcc/testsuite/gcc.dg/pr90898.c	2019-11-19 19:18:52.613959787 +0100
> @@ -0,0 +1,16 @@
> +/* PR c/90898 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +void *p;
> +int bar (void);
> +void baz (int *);
> +
> +void
> +foo (void)
> +{
> +  p = __builtin_stack_save ();
> +  int a[(bar (), 2)];
> +  baz (a);
> +  __builtin_stack_restore (p);
> +}
> 
> 	Jakub
> 
>

Patch
diff mbox series

--- gcc/tree-ssa-ccp.c.jj	2019-11-13 10:54:47.093020613 +0100
+++ gcc/tree-ssa-ccp.c	2019-11-19 19:13:35.332705130 +0100
@@ -2114,8 +2114,6 @@  insert_clobber_before_stack_restore (tre
     else if (gimple_assign_ssa_name_copy_p (stmt))
       insert_clobber_before_stack_restore (gimple_assign_lhs (stmt), var,
 					   visited);
-    else
-      gcc_assert (is_gimple_debug (stmt));
 }
 
 /* Advance the iterator to the previous non-debug gimple statement in the same
@@ -2140,9 +2138,9 @@  gsi_prev_dom_bb_nondebug (gimple_stmt_it
 /* Find a BUILT_IN_STACK_SAVE dominating gsi_stmt (I), and insert
    a clobber of VAR before each matching BUILT_IN_STACK_RESTORE.
 
-   It is possible that BUILT_IN_STACK_SAVE cannot be find in a dominator when a
-   previous pass (such as DOM) duplicated it along multiple paths to a BB.  In
-   that case the function gives up without inserting the clobbers.  */
+   It is possible that BUILT_IN_STACK_SAVE cannot be found in a dominator when
+   a previous pass (such as DOM) duplicated it along multiple paths to a BB.
+   In that case the function gives up without inserting the clobbers.  */
 
 static void
 insert_clobbers_for_var (gimple_stmt_iterator i, tree var)
--- gcc/testsuite/gcc.dg/pr90898.c.jj	2019-11-19 19:18:02.277712801 +0100
+++ gcc/testsuite/gcc.dg/pr90898.c	2019-11-19 19:18:52.613959787 +0100
@@ -0,0 +1,16 @@ 
+/* PR c/90898 */
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+void *p;
+int bar (void);
+void baz (int *);
+
+void
+foo (void)
+{
+  p = __builtin_stack_save ();
+  int a[(bar (), 2)];
+  baz (a);
+  __builtin_stack_restore (p);
+}