Properly check for _Cilk_spawn in return stmt (PR c/60197)

I don't know Cilk stuff at all, but it seems that _Cilk_spawn is
forbidden in a return statement.  But then only checking
TREE_CODE (retval) == CILK_SPAWN_STMT isn't sufficient, because
CILK_SPAWN_STMT can be wrapped e.g. in MINUS_EXPR, C_MAYBE_CONST_EXPR,
EQ_EXPR, and a bunch of others.  I used walk_tree for checking whether
a return statement contains any CILK_SPAWN_STMT.

Regtested/bootstrapped on x86_64-linux, ok for 5.0?

2014-02-17  Marek Polacek  <polacek@redhat.com>

	PR c/60197
c-family/
	* array-notation-common.c (contains_cilk_spawn_stmt): New function.
	(contains_cilk_spawn_stmt_walker): Likewise.
	* c-common.h (contains_cilk_spawn_stmt): Add declaration.
c/
	* c-typeck.c (c_finish_return): Call contains_cilk_spawn_stmt instead
	of checking tree code.
cp/
	* typeck.c (check_return_expr): Call contains_cilk_spawn_stmt instead
	of checking tree code.
testsuite/
	* c-c++-common/cilk-plus/CK/pr60197.c: New test.


	Marek

Hi Marek,
	Thanks for working on this. Please see my comments below.

> -----Original Message-----
> From: Marek Polacek [mailto:polacek@redhat.com]
> Sent: Monday, February 17, 2014 12:43 PM
> To: GCC Patches
> Cc: Iyer, Balaji V
> Subject: [PATCH] Properly check for _Cilk_spawn in return stmt (PR c/60197)
> 
> I don't know Cilk stuff at all, but it seems that _Cilk_spawn is forbidden in a
> return statement.  But then only checking TREE_CODE (retval) ==
> CILK_SPAWN_STMT isn't sufficient, because CILK_SPAWN_STMT can be
> wrapped e.g. in MINUS_EXPR, C_MAYBE_CONST_EXPR, EQ_EXPR, and a
> bunch of others.  I used walk_tree for checking whether a return statement
> contains any CILK_SPAWN_STMT.
> 
> Regtested/bootstrapped on x86_64-linux, ok for 5.0?
>

5.0? you mean 4.9 right?... since this is a minor bug-fix.

 
> 2014-02-17  Marek Polacek  <polacek@redhat.com>
> 
> 	PR c/60197
> c-family/
> 	* array-notation-common.c (contains_cilk_spawn_stmt): New
> function.
> 	(contains_cilk_spawn_stmt_walker): Likewise.
> 	* c-common.h (contains_cilk_spawn_stmt): Add declaration.
> c/
> 	* c-typeck.c (c_finish_return): Call contains_cilk_spawn_stmt instead
> 	of checking tree code.
> cp/
> 	* typeck.c (check_return_expr): Call contains_cilk_spawn_stmt
> instead
> 	of checking tree code.
> testsuite/
> 	* c-c++-common/cilk-plus/CK/pr60197.c: New test.
> 
> diff --git gcc/c-family/array-notation-common.c gcc/c-family/array-notation-
> common.c
> index c010039..5d1e2c8 100644
> --- gcc/c-family/array-notation-common.c
> +++ gcc/c-family/array-notation-common.c
> @@ -657,3 +657,25 @@ fix_sec_implicit_args (location_t loc, vec <tree,
> va_gc> *list,
>        vec_safe_push (array_operand, (*list)[ii]);
>    return array_operand;
>  }
> +
> +/* Helper for contains_cilk_spawn_stmt, callback for walk_tree.  Return
> +   non-null tree if TP contains CILK_SPAWN_STMT.  */
> +
> +static tree
> +contains_cilk_spawn_stmt_walker (tree *tp, int *, void *) {
> +  if (TREE_CODE (*tp) == CILK_SPAWN_STMT)
> +    return *tp;
> +  else
> +    return NULL_TREE;
> +}
> +
> +/* Returns true if EXPR or any of its subtrees contain CILK_SPAWN_STMT
> +   node.  */
> +
> +bool
> +contains_cilk_spawn_stmt (tree expr)
> +{
> +  return walk_tree (&expr, contains_cilk_spawn_stmt_walker, NULL, NULL)
> +	 != NULL_TREE;
> +}

I would move these two functions to cilk.c file instead of array-notations-common.c since cilk.c contains all Cilk keyword handling routines.

> diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h index
> f074ab1..6f2c6b7 100644
> --- gcc/c-family/c-common.h
> +++ gcc/c-family/c-common.h
> @@ -1375,6 +1375,7 @@ extern void cilkplus_extract_an_triplets (vec<tree,
> va_gc> *, size_t, size_t,
>  					  vec<vec<an_parts> > *);
>  extern vec <tree, va_gc> *fix_sec_implicit_args
>    (location_t, vec <tree, va_gc> *, vec<an_loop_parts>, size_t, tree);
> +extern bool contains_cilk_spawn_stmt (tree);
> 

