RE: [PATCH, PR60189, Cilk+] Fix for ICE with incorrect Cilk_sync usage
Resending with correct Changelog Is it OK? Thanks, Igor gcc/cp/ChangeLog: 2014-04-10 Igor Zamyatin 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 PR c++/60189 * c-c++-common/cilk-plus/CK/invalid_sync.cс: New test. 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 000..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; +} > -Original Message- > From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches- > ow...@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 (ja...@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 > >
Re: [PATCH, PR60189, Cilk+] Fix for ICE with incorrect Cilk_sync usage
On Thu, Apr 10, 2014 at 02:23:16PM +, Zamyatin, Igor wrote: > 2014-04-10 Igor Zamyatin > > 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 > > 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
Re: [PATCH, PR60189, Cilk+] Fix for ICE with incorrect Cilk_sync usage
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
RE: [PATCH, PR60189, Cilk+] Fix for ICE with incorrect Cilk_sync usage
> > >> + 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
RE: [PATCH, PR60189, Cilk+] Fix for ICE with incorrect Cilk_sync usage
> > >> + 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
Re: [PATCH, PR60189, Cilk+] Fix for ICE with incorrect Cilk_sync usage
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
RE: [PATCH, PR60189, Cilk+] Fix for ICE with incorrect Cilk_sync usage
> -Original Message- > From: Jason Merrill [mailto:ja...@redhat.com] > Sent: Monday, April 14, 2014 8:13 AM > To: Zamyatin, Igor; Jakub Jelinek > Cc: GCC Patches (gcc-patches@gcc.gnu.org); Iyer, Balaji V > Subject: Re: [PATCH, PR60189, Cilk+] Fix for ICE with incorrect Cilk_sync > usage > > 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. As far as I understand, it can't replace it in this case since cp_parser_require contains call to cp_lexer_peek_token and, without consuming, it will still return pointer to CILK_SYNC not on the token after it. (Actually, this is somewhat similar to the check in RID_CILK_SPAWN case) So I suggest the following gcc/ChangeLog: 2014-04-14 Igor Zamyatin PR c++/60189 * cp/parser.c (cp_parser_postfix_expression): Make sure only semicolon can go after Cilk_sync. gcc/testsuite/ChangeLog: 2014-04-14 Igor Zamyatin PR c++/60189 * c-c++-common/cilk-plus/CK/invalid_sync.cс: New test. diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index bb59e3b..0b3cb5a 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -5837,17 +5837,27 @@ cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p, } case RID_CILK_SYNC: + cp_lexer_consume_token (parser->lexer); if (flag_cilkplus) - { + { + if (cp_lexer_next_token_is_not (parser->lexer, 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); + SET_EXPR_LOCATION (sync_expr, +token->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); + { + 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.cc b/gcc/testsuite/c-c++-common/cilk-plus/CK/invalid_sync.cc new file mode 100644 index 000..e7bec68 --- /dev/null +++ b/gcc/testsuite/c-c++-common/cilk-plus/CK/invalid_sync.cc @@ -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; +} Igor > > Jason >
Re: [PATCH, PR60189, Cilk+] Fix for ICE with incorrect Cilk_sync usage
Oh, I see where the problem is coming from. Cilk_sync is a statement, but it's being parsed as an expression. Let's move it to cp_parser_statement. Jason
RE: [PATCH, PR60189, Cilk+] Fix for ICE with incorrect Cilk_sync usage
> -Original Message- > From: Jason Merrill [mailto:ja...@redhat.com] > Sent: Monday, April 14, 2014 9:49 PM > To: Zamyatin, Igor; Jakub Jelinek > Cc: GCC Patches (gcc-patches@gcc.gnu.org); Iyer, Balaji V > Subject: Re: [PATCH, PR60189, Cilk+] Fix for ICE with incorrect Cilk_sync > usage > > Oh, I see where the problem is coming from. Cilk_sync is a statement, but > it's being parsed as an expression. Let's move it to cp_parser_statement. Something like this (better to put new code in separate routine?)? diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index bb59e3b..3105d6c 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -5835,20 +5835,6 @@ 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; case RID_BUILTIN_SHUFFLE: { @@ -9404,6 +9390,24 @@ cp_parser_statement (cp_parser* parser, tree in_statement_expr, statement = cp_parser_jump_statement (parser); break; + case RID_CILK_SYNC: + cp_lexer_consume_token (parser->lexer); + if (flag_cilkplus) + { + tree sync_expr = build_cilk_sync (); + SET_EXPR_LOCATION (sync_expr, +token->location); + statement = finish_expr_stmt (sync_expr); + } + else + { + error_at (token->location, "-fcilkplus must be enabled to use" + " %<_Cilk_sync%>"); + statement = error_mark_node; + } + cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON); + break; + /* Objective-C++ exception-handling constructs. */ case RID_AT_TRY: case RID_AT_CATCH: Thanks, Igor > > Jason
RE: [PATCH, PR60189, Cilk+] Fix for ICE with incorrect Cilk_sync usage
> > From: Jason Merrill [mailto:ja...@redhat.com] > > Sent: Monday, April 14, 2014 9:49 PM > > To: Zamyatin, Igor; Jakub Jelinek > > Cc: GCC Patches (gcc-patches@gcc.gnu.org); Iyer, Balaji V > > Subject: Re: [PATCH, PR60189, Cilk+] Fix for ICE with incorrect > > Cilk_sync usage > > > > Oh, I see where the problem is coming from. Cilk_sync is a statement, > > but it's being parsed as an expression. Let's move it to > cp_parser_statement. > > Something like this (better to put new code in separate routine?)? > > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index bb59e3b..3105d6c 100644 > --- a/gcc/cp/parser.c > +++ b/gcc/cp/parser.c > @@ -5835,20 +5835,6 @@ 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; > > case RID_BUILTIN_SHUFFLE: >{ > @@ -9404,6 +9390,24 @@ cp_parser_statement (cp_parser* parser, tree > in_statement_expr, > statement = cp_parser_jump_statement (parser); > break; > > + case RID_CILK_SYNC: > + cp_lexer_consume_token (parser->lexer); > + if (flag_cilkplus) > + { > + tree sync_expr = build_cilk_sync (); > + SET_EXPR_LOCATION (sync_expr, > +token->location); > + statement = finish_expr_stmt (sync_expr); > + } > + else > + { > + error_at (token->location, "-fcilkplus must be enabled to use" > + " %<_Cilk_sync%>"); > + statement = error_mark_node; > + } > + cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON); > + break; > + > /* Objective-C++ exception-handling constructs. */ > case RID_AT_TRY: > case RID_AT_CATCH: > > Thanks, > Igor > Jason, Does the above change make sense? Thanks in advance, Igor > > > > Jason
RE: [PATCH, PR60189, Cilk+] Fix for ICE with incorrect Cilk_sync usage
Ping. Should I prepare the patch? Thanks, Igor > > -Original Message > > From: Jason Merrill [mailto:ja...@redhat.com] > > Sent: Monday, April 14, 2014 9:49 PM > > To: Zamyatin, Igor; Jakub Jelinek > > Cc: GCC Patches (gcc-patches@gcc.gnu.org); Iyer, Balaji V > > Subject: Re: [PATCH, PR60189, Cilk+] Fix for ICE with incorrect > > Cilk_sync usage > > > > Oh, I see where the problem is coming from. Cilk_sync is a statement, > > but it's being parsed as an expression. Let's move it to > cp_parser_statement. > > Something like this (better to put new code in separate routine?)? > > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index bb59e3b..3105d6c 100644 > --- a/gcc/cp/parser.c > +++ b/gcc/cp/parser.c > @@ -5835,20 +5835,6 @@ 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; > > case RID_BUILTIN_SHUFFLE: >{ > @@ -9404,6 +9390,24 @@ cp_parser_statement (cp_parser* parser, tree > in_statement_expr, > statement = cp_parser_jump_statement (parser); > break; > > + case RID_CILK_SYNC: > + cp_lexer_consume_token (parser->lexer); > + if (flag_cilkplus) > + { > + tree sync_expr = build_cilk_sync (); > + SET_EXPR_LOCATION (sync_expr, > +token->location); > + statement = finish_expr_stmt (sync_expr); > + } > + else > + { > + error_at (token->location, "-fcilkplus must be enabled to use" > + " %<_Cilk_sync%>"); > + statement = error_mark_node; > + } > + cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON); > + break; > + > /* Objective-C++ exception-handling constructs. */ > case RID_AT_TRY: > case RID_AT_CATCH: > > Thanks, > Igor > > > > > Jason
RE: [PATCH, PR60189, Cilk+] Fix for ICE with incorrect Cilk_sync usage
Please look then on the following patch. Regtested successfully on x86_64. Is it ok for trunk and 4.9? gcc/cp/ChangeLog: 2014-05-20 Igor Zamyatin PR c/60189 * parser.c (cp_parser_postfix_expression): Move handling of cilk_sync from here to... (cp_parser_statement): ...here. Make sure only semicolon can go after Cilk_sync. gcc/testsuite/ChangeLog: 2014-05-20 Igor Zamyatin PR c++/60189 * c-c++-common/cilk-plus/CK/invalid_sync.cс: New test. diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index 0c9e113..814f323 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -5845,20 +5845,6 @@ 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; case RID_BUILTIN_SHUFFLE: { @@ -9414,6 +9400,24 @@ cp_parser_statement (cp_parser* parser, tree in_statement_expr, statement = cp_parser_jump_statement (parser); break; + case RID_CILK_SYNC: + cp_lexer_consume_token (parser->lexer); + if (flag_cilkplus) + { + tree sync_expr = build_cilk_sync (); + SET_EXPR_LOCATION (sync_expr, +token->location); + statement = finish_expr_stmt (sync_expr); + } + else + { + error_at (token->location, "-fcilkplus must be enabled to use" + " %<_Cilk_sync%>"); + statement = error_mark_node; + } + cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON); + break; + /* Objective-C++ exception-handling constructs. */ case RID_AT_TRY: case RID_AT_CATCH: diff --git a/gcc/testsuite/c-c++-common/cilk-plus/CK/invalid_sync.cc b/gcc/testsuite/c-c++-common/cilk-plus/CK/invalid_sync.cc new file mode 100644 index 000..cf1caf1 --- /dev/null +++ b/gcc/testsuite/c-c++-common/cilk-plus/CK/invalid_sync.cc @@ -0,0 +1,9 @@ +/* PR c/60189 */ +/* { dg-do compile } */ +/* { dg-options "-fcilkplus" } */ + +int main (void) +{ +_Cilk_sync return; /* { dg-error " expected ';' before 'return'" } */ +return 0; +} Thanks, Igor > -Original Message- > From: Zamyatin, Igor > Sent: Tuesday, May 13, 2014 12:28 AM > To: 'Jason Merrill'; 'Jakub Jelinek' > Cc: 'GCC Patches (gcc-patches@gcc.gnu.org)'; Iyer, Balaji V > Subject: RE: [PATCH, PR60189, Cilk+] Fix for ICE with incorrect Cilk_sync > usage > > Ping. Should I prepare the patch? > > Thanks, > Igor > > > > -Original Message > > > From: Jason Merrill [mailto:ja...@redhat.com] > > > Sent: Monday, April 14, 2014 9:49 PM > > > To: Zamyatin, Igor; Jakub Jelinek > > > Cc: GCC Patches (gcc-patches@gcc.gnu.org); Iyer, Balaji V > > > Subject: Re: [PATCH, PR60189, Cilk+] Fix for ICE with incorrect > > > Cilk_sync usage > > > > > > Oh, I see where the problem is coming from. Cilk_sync is a > > > statement, but it's being parsed as an expression. Let's move it to > > cp_parser_statement. > > > > Something like this (better to put new code in separate routine?)? > > > > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index bb59e3b..3105d6c > > 100644 > > --- a/gcc/cp/parser.c > > +++ b/gcc/cp/parser.c > > @@ -5835,20 +5835,6 @@ 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; > > > > case RID_BUILTIN_SHUFFLE: > >{ > > @@ -9404,6 +9390,24 @@ cp_parser_statement (cp_parser* parser, tree > > in_statement_expr, > >
Re: [PATCH, PR60189, Cilk+] Fix for ICE with incorrect Cilk_sync usage
On 05/20/14 08:10, Zamyatin, Igor wrote: Please look then on the following patch. Regtested successfully on x86_64. Is it ok for trunk and 4.9? gcc/cp/ChangeLog: 2014-05-20 Igor Zamyatin PR c/60189 * parser.c (cp_parser_postfix_expression): Move handling of cilk_sync from here to... (cp_parser_statement): ...here. Make sure only semicolon can go after Cilk_sync. gcc/testsuite/ChangeLog: 2014-05-20 Igor Zamyatin PR c++/60189 * c-c++-common/cilk-plus/CK/invalid_sync.cс: New test. This is fine for both the trunk and the 4.9 branch. Thanks, Jeff