[PATCH] Fix ICE with __builtin_stack_save (PR c/90898)

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Fix ICE with __builtin_stack_save (PR c/90898)

Jakub Jelinek
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  <[hidden email]>

        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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix ICE with __builtin_stack_save (PR c/90898)

Jeff Law
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  <[hidden email]>
>
> 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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix ICE with __builtin_stack_save (PR c/90898)

Richard Biener
In reply to this post by Jakub Jelinek
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  <[hidden email]>
>
> 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
>
>
--
Richard Biener <[hidden email]>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)