...and put the prototype under /* In Cilk.c.  */ part).

>  /* In cilk.c.  */
>  extern tree insert_cilk_frame (tree);
> diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c index da6a6fc..23cdb0e 100644
> --- gcc/c/c-typeck.c
> +++ gcc/c/c-typeck.c
> @@ -9134,7 +9134,7 @@ c_finish_return (location_t loc, tree retval, tree
> origtype)
>  	  return error_mark_node;
>  	}
>      }
> -  if (flag_cilkplus && retval && TREE_CODE (retval) == CILK_SPAWN_STMT)
> +  if (flag_cilkplus && retval && contains_cilk_spawn_stmt (retval))
>      {
>        error_at (loc, "use of %<_Cilk_spawn%> in a return statement is not "
>  		"allowed");
> diff --git gcc/cp/typeck.c gcc/cp/typeck.c index 5fc0e6b..566411f 100644
> --- gcc/cp/typeck.c
> +++ gcc/cp/typeck.c
> @@ -8328,7 +8328,7 @@ check_return_expr (tree retval, bool *no_warning)
> 
>    *no_warning = false;
> 
> -  if (flag_cilkplus && retval && TREE_CODE (retval) == CILK_SPAWN_STMT)
> +  if (flag_cilkplus && retval && contains_cilk_spawn_stmt (retval))
>      {
>        error_at (EXPR_LOCATION (retval), "use of %<_Cilk_spawn%> in a return
> "
>  		"statement is not allowed");
> diff --git gcc/testsuite/c-c++-common/cilk-plus/CK/pr60197.c gcc/testsuite/c-
> c++-common/cilk-plus/CK/pr60197.c
> index e69de29..2b47d1e 100644
> --- gcc/testsuite/c-c++-common/cilk-plus/CK/pr60197.c
> +++ gcc/testsuite/c-c++-common/cilk-plus/CK/pr60197.c
> @@ -0,0 +1,66 @@
> +/* PR c/60197 */
> +/* { dg-do compile } */
> +/* { dg-options "-fcilkplus" } */
> +
> +extern int foo (void);
> +extern int bar (int);
> +
> +int
> +fn1 (void)
> +{
> +  return (_Cilk_spawn foo ()) * 2; /* { dg-error "in a return statement
> +is not allowed" } */ }
> +
> +int
> +fn2 (void)
> +{
> +  return (_Cilk_spawn foo ()) > 2; /* { dg-error "in a return statement
> +is not allowed" } */ }
> +
> +int
> +fn3 (int i, int j, int k)
> +{
> +  return ((((((_Cilk_spawn foo () + i) - j) * k) / j) | i) ^ k) ; /* {
> +dg-error "in a return statement is not allowed" } */ }
> +
> +int
> +fn4 (int i, int j, int k)
> +{
> +  return (((((i - _Cilk_spawn foo ()) * k) / j) | i) ^ k); /* {
> +dg-error "in a return statement is not allowed" } */ }
> +
> +int
> +fn5 (void)
> +{
> +  return _Cilk_spawn foo (); /* { dg-error "in a return statement is
> +not allowed" } */ }
> +
> +int
> +fn6 (void)
> +{
> +  return _Cilk_spawn foo () + _Cilk_spawn foo (); /* { dg-error "in a
> +return statement is not allowed" } */ }
> +
> +int
> +fn7 (void)
> +{
> +  return 5 % _Cilk_spawn foo (); /* { dg-error "in a return statement
> +is not allowed" } */ }
> +
> +int
> +fn8 (void)
> +{
> +  return !_Cilk_spawn foo (); /* { dg-error "in a return statement is
> +not allowed" } */ }
> +
> +int
> +fn9 (void)
> +{
> +  return foo () && _Cilk_spawn foo (); /* { dg-error "in a return
> +statement is not allowed" } */ }
> +
> +int
> +fn10 (void)
> +{
> +  return bar (_Cilk_spawn foo ()); /* { dg-error "in a return statement
> +is not allowed" } */ }
> 

Other than the two above comments, the patch looks OK to me. But I don't have approval rights. I assume it doesn't break any existing regressions in the cilk-plus suite (doesn't look like it though)?

Thanks,

Balaji V. Iyer.

> 	Marek

On 02/17/14 10:51, Iyer, Balaji V wrote:
>> I don't know Cilk stuff at all, but it seems that _Cilk_spawn is forbidden in a
>> return statement.  But then only checking TREE_CODE (retval) ==
>> CILK_SPAWN_STMT isn't sufficient, because CILK_SPAWN_STMT can be
>> wrapped e.g. in MINUS_EXPR, C_MAYBE_CONST_EXPR, EQ_EXPR, and a
>> bunch of others.  I used walk_tree for checking whether a return statement
>> contains any CILK_SPAWN_STMT.
>>
>> Regtested/bootstrapped on x86_64-linux, ok for 5.0?
>>
>
> 5.0? you mean 4.9 right?... since this is a minor bug-fix.
Fixes go onto the trunk with selected fixes being backported to the 4.9 
branch at the discretion of the release managers.

jeff