| Message ID | 20191119235827.GS4650@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | Fix ICE with __builtin_stack_save (PR c/90898) | expand |
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
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 > >
--- 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); +}
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