diff mbox

[PR60189,Cilk+] Fix for ICE with incorrect Cilk_sync usage

Message ID 0EFAB2BDD0F67E4FB6CCC8B9F87D7569429DC138@IRSMSX103.ger.corp.intel.com
State New
Headers show

Commit Message

Zamyatin, Igor April 10, 2014, 2:23 p.m. UTC 
Resending with correct Changelog

Is it OK?

Thanks,
Igor

gcc/cp/ChangeLog:

2014-04-10  Igor Zamyatin  <igor.zamyatin@intel.com>

PR c++/60189
* parser.c (cp_parser_postfix_expression): Make sure only
semicolon can go after Cilk_sync.

gcc/testsuite/ChangeLog:

2014-04-10  Igor Zamyatin  <igor.zamyatin@intel.com>

PR c++/60189
* c-c++-common/cilk-plus/CK/invalid_sync.cс: New test.


> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> owner@gcc.gnu.org] On Behalf Of Zamyatin, Igor
> Sent: Thursday, April 10, 2014 5:56 PM
> To: GCC Patches (gcc-patches@gcc.gnu.org)
> Cc: Iyer, Balaji V; Jakub Jelinek (jakub@redhat.com)
> Subject: [PATCH, PR60189, Cilk+] Fix for ICE with incorrect Cilk_sync usage
> 
> Hi!
> 
> This fixes ICE on inappropriate usage of Cilk_sync keyword.
> 
> Bootstrapped/regtested on x86_64. Ok for trunk?
> 
> 
> Thanks,
> Igor
> 
>

Comments

Jakub Jelinek April 10, 2014, 2:27 p.m. UTC | #1 
On Thu, Apr 10, 2014 at 02:23:16PM +0000, Zamyatin, Igor wrote:
> 2014-04-10  Igor Zamyatin  <igor.zamyatin@intel.com>
> 
> PR c++/60189
> * parser.c (cp_parser_postfix_expression): Make sure only
> semicolon can go after Cilk_sync.
> 
> gcc/testsuite/ChangeLog:
> 
> 2014-04-10  Igor Zamyatin  <igor.zamyatin@intel.com>
> 
> PR c++/60189
> * c-c++-common/cilk-plus/CK/invalid_sync.cс: New test.

CCing Jason as this is a C++ FE change.

> --- a/gcc/cp/parser.c
> +++ b/gcc/cp/parser.c
> @@ -5835,20 +5835,33 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
>           }
>         break;
>        }
> -      
> +
>      case RID_CILK_SYNC:
> -      if (flag_cilkplus)
> -       { 
> -         tree sync_expr = build_cilk_sync ();
> -         SET_EXPR_LOCATION (sync_expr, 
> -                            cp_lexer_peek_token (parser->lexer)->location);
> -         finish_expr_stmt (sync_expr);
> -       }
> -      else
> -       error_at (token->location, "-fcilkplus must be enabled to use" 
> -                 " %<_Cilk_sync%>");
> -      cp_lexer_consume_token (parser->lexer);
> -      break;
> +      {

I don't see the point of adding the extra {} around the whole case,
there is no varaible declared at that point.

Other than that it looks good to me, but I'll defer the review to Jason.

> +       cp_lexer_consume_token (parser->lexer);
> +       if (flag_cilkplus)
> +         {
> +           token = cp_lexer_peek_token (parser->lexer);
> +           if (token->type != CPP_SEMICOLON)
> +             {
> +               error_at (token->location, "%<_Cilk_sync%> must be followed"
> +                         " by semicolon");
> +               postfix_expression = error_mark_node;
> +               break;
> +             }
> +           tree sync_expr = build_cilk_sync ();
> +           SET_EXPR_LOCATION (sync_expr,
> +                              cp_lexer_peek_token (parser->lexer)->location);
> +           finish_expr_stmt (sync_expr);
> +         }
> +       else
> +         {
> +           error_at (token->location, "-fcilkplus must be enabled to use"
> +                     " %<_Cilk_sync%>");
> +           postfix_expression = error_mark_node;
> +         }
> +       break;
> +      }

	Jakub
Jason Merrill April 11, 2014, 2:55 p.m. UTC | #2 
On 04/10/2014 10:27 AM, Jakub Jelinek wrote:
> I don't see the point of adding the extra {} around the whole case,
> there is no variable declared at that point.

