| Message ID | CACqP_wypCZ_VZZCszN2mEhCgRg=1uXW4-0h7xzmtz6zZC-hR7w@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 01/14/2016 02:19 PM, Ryan Burn wrote: > This patch adds a missing cleanup point to cilk_spawn expressions to > prevent an ICE when calling functions that return types with > non-trivial destructors. > > Bootstrapped and regression tested on x86_64-linux. > > 2015-01-14 Ryan Burn <contact@rnburn.com> > > PR c++/69048 > * cilk.c (create_cilk_wrapper_body): Call > fold_build_cleanup_point_expr to add missing cleanup point. > > * gcc/testsuite/g++.dg/cilk-plus/CK/pr69048.cc: New test I thought something was horribly wrong for a few minutes. STMT inside cilk_create_wrapper_body is a tree, not a gimple statement. So my worries were unfounded :-) OK for the trunk. jeff
Could you push please? I don't have write access. Thanks - Ryan. On Thu, Jan 14, 2016 at 4:57 PM, Jeff Law <law@redhat.com> wrote: > On 01/14/2016 02:19 PM, Ryan Burn wrote: >> >> This patch adds a missing cleanup point to cilk_spawn expressions to >> prevent an ICE when calling functions that return types with >> non-trivial destructors. >> >> Bootstrapped and regression tested on x86_64-linux. >> >> 2015-01-14 Ryan Burn <contact@rnburn.com> >> >> PR c++/69048 >> * cilk.c (create_cilk_wrapper_body): Call >> fold_build_cleanup_point_expr to add missing cleanup point. >> >> * gcc/testsuite/g++.dg/cilk-plus/CK/pr69048.cc: New test > > I thought something was horribly wrong for a few minutes. STMT inside > cilk_create_wrapper_body is a tree, not a gimple statement. So my worries > were unfounded :-) > > OK for the trunk. > jeff
On 01/14/2016 03:14 PM, Ryan Burn wrote:
> Could you push please? I don't have write access. Thanks - Ryan.
Will do.
Do you *want* write access? It's pretty easy to set up.
jeff
On Thu, Jan 14, 2016 at 02:57:06PM -0700, Jeff Law wrote: > On 01/14/2016 02:19 PM, Ryan Burn wrote: > >This patch adds a missing cleanup point to cilk_spawn expressions to > >prevent an ICE when calling functions that return types with > >non-trivial destructors. > > > >Bootstrapped and regression tested on x86_64-linux. > > > >2015-01-14 Ryan Burn <contact@rnburn.com> > > > > PR c++/69048 > > * cilk.c (create_cilk_wrapper_body): Call > >fold_build_cleanup_point_expr to add missing cleanup point. > > > > * gcc/testsuite/g++.dg/cilk-plus/CK/pr69048.cc: New test > I thought something was horribly wrong for a few minutes. STMT inside > cilk_create_wrapper_body is a tree, not a gimple statement. So my worries > were unfounded :-) > > OK for the trunk. Note the ChangeLog entry is badly formatted (unless the MUA broke it). There should be two spaces in between date and name instead of one, and the PR line as well as all the other lines of entries should be tab indented, and each description should end with full stop, and gcc/testsuite/ has its own ChangeLog, so that part should go into that ChangeLog file and be without prefix. 2015-01-14 Ryan Burn <contact@rnburn.com> PR c++/69048 * cilk.c (create_cilk_wrapper_body): Call fold_build_cleanup_point_expr to add missing cleanup point. * g++.dg/cilk-plus/CK/pr69048.cc: New test. The /* The statement containing the spawn expression might create temporaries with line is (1 char) too long, so you want to reformat that comment. Jakub
On Thu, Jan 14, 2016 at 5:34 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Thu, Jan 14, 2016 at 02:57:06PM -0700, Jeff Law wrote: >> On 01/14/2016 02:19 PM, Ryan Burn wrote: >> >This patch adds a missing cleanup point to cilk_spawn expressions to >> >prevent an ICE when calling functions that return types with >> >non-trivial destructors. >> > >> >Bootstrapped and regression tested on x86_64-linux. >> > >> >2015-01-14 Ryan Burn <contact@rnburn.com> >> > >> > PR c++/69048 >> > * cilk.c (create_cilk_wrapper_body): Call >> >fold_build_cleanup_point_expr to add missing cleanup point. >> > >> > * gcc/testsuite/g++.dg/cilk-plus/CK/pr69048.cc: New test >> I thought something was horribly wrong for a few minutes. STMT inside >> cilk_create_wrapper_body is a tree, not a gimple statement. So my worries >> were unfounded :-) >> >> OK for the trunk. > > Note the ChangeLog entry is badly formatted (unless the MUA broke it). There > should be two spaces in between date and name instead of one, and the PR > line as well as all the other lines of entries should be tab indented, > and each description should end with full stop, and gcc/testsuite/ has its > own ChangeLog, so that part should go into that ChangeLog file and be > without prefix. > > 2015-01-14 Ryan Burn <contact@rnburn.com> > > PR c++/69048 > * cilk.c (create_cilk_wrapper_body): Call > fold_build_cleanup_point_expr to add missing cleanup point. > > * g++.dg/cilk-plus/CK/pr69048.cc: New test. > > The > /* The statement containing the spawn expression might create temporaries with > line is (1 char) too long, so you want to reformat that comment. > > Jakub Are you sure? It passed the check_GNU_style.sh script. If you exclude the extra + added in the diff, it's within 80 characters.
On 01/14/2016 03:34 PM, Jakub Jelinek wrote: > On Thu, Jan 14, 2016 at 02:57:06PM -0700, Jeff Law wrote: >> On 01/14/2016 02:19 PM, Ryan Burn wrote: >>> This patch adds a missing cleanup point to cilk_spawn expressions to >>> prevent an ICE when calling functions that return types with >>> non-trivial destructors. >>> >>> Bootstrapped and regression tested on x86_64-linux. >>> >>> 2015-01-14 Ryan Burn <contact@rnburn.com> >>> >>> PR c++/69048 >>> * cilk.c (create_cilk_wrapper_body): Call >>> fold_build_cleanup_point_expr to add missing cleanup point. >>> >>> * gcc/testsuite/g++.dg/cilk-plus/CK/pr69048.cc: New test >> I thought something was horribly wrong for a few minutes. STMT inside >> cilk_create_wrapper_body is a tree, not a gimple statement. So my worries >> were unfounded :-) >> >> OK for the trunk. > > Note the ChangeLog entry is badly formatted (unless the MUA broke it). There > should be two spaces in between date and name instead of one, and the PR > line as well as all the other lines of entries should be tab indented, > and each description should end with full stop, and gcc/testsuite/ has its > own ChangeLog, so that part should go into that ChangeLog file and be > without prefix. I had actually fixed most of the ChangeLog nits before pushing. I missed the two space thingie :( > > 2015-01-14 Ryan Burn <contact@rnburn.com> > > PR c++/69048 > * cilk.c (create_cilk_wrapper_body): Call > fold_build_cleanup_point_expr to add missing cleanup point. > > * g++.dg/cilk-plus/CK/pr69048.cc: New test. > > The > /* The statement containing the spawn expression might create temporaries with > line is (1 char) too long, so you want to reformat that comment. It ends right at 80 columns. Arguably we should have everything ending at 79. Jeff
On Thu, Jan 14, 2016 at 05:41:25PM -0500, Ryan Burn wrote: > > The > > /* The statement containing the spawn expression might create temporaries with > > line is (1 char) too long, so you want to reformat that comment. > > > > Jakub > > Are you sure? It passed the check_GNU_style.sh script. If you exclude > the extra + added in the diff, it's within 80 characters. You're right, sorry. echo -n ' /* The statement containing the spawn expression might create temporaries with' | wc -c 80 Jakub
Index: gcc/c-family/cilk.c =================================================================== --- gcc/c-family/cilk.c (revision 232359) +++ gcc/c-family/cilk.c (working copy) @@ -592,6 +592,11 @@ for (p = wd->parms; p; p = TREE_CHAIN (p)) DECL_CONTEXT (p) = fndecl; + /* The statement containing the spawn expression might create temporaries with + destructors defined; if so we need to add a CLEANUP_POINT_EXPR to ensure + the expression is properly gimplified. */ + stmt = fold_build_cleanup_point_expr (void_type_node, stmt); + gcc_assert (!DECL_SAVED_TREE (fndecl)); cilk_install_body_with_frame_cleanup (fndecl, stmt, (void *) wd); gcc_assert (DECL_SAVED_TREE (fndecl)); Index: gcc/testsuite/g++.dg/cilk-plus/CK/pr69048.cc =================================================================== --- gcc/testsuite/g++.dg/cilk-plus/CK/pr69048.cc (revision 0) +++ gcc/testsuite/g++.dg/cilk-plus/CK/pr69048.cc (working copy) @@ -0,0 +1,14 @@ +/* { dg-do compile } */ +/* { dg-options "-fcilkplus" } */ + +struct A { + ~A () {} +}; + +A f () { + return A (); +} + +void t1 () { + _Cilk_spawn f (); +}
This patch adds a missing cleanup point to cilk_spawn expressions to prevent an ICE when calling functions that return types with non-trivial destructors. Bootstrapped and regression tested on x86_64-linux. 2015-01-14 Ryan Burn <contact@rnburn.com> PR c++/69048 * cilk.c (create_cilk_wrapper_body): Call fold_build_cleanup_point_expr to add missing cleanup point. * gcc/testsuite/g++.dg/cilk-plus/CK/pr69048.cc: New test