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