Agreed.

>> +           token = cp_lexer_peek_token (parser->lexer);
>> +           if (token->type != CPP_SEMICOLON)
>> +             {
>> +               error_at (token->location, "%<_Cilk_sync%> must be followed"
>> +                         " by semicolon");
>> +               postfix_expression = error_mark_node;
>> +               break;
>> +             }

Any reason not to use cp_parser_require here?

Jason
Zamyatin, Igor April 11, 2014, 5:38 p.m. UTC | #3 
> 

> >> +           token = cp_lexer_peek_token (parser->lexer);

> >> +           if (token->type != CPP_SEMICOLON)

> >> +             {

> >> +               error_at (token->location, "%<_Cilk_sync%> must be followed"

> >> +                         " by semicolon");

> >> +               postfix_expression = error_mark_node;

> >> +               break;

> >> +             }

> 

> Any reason not to use cp_parser_require here?


Right! Will try it and repost the patch. Thanks!

Igor

> 

> Jason
Zamyatin, Igor April 11, 2014, 7:08 p.m. UTC | #4 
> 

> >> +           token = cp_lexer_peek_token (parser->lexer);

> >> +           if (token->type != CPP_SEMICOLON)

> >> +             {

> >> +               error_at (token->location, "%<_Cilk_sync%> must be followed"

> >> +                         " by semicolon");

> >> +               postfix_expression = error_mark_node;

> >> +               break;

> >> +             }

> 

> Any reason not to use cp_parser_require here?


I remembered - I haven't used cp_parser_require since it calls cp_lexer_consume_token which is not needed at this point. It is already called a bit earlier.

Igor

> 

> Jason
Jason Merrill April 14, 2014, 4:13 a.m. UTC | #5 
On 04/11/2014 03:08 PM, Zamyatin, Igor wrote:
> I remembered - I haven't used cp_parser_require since it calls cp_lexer_consume_token which is not needed at this point. It is already called a bit earlier.

So the call to cp_parser_require can replace that call as well.

Jason
diff mbox

Patch

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index 7bea3d2..95f9c93 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -5835,20 +5835,33 @@  cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
          }
        break;
       }
-      
+
     case RID_CILK_SYNC:
-      if (flag_cilkplus)
-       { 
-         tree sync_expr = build_cilk_sync ();
-         SET_EXPR_LOCATION (sync_expr, 
-                            cp_lexer_peek_token (parser->lexer)->location);
-         finish_expr_stmt (sync_expr);
-       }
-      else
-       error_at (token->location, "-fcilkplus must be enabled to use" 
-                 " %<_Cilk_sync%>");
-      cp_lexer_consume_token (parser->lexer);
-      break;
+      {
+       cp_lexer_consume_token (parser->lexer);
+       if (flag_cilkplus)
+         {
+           token = cp_lexer_peek_token (parser->lexer);
+           if (token->type != CPP_SEMICOLON)
+             {
+               error_at (token->location, "%<_Cilk_sync%> must be followed"
+                         " by semicolon");
+               postfix_expression = error_mark_node;
+               break;
+             }
+           tree sync_expr = build_cilk_sync ();
+           SET_EXPR_LOCATION (sync_expr,
+                              cp_lexer_peek_token (parser->lexer)->location);
+           finish_expr_stmt (sync_expr);
+         }
+       else
+         {
+           error_at (token->location, "-fcilkplus must be enabled to use"
+                     " %<_Cilk_sync%>");
+           postfix_expression = error_mark_node;
+         }
+       break;
+      }

     case RID_BUILTIN_SHUFFLE:
       {
diff --git a/gcc/testsuite/c-c++-common/cilk-plus/CK/invalid_sync.сc b/gcc/testsuite/c-c++-common/cilk-plus/CK/invalid_sync.cс
new file mode 100644
index 0000000..e7bec68
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/cilk-plus/CK/invalid_sync.cс
@@ -0,0 +1,9 @@ 
+/* PR c/60189 */
+/* { dg-do compile } */
+/* { dg-options "-fcilkplus" } */
+
+int main (void)
+{
+    _Cilk_sync return; /* { dg-error " '_Cilk_sync' must be followed by semicolon" } */
+    return 0;
+}