| Message ID | 20160303161535.GE10006@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 03/03/2016 09:15 AM, Marek Polacek wrote: > On Thu, Mar 03, 2016 at 03:28:01PM +0100, Jakub Jelinek wrote: >> On Thu, Mar 03, 2016 at 03:15:41PM +0100, Marek Polacek wrote: >>> This is ICE on invalid Cilk+ code. cilk_spawn expects a function call, so e.g. >>> _Cilk_spawn (void) is invalid. The function call after the cilk_spawn keyword >>> is parsed using recursive call in c_parser_postfix_expression (case >>> RID_CILK_SPAWN). Now, c_parser_postfix_expression sees '(' followed by a >>> typename, so it thinks we're inside a compound literal, which means it expects >>> '{', but that isn't there, so we crash on the assert in c_parser_braced_init: >>> gcc_assert (c_parser_next_token_is (parser, CPP_OPEN_BRACE)); >>> But as the comment in c_parser_postfix_expression says, the code for parsing >>> a compound literal here is likely dead. I made an experiment and added >>> gcc_unreachable () in that block, ran regtest, and there were no failures. >>> Thus it should be safe to just remove the code, which also fixes this ICE; with >>> the patch we just give a proper error and don't crash anymore. >>> >>> Bootstrapped/regtested on x86_64-linux, ok for trunk? I'm actually slightly >>> nervous about the change, so maybe better table until gcc7? >> >> This reminds me of PR67495. Perhaps the answer here is also during the >> _Cilk* stuff parsing don't call c_parser_postfix_expression, but call >> c_parser_cast_expression instead and then verify what it got? > > Alternatively this one works as well. I don't know if any verification of the > result should be done (in the second call, the first one is invalid anyway). > > 2016-03-03 Marek Polacek <polacek@redhat.com> > > PR c/69798 > * c-parser.c (c_parser_postfix_expression): Call > c_parser_cast_expression instead of c_parser_postfix_expression. > > * gcc.dg/cilk-plus/pr69798-1.c: New test. > * gcc.dg/cilk-plus/pr69798-2.c: New test. I'd wait for gcc-7. There's actually further Cilk+ fixes queued up from Ryan. I wanted to get those into gcc-6, but just flat ran out of time. Perhaps ask for an exception to address the queued up Cilk+ stuff in a minor release? jeff
On Thu, Mar 03, 2016 at 09:24:36AM -0700, Jeff Law wrote: > I'd wait for gcc-7. There's actually further Cilk+ fixes queued up from > Ryan. I wanted to get those into gcc-6, but just flat ran out of time. > Perhaps ask for an exception to address the queued up Cilk+ stuff in a minor > release? Works for me. Marek
On Thu, Mar 03, 2016 at 09:24:36AM -0700, Jeff Law wrote: > >2016-03-03 Marek Polacek <polacek@redhat.com> > > > > PR c/69798 > > * c-parser.c (c_parser_postfix_expression): Call > > c_parser_cast_expression instead of c_parser_postfix_expression. > > > > * gcc.dg/cilk-plus/pr69798-1.c: New test. > > * gcc.dg/cilk-plus/pr69798-2.c: New test. > I'd wait for gcc-7. There's actually further Cilk+ fixes queued up from > Ryan. I wanted to get those into gcc-6, but just flat ran out of time. > Perhaps ask for an exception to address the queued up Cilk+ stuff in a minor > release? Well, this one looks fairly safe to me even for gcc-6, and has the additional benefit that it affects solely Cilk+ and nothing else. Furthermore, I believe the difference between cast and postfix expression is just in what is invalid for Cilk+, so it shouldn't affect valid Cilk+ code, only invalid. Jakub
On 03/03/2016 09:15 AM, Marek Polacek wrote: > On Thu, Mar 03, 2016 at 03:28:01PM +0100, Jakub Jelinek wrote: >> On Thu, Mar 03, 2016 at 03:15:41PM +0100, Marek Polacek wrote: >>> This is ICE on invalid Cilk+ code. cilk_spawn expects a function call, so e.g. >>> _Cilk_spawn (void) is invalid. The function call after the cilk_spawn keyword >>> is parsed using recursive call in c_parser_postfix_expression (case >>> RID_CILK_SPAWN). Now, c_parser_postfix_expression sees '(' followed by a >>> typename, so it thinks we're inside a compound literal, which means it expects >>> '{', but that isn't there, so we crash on the assert in c_parser_braced_init: >>> gcc_assert (c_parser_next_token_is (parser, CPP_OPEN_BRACE)); >>> But as the comment in c_parser_postfix_expression says, the code for parsing >>> a compound literal here is likely dead. I made an experiment and added >>> gcc_unreachable () in that block, ran regtest, and there were no failures. >>> Thus it should be safe to just remove the code, which also fixes this ICE; with >>> the patch we just give a proper error and don't crash anymore. >>> >>> Bootstrapped/regtested on x86_64-linux, ok for trunk? I'm actually slightly >>> nervous about the change, so maybe better table until gcc7? >> >> This reminds me of PR67495. Perhaps the answer here is also during the >> _Cilk* stuff parsing don't call c_parser_postfix_expression, but call >> c_parser_cast_expression instead and then verify what it got? > > Alternatively this one works as well. I don't know if any verification of the > result should be done (in the second call, the first one is invalid anyway). > > 2016-03-03 Marek Polacek <polacek@redhat.com> > > PR c/69798 > * c-parser.c (c_parser_postfix_expression): Call > c_parser_cast_expression instead of c_parser_postfix_expression. > > * gcc.dg/cilk-plus/pr69798-1.c: New test. > * gcc.dg/cilk-plus/pr69798-2.c: New test. Do we need to do anything for the call into c_parser_postfix_expression that occurs between the two you changed in this patch. I think you can get into that code with something like _Cilk_spawn _Cilk_spawn (<whatever>)); For the non-error case (the second one you changed), I think we do want to verify that the expression we got back was a function call and issue an appropriate error otherwise. You could make an argument that we should issue a diagnostic for the other cases as well, but it's less obviously correct. jeff
On Thu, Mar 03, 2016 at 11:31:08PM -0700, Jeff Law wrote: > >2016-03-03 Marek Polacek <polacek@redhat.com> > > > > PR c/69798 > > * c-parser.c (c_parser_postfix_expression): Call > > c_parser_cast_expression instead of c_parser_postfix_expression. > > > > * gcc.dg/cilk-plus/pr69798-1.c: New test. > > * gcc.dg/cilk-plus/pr69798-2.c: New test. > Do we need to do anything for the call into c_parser_postfix_expression that > occurs between the two you changed in this patch. > > I think you can get into that code with something like > > _Cilk_spawn _Cilk_spawn (<whatever>)); The _Cilk_spawn _Cilk_spawn (struct S); case needs the same treatment as the other 2 spots, sure, and should be also covered by a testcase. Perhaps another testcase should test the various other cases of the postfix vs. cast expression I've mentioned in the other mail, just make sure the diagnostics is reasonable and we don't ICE. Jakub
On Thu, Mar 03, 2016 at 11:31:08PM -0700, Jeff Law wrote: > Do we need to do anything for the call into c_parser_postfix_expression that > occurs between the two you changed in this patch. > > I think you can get into that code with something like > > _Cilk_spawn _Cilk_spawn (<whatever>)); Absolutely. Dunno how I missed that. Will fix in a new version of the patch... > For the non-error case (the second one you changed), I think we do want to > verify that the expression we got back was a function call and issue an > appropriate error otherwise. You could make an argument that we should > issue a diagnostic for the other cases as well, but it's less obviously > correct. As noted elsewhere, cilk_set_spawn_marker rejects everything except CALL_EXPR. Marek
diff --git gcc/c/c-parser.c gcc/c/c-parser.c index bb508b7..ce00457 100644 --- gcc/c/c-parser.c +++ gcc/c/c-parser.c @@ -8024,8 +8024,8 @@ c_parser_postfix_expression (c_parser *parser) { error_at (loc, "-fcilkplus must be enabled to use " "%<_Cilk_spawn%>"); - expr = c_parser_postfix_expression (parser); - expr.value = error_mark_node; + expr = c_parser_cast_expression (parser, NULL); + expr.value = error_mark_node; } else if (c_parser_peek_token (parser)->keyword == RID_CILK_SPAWN) { @@ -8038,7 +8038,7 @@ c_parser_postfix_expression (c_parser *parser) } else { - expr = c_parser_postfix_expression (parser); + expr = c_parser_cast_expression (parser, NULL); expr.value = build_cilk_spawn (loc, expr.value); } break; diff --git gcc/testsuite/gcc.dg/cilk-plus/pr69798-1.c gcc/testsuite/gcc.dg/cilk-plus/pr69798-1.c index e69de29..1120193 100644 --- gcc/testsuite/gcc.dg/cilk-plus/pr69798-1.c +++ gcc/testsuite/gcc.dg/cilk-plus/pr69798-1.c @@ -0,0 +1,12 @@ +/* PR c/69798 */ +/* { dg-do compile } */ +/* { dg-options "-fcilkplus" } */ + +int +main () +{ + _Cilk_spawn (void); /* { dg-error "expected expression" } */ + _Cilk_spawn (char []); /* { dg-error "expected expression" } */ + _Cilk_spawn (int *); /* { dg-error "expected expression" } */ + _Cilk_spawn ({}); /* { dg-error "only function calls can be spawned" } */ +} diff --git gcc/testsuite/gcc.dg/cilk-plus/pr69798-2.c gcc/testsuite/gcc.dg/cilk-plus/pr69798-2.c index e69de29..66bcdc8 100644 --- gcc/testsuite/gcc.dg/cilk-plus/pr69798-2.c +++ gcc/testsuite/gcc.dg/cilk-plus/pr69798-2.c @@ -0,0 +1,11 @@ +/* PR c/69798 */ +/* { dg-do compile } */ + +int +main () +{ + _Cilk_spawn (void); /* { dg-error "expected expression" } */ + _Cilk_spawn (char []); /* { dg-error "expected expression" } */ + _Cilk_spawn (int *); /* { dg-error "expected expression" } */ + _Cilk_spawn ({}); /* { dg-error "only function calls can be spawned" } */ +}