On Tue, May 30, 2017 at 1:46 PM, Richard Biener
<richard.guent...@gmail.com> wrote:
> 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.
Oh, and the patch you posted is ok.
Thanks,
Richard.
> Richard.
>
>>
>> --
>> Regards,
>> Mikhail Maltsev
