[cilkplus] Fix cilk_spawn gimplification bug (PR cilkplus/69048)

[cilkplus] Fix cilk_spawn gimplification bug (PR cilkplus/69048)

[Ryan Burn]

[-- Attachment #1: Type: text/plain, Size: 445 bytes --]

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

[-- Attachment #2: pr69048.diff --]
[-- Type: text/plain, Size: 1131 bytes --]

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 ();
+}

Re: [cilkplus] Fix cilk_spawn gimplification bug (PR cilkplus/69048)

[Jeff Law]

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

Re: [cilkplus] Fix cilk_spawn gimplification bug (PR cilkplus/69048)

[Ryan Burn]

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

Re: [cilkplus] Fix cilk_spawn gimplification bug (PR cilkplus/69048)

[Jeff Law]

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

Re: [cilkplus] Fix cilk_spawn gimplification bug (PR cilkplus/69048)

[Jakub Jelinek]

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

Re: [cilkplus] Fix cilk_spawn gimplification bug (PR cilkplus/69048)

[Ryan Burn]

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.

Re: [cilkplus] Fix cilk_spawn gimplification bug (PR cilkplus/69048)

[Jeff Law]

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

Re: [cilkplus] Fix cilk_spawn gimplification bug (PR cilkplus/69048)

[Jakub Jelinek]

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