| Message ID | 20160303141541.GD10006@redhat.com |
|---|---|
| State | New |
| Headers | show |
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? > 2016-03-03 Marek Polacek <polacek@redhat.com> > > PR c/69798 > * c-parser.c (c_parser_postfix_expression): Remove code dealing with > compound literals. > > * gcc.dg/cilk-plus/pr69798-1.c: New test. > * gcc.dg/cilk-plus/pr69798-2.c: New test. Jakub
On 03/03/2016 07:15 AM, 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? > > 2016-03-03 Marek Polacek <polacek@redhat.com> > > PR c/69798 > * c-parser.c (c_parser_postfix_expression): Remove code dealing with > compound literals. > > * gcc.dg/cilk-plus/pr69798-1.c: New test. > * gcc.dg/cilk-plus/pr69798-2.c: New test. FWIW, I don't particularly like this version. My worry about just removing this code is that I expect our testsuite doesn't cover parsing issues all that well. I'd feel better if it'd been run through one of the commercial suites. Even then those suites don't cover the GNU extensions, but at least the scope of things that might go wrong is significantly diminished. Jeff
On 03/03/2016 07:28 AM, 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? That sounds more sensible to me -- as long as there aren't unintended side effects of calling c_parser_cast_expression. Jeff
On Thu, Mar 03, 2016 at 10:52:20PM -0700, Jeff Law wrote: > On 03/03/2016 07:28 AM, 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? > That sounds more sensible to me -- as long as there aren't unintended side > effects of calling c_parser_cast_expression. The difference is that c_parser_cast_expression can parse (type)..., ++..., --..., &..., +..., -..., *..., ~..., !..., &&..., sizeof/alignof/__extension__/__real/__imag/__transaction_{atomic,relaxed}... Perhaps even safer version would be: if (c_parser_next_token_is (parser, CPP_OPEN_PAREN) && c_token_starts_typename (c_parser_peek_2nd_token (parser))) { ... error handling ... } else ... c_parser_postfix_expression (parser); But, even c_parser_postfix_expression parses tons of expressions that aren't allowed there, so I think it is overkill. It needs to check afterwards, and from what I can see that happens in cilk_set_spawn_marker. Thus I think c_parser_cast_expression is safe, it parses perhaps more unexpected expressions, but will reject them all anyway, because they aren't CALL_EXPR after parsing. Jakub
On Fri, Mar 04, 2016 at 07:27:51AM +0100, Jakub Jelinek wrote: > The difference is that c_parser_cast_expression can parse (type)..., > ++..., --..., &..., +..., -..., *..., ~..., !..., &&..., > sizeof/alignof/__extension__/__real/__imag/__transaction_{atomic,relaxed}... > Perhaps even safer version would be: > if (c_parser_next_token_is (parser, CPP_OPEN_PAREN) > && c_token_starts_typename (c_parser_peek_2nd_token (parser))) > { > ... error handling ... > } > else > ... c_parser_postfix_expression (parser); This occurred to me too, but I found that too ugly, and I'm not even sure it prevents other possible ICEs. Plus we'd need to put this new check into three places, so I dropped this idea. > But, even c_parser_postfix_expression parses tons of expressions that > aren't allowed there, so I think it is overkill. It needs to check > afterwards, and from what I can see that happens in cilk_set_spawn_marker. > Thus I think c_parser_cast_expression is safe, it parses perhaps more > unexpected expressions, but will reject them all anyway, because they aren't > CALL_EXPR after parsing. Yeah, that's right. Marek
On Thu, Mar 03, 2016 at 10:51:14PM -0700, Jeff Law wrote: > On 03/03/2016 07:15 AM, 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? > > > >2016-03-03 Marek Polacek <polacek@redhat.com> > > > > PR c/69798 > > * c-parser.c (c_parser_postfix_expression): Remove code dealing with > > compound literals. > > > > * gcc.dg/cilk-plus/pr69798-1.c: New test. > > * gcc.dg/cilk-plus/pr69798-2.c: New test. > FWIW, I don't particularly like this version. My worry about just removing > this code is that I expect our testsuite doesn't cover parsing issues all > that well. > > I'd feel better if it'd been run through one of the commercial suites. Even > then those suites don't cover the GNU extensions, but at least the scope of > things that might go wrong is significantly diminished. I agree but unfortunately I don't have the means to run such a suite. Compound literals are ISO C99 thing, so hopefully they are well-covered in those test suites. Ultimately I want to either do away with the code, or add tests that exercise the codepath. Marek
diff --git gcc/c/c-parser.c gcc/c/c-parser.c index bb508b7..9e8ac1b 100644 --- gcc/c/c-parser.c +++ gcc/c/c-parser.c @@ -7512,28 +7512,6 @@ c_parser_postfix_expression (c_parser *parser) set_c_expr_source_range (&expr, loc, close_loc); mark_exp_read (expr.value); } - else if (c_token_starts_typename (c_parser_peek_2nd_token (parser))) - { - /* A compound literal. ??? Can we actually get here rather - than going directly to - c_parser_postfix_expression_after_paren_type from - elsewhere? */ - location_t loc; - struct c_type_name *type_name; - c_parser_consume_token (parser); - loc = c_parser_peek_token (parser)->location; - type_name = c_parser_type_name (parser); - c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, - "expected %<)%>"); - if (type_name == NULL) - { - expr.value = error_mark_node; - } - else - expr = c_parser_postfix_expression_after_paren_type (parser, - type_name, - loc); - } else { /* A parenthesized expression. */ 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" } */ +}
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? 2016-03-03 Marek Polacek <polacek@redhat.com> PR c/69798 * c-parser.c (c_parser_postfix_expression): Remove code dealing with compound literals. * gcc.dg/cilk-plus/pr69798-1.c: New test. * gcc.dg/cilk-plus/pr69798-2.c: New test. Marek