Re: [PATCH 1/5][GIMPLE FE] PR testsuite/80580. Handle missing labels in goto statements

[Richard Biener <richard.guenther@gmail.com>]

On Tue, May 30, 2017 at 1:46 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Mon, May 29, 2017 at 6:38 AM, Mikhail Maltsev <maltsevm@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