On Mon, May 29, 2017 at 6:38 AM, Mikhail Maltsev <malts...@gmail.com> wrote:
> Hi. Sorry for a long delay.
>
> On 02.05.2017 17:16, Richard Biener wrote:
>> Certainly an improvement. I suppose we can do better error recovery
>> for cases like
>>
>> if (1)
>> goto
>> else
>> goto bar;
>>
>> but I guess this is better than nothing.
> I think it's worth spending a bit more time to get this right.
>
>>
>> And yes, we use c_parser_error -- input_location should be ok but here
>> we just peek which may upset the parser. Maybe it works better
>> when consuming the token before issueing the error? Thus
>>
>> Index: gcc/c/gimple-parser.c
>> ===================================================================
>> --- gcc/c/gimple-parser.c (revision 247498)
>> +++ gcc/c/gimple-parser.c (working copy)
>> @@ -1315,8 +1315,8 @@ c_parser_gimple_if_stmt (c_parser *parse
>> loc = c_parser_peek_token (parser)->location;
>> c_parser_consume_token (parser);
>> label = c_parser_peek_token (parser)->value;
>> - t_label = lookup_label_for_goto (loc, label);
>> c_parser_consume_token (parser);
>> + t_label = lookup_label_for_goto (loc, label);
>> if (! c_parser_require (parser, CPP_SEMICOLON, "expected %<;%>"))
>> return;
>> }
>>
> I was more focused on cases with missing labels (i.e. 'label' is NULL), rather
> than cases with syntactically correct if-statements referencing undefined
> labels. And, frankly speaking, I'm not sure that swapping
> 'c_parser_consume_token' with 'lookup_label_for_goto' will help, because
> 'lookup_label_for_goto' already gets a 'loc' parameter.
Ah, indeed.
> BTW, unfortunately GIMPLE FE does not handle undefined labels properly. I.e.,
> this test case
>
> __GIMPLE() void foo()
> {
> bb_0:
> if (0)
> goto bb_0;
> else
> goto bb_1;
> }
>
> causes an ICE somewhere in build_gimple_cfg/cleanup_dead_labels. But this is a
> separate issue, of course.
Yes. I think ICEing for invalid GIMPLE (as opposed for syntactic
errors) is OK for now.
> I attached a slightly modified patch, which incorporates your changes and
> also uses
>
> if (! c_parser_next_token_is (parser, CPP_NAME))
> ...
>
> instead of
>
> label = c_parser_peek_token (parser)->value;
> ...
> if (!label)
> ...
>
> for better readability. This version correctly handles missing labels and
> semicolons in both branches of the 'if' statement.
>
> The only major problem, which I want to fix is error recovery in the following
> example:
>
> __GIMPLE() void foo()
> {
> if (1)
> goto lbl;
> else
> goto ; /* { dg-error "expected label" } */
> }
>
> __GIMPLE() void bar()
> {
> if (1)
> goto lbl;
> else
> goto
> } /* { dg-error "expected label" } */
>
> In this case GCC correctly diagnoses both errors. But if I swap these two
> functions so that 'bar' comes before 'foo', the error in 'foo' is not
> diagnosed.
> I did not dive into details, but my speculation is that the parser enters
> some
> strange state and skips 'foo' entirely (-fdump-tree-gimple contains only
> 'bar').
> If I add another function after 'foo', it is handled correctly.
> Any ideas, why this could happen and how to improve error recovery in this
> case?
Huh. I suppose this is due to us testing c_parser_error () to skip
tokens in some places and
not clearing it after (successfully) ending parsing of a function.
Not sure where clearing of parser->error happens usually, it somewhat
looks like it has
to be done manually somewhere up in the callstack (I suppose once we managed to
"recover"). Most c_parser_skip* routines clear error for example.
Richard.
>
> --
> Regards,
> Mikhail Maltsev
