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

[Mikhail Maltsev <maltsevm at gmail dot com>]

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.

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.

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?

-- 
Regards,
   Mikhail Maltsev

Attachment: wip1.patch
Description: Text document