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

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

[Mikhail Maltsev]

[-- Attachment #1: Type: text/plain, Size: 733 bytes --]

The first problem happens because we don't check for missing labels when parsing
'goto' statements. I.e.:

__GIMPLE() void fn1() {
  if (1)
    goto
}

The fix is pretty obvious: just add a check.
My question is: which functions should I use to produce diagnostics? The
surrounding code uses 'c_parser_error', but this function does not handle
locations very well (in fact, it uses input_location).

-- 
Regards,
   Mikhail Maltsev

gcc/testsuite/ChangeLog:

2017-05-01  Mikhail Maltsev  <maltsevm@gmail.com>

	* gcc.dg/gimplefe-error-4.c: New test.
	* gcc.dg/gimplefe-error-5.c: New test.


gcc/c/ChangeLog:

2017-05-01  Mikhail Maltsev  <maltsevm@gmail.com>

	* gimple-parser.c (c_parser_gimple_if_stmt): Check for empty labels.



[-- Attachment #2: 0001-GIMPLEFE-handle-missing-labels-in-goto-statements.patch --]
[-- Type: text/plain, Size: 2411 bytes --]

From 07453ba1bf0b1290cef54dcb028cb477b17966df Mon Sep 17 00:00:00 2001
From: Mikhail Maltsev <maltsevm@gmail.com>
Date: Fri, 24 Feb 2017 13:09:10 +0300
Subject: [PATCH 1/5] GIMPLEFE: handle missing labels in goto statements

---
 gcc/c/gimple-parser.c                   | 10 ++++++++++
 gcc/testsuite/gcc.dg/gimplefe-error-4.c |  7 +++++++
 gcc/testsuite/gcc.dg/gimplefe-error-5.c |  9 +++++++++
 3 files changed, 26 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/gimplefe-error-4.c
 create mode 100644 gcc/testsuite/gcc.dg/gimplefe-error-5.c

diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c
index 0d6384b..a99b502 100644
--- a/gcc/c/gimple-parser.c
+++ b/gcc/c/gimple-parser.c
@@ -1315,6 +1315,11 @@ c_parser_gimple_if_stmt (c_parser *parser, gimple_seq *seq)
       loc = c_parser_peek_token (parser)->location;
       c_parser_consume_token (parser);
       label = c_parser_peek_token (parser)->value;
+      if (! label)
+	{
+	  c_parser_error (parser, "expected label");
+	  return;
+	}
       t_label = lookup_label_for_goto (loc, label);
       c_parser_consume_token (parser);
       if (! c_parser_require (parser, CPP_SEMICOLON, "expected %<;%>"))
@@ -1339,6 +1344,11 @@ c_parser_gimple_if_stmt (c_parser *parser, gimple_seq *seq)
       loc = c_parser_peek_token (parser)->location;
       c_parser_consume_token (parser);
       label = c_parser_peek_token (parser)->value;
+      if (! label)
+	{
+	  c_parser_error (parser, "expected label");
+	  return;
+	}
       f_label = lookup_label_for_goto (loc, label);
       c_parser_consume_token (parser);
       if (! c_parser_require (parser, CPP_SEMICOLON, "expected %<;%>"))
diff --git a/gcc/testsuite/gcc.dg/gimplefe-error-4.c b/gcc/testsuite/gcc.dg/gimplefe-error-4.c
new file mode 100644
index 0000000..c61539c
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/gimplefe-error-4.c
@@ -0,0 +1,7 @@
+/* { dg-do compile } */
+/* { dg-options "-fgimple" } */
+
+__GIMPLE() void fn1() {
+  if (1)
+    goto
+} /* { dg-error "expected label" } */
diff --git a/gcc/testsuite/gcc.dg/gimplefe-error-5.c b/gcc/testsuite/gcc.dg/gimplefe-error-5.c
new file mode 100644
index 0000000..7398861
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/gimplefe-error-5.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-fgimple" } */
+
+__GIMPLE() void fn1() {
+  if (1)
+    goto lbl;
+  else
+    goto
+} /* { dg-error "expected label" } */
-- 
2.1.4

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

[Richard Biener]

On Mon, May 1, 2017 at 8:04 PM, Mikhail Maltsev <maltsevm@gmail.com> wrote:
> The first problem happens because we don't check for missing labels when parsing
> 'goto' statements. I.e.:
>
> __GIMPLE() void fn1() {
>   if (1)
>     goto
> }
>
> The fix is pretty obvious: just add a check.
> My question is: which functions should I use to produce diagnostics? The
> surrounding code uses 'c_parser_error', but this function does not handle
> locations very well (in fact, it uses input_location).

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.

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;
     }

?

Patch is ok with or without this adjustment (and testcase adjustment).

Thanks,
Richard.

> --
> Regards,
>    Mikhail Maltsev
>
> gcc/testsuite/ChangeLog:
>
> 2017-05-01  Mikhail Maltsev  <maltsevm@gmail.com>
>
>         * gcc.dg/gimplefe-error-4.c: New test.
>         * gcc.dg/gimplefe-error-5.c: New test.
>
>
> gcc/c/ChangeLog:
>
> 2017-05-01  Mikhail Maltsev  <maltsevm@gmail.com>
>
>         * gimple-parser.c (c_parser_gimple_if_stmt): Check for empty labels.
>
>

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

[Mikhail Maltsev]

[-- Attachment #1: Type: text/plain, Size: 2965 bytes --]

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 #2: wip1.patch --]
[-- Type: text/plain, Size: 1941 bytes --]

diff --git a/gcc/c/gimple-parser.c b/gcc/c/gimple-parser.c
index ed9e7c5..44ca738 100644
--- a/gcc/c/gimple-parser.c
+++ b/gcc/c/gimple-parser.c
@@ -1336,9 +1336,14 @@ c_parser_gimple_if_stmt (c_parser *parser, gimple_seq *seq)
     {
       loc = c_parser_peek_token (parser)->location;
       c_parser_consume_token (parser);
+      if (! c_parser_next_token_is (parser, CPP_NAME))
+	{
+	  c_parser_error (parser, "expected label");
+	  return;
+	}
       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;
     }
@@ -1360,9 +1365,14 @@ c_parser_gimple_if_stmt (c_parser *parser, gimple_seq *seq)
     {
       loc = c_parser_peek_token (parser)->location;
       c_parser_consume_token (parser);
+      if (! c_parser_next_token_is (parser, CPP_NAME))
+	{
+	  c_parser_error (parser, "expected label");
+	  return;
+	}
       label = c_parser_peek_token (parser)->value;
-      f_label = lookup_label_for_goto (loc, label);
       c_parser_consume_token (parser);
+      f_label = lookup_label_for_goto (loc, label);
       if (! c_parser_require (parser, CPP_SEMICOLON, "expected %<;%>"))
 	return;
     }
diff --git a/gcc/testsuite/gcc.dg/gimplefe-error-7.c b/gcc/testsuite/gcc.dg/gimplefe-error-7.c
new file mode 100644
index 0000000..7d5ff37
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/gimplefe-error-7.c
@@ -0,0 +1,27 @@
+/* { dg-do compile } */
+/* { dg-options "-fgimple" } */
+
+__GIMPLE() void fn1()
+{
+  if (1)
+    goto
+  else /* { dg-error "expected label" } */
+    goto lbl;
+}
+
+__GIMPLE() void fn2()
+{
+  if (1)
+    goto lbl;
+  else
+    goto ; /* { dg-error "expected label" } */
+}
+
+__GIMPLE() void fn3()
+{
+  if (1)
+    goto lbl;
+  else
+    goto
+} /* { dg-error "expected label" } */
+

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

[Richard Biener]

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.

Richard.

>
> --
> Regards,
>    Mikhail Maltsev

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

[Richard Biener]

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