C: detect more missing semicolons (PR c/7356)

Message ID 1507750371-60739-1-git-send-email-dmalcolm@redhat.com
State New
Headers show
Series
  • C: detect more missing semicolons (PR c/7356)
Related show

Commit Message

David Malcolm Oct. 11, 2017, 7:32 p.m.
[This patch assumes the two patches here:
 https://gcc.gnu.org/ml/gcc-patches/2017-10/msg00274.html ]

c_parser_declaration_or_fndef has logic for parsing what might be
either a declaration or a function definition.

This patch adds a test to detect cases where a semicolon would have
terminated the decls as a declaration, where the token that follows
would start a new declaration specifier, and updates the error message
accordingly, with a fix-it hint.

This addresses PR c/7356, fixing the case of a stray token before a
#include which previously gave inscrutable output, and improving e.g.:

  int i
  int j;

from:

  t.c:2:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'int'
   int j;
   ^~~

to:

  t.c:1:6: error: expected ';' before 'int'
   int i
        ^
        ;
   int j;
   ~~~

The patch also adds a test for PR c/44515 as a baseline.

gcc.dg/noncompile/920923-1.c needs a slight update, as the output for
the first line changes from:

  920923-1.c:2:14: error: expected '=', ',', ';', 'asm' or
  '__attribute__' before 'unsigned'
   typedef BYTE unsigned char; /* { dg-error "expected" } */
                ^~~~~~~~

to:
  920923-1.c:2:13: error: expected ';' before 'unsigned'
   typedef BYTE unsigned char; /* { dg-error "expected" } */
               ^~~~~~~~~
               ;
  920923-1.c:2:1: warning: useless type name in empty declaration
   typedef BYTE unsigned char; /* { dg-error "expected" } */
   ^~~~~~~

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
OK for trunk?

Thanks
Dave

gcc/c/ChangeLog:
	PR c/7356
	* c-parser.c (c_parser_declaration_or_fndef): Detect missing
	semicolons.

gcc/testsuite/ChangeLog:
	PR c/7356
	PR c/44515
	* c-c++-common/pr44515.c: New test case.
	* gcc.dg/pr7356-2.c: New test case.
	* gcc.dg/pr7356.c: New test case.
	* gcc.dg/spellcheck-typenames.c: Update the "singed" char "TODO"
	case to reflect changes to output.
	* gcc.dg/noncompile/920923-1.c: Add dg-warning to reflect changes
	to output.
---
 gcc/c/c-parser.c                            | 36 +++++++++++++++++++++++++----
 gcc/testsuite/c-c++-common/pr44515.c        | 14 +++++++++++
 gcc/testsuite/gcc.dg/noncompile/920923-1.c  |  1 +
 gcc/testsuite/gcc.dg/pr7356-2.c             | 33 ++++++++++++++++++++++++++
 gcc/testsuite/gcc.dg/pr7356.c               | 17 ++++++++++++++
 gcc/testsuite/gcc.dg/spellcheck-typenames.c |  5 ++--
 6 files changed, 99 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/pr44515.c
 create mode 100644 gcc/testsuite/gcc.dg/pr7356-2.c
 create mode 100644 gcc/testsuite/gcc.dg/pr7356.c

Comments

Jeff Law Oct. 25, 2017, 3:59 p.m. | #1
On 10/11/2017 01:32 PM, David Malcolm wrote:
> [This patch assumes the two patches here:
>  https://gcc.gnu.org/ml/gcc-patches/2017-10/msg00274.html ]
I see the patch directly referenced in that message on the trunk.  But
I'm not sure what you mean by "two patches".  If there's a prereq that
hasn't been approved, let me know.


> 
> c_parser_declaration_or_fndef has logic for parsing what might be
> either a declaration or a function definition.
> 
> This patch adds a test to detect cases where a semicolon would have
> terminated the decls as a declaration, where the token that follows
> would start a new declaration specifier, and updates the error message
> accordingly, with a fix-it hint.
> 
> This addresses PR c/7356, fixing the case of a stray token before a
> #include which previously gave inscrutable output, and improving e.g.:
> 
>   int i
>   int j;
> 
> from:
> 
>   t.c:2:1: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'int'
>    int j;
>    ^~~
> 
> to:
> 
>   t.c:1:6: error: expected ';' before 'int'
>    int i
>         ^
>         ;
>    int j;
>    ~~~
> 
> The patch also adds a test for PR c/44515 as a baseline.
Personally I find the current error easier to digest.  It quite clearly
tells me to look before the current token and shove in an appropriate
thingie :-)   The extra vertical space, to me, makes the error message
harder to parse.

But I can see how others would prefer to see the point where the
punctuation was missing.  So I won't let my biases get in the way here.



> 
> gcc/c/ChangeLog:
> 	PR c/7356
> 	* c-parser.c (c_parser_declaration_or_fndef): Detect missing
> 	semicolons.
> 
> gcc/testsuite/ChangeLog:
> 	PR c/7356
> 	PR c/44515
> 	* c-c++-common/pr44515.c: New test case.
> 	* gcc.dg/pr7356-2.c: New test case.
> 	* gcc.dg/pr7356.c: New test case.
> 	* gcc.dg/spellcheck-typenames.c: Update the "singed" char "TODO"
> 	case to reflect changes to output.
> 	* gcc.dg/noncompile/920923-1.c: Add dg-warning to reflect changes
> 	to output.
OK.
jeff
David Malcolm Oct. 26, 2017, 12:17 a.m. | #2
On Wed, 2017-10-25 at 09:59 -0600, Jeff Law wrote:
> On 10/11/2017 01:32 PM, David Malcolm wrote:
> > [This patch assumes the two patches here:
> >  https://gcc.gnu.org/ml/gcc-patches/2017-10/msg00274.html ]
> 
> I see the patch directly referenced in that message on the
> trunk.  But
> I'm not sure what you mean by "two patches".  If there's a prereq
> that
> hasn't been approved, let me know.

Sorry about the confusion; the one at the URL above was:

"[PATCH 2/2] C/C++: add fix-it hints for various missing symbols (v3)",

which is an updated version of:

"[PATCH 2/2] C/C++: add fix-it hints for various missing symbols (v2)"
https://gcc.gnu.org/ml/gcc-patches/2017-09/msg01746.html

the other one I was referring to was:

"[PATCH 1/2] C++: avoid partial duplicate implementation of
cp_parser_error"
https://gcc.gnu.org/ml/gcc-patches/2017-09/msg01745.html


Both these prereqs are in trunk (as r253686 and r253690 respectively).

> 
> > 
> > c_parser_declaration_or_fndef has logic for parsing what might be
> > either a declaration or a function definition.
> > 
> > This patch adds a test to detect cases where a semicolon would have
> > terminated the decls as a declaration, where the token that follows
> > would start a new declaration specifier, and updates the error
> > message
> > accordingly, with a fix-it hint.
> > 
> > This addresses PR c/7356, fixing the case of a stray token before a
> > #include which previously gave inscrutable output, and improving
> > e.g.:
> > 
> >   int i
> >   int j;
> > 
> > from:
> > 
> >   t.c:2:1: error: expected '=', ',', ';', 'asm' or '__attribute__'
> > before 'int'
> >    int j;
> >    ^~~
> > 
> > to:
> > 
> >   t.c:1:6: error: expected ';' before 'int'
> >    int i
> >         ^
> >         ;
> >    int j;
> >    ~~~
> > 
> > The patch also adds a test for PR c/44515 as a baseline.
> 
> Personally I find the current error easier to digest.  It quite
> clearly
> tells me to look before the current token and shove in an appropriate
> thingie :-)   The extra vertical space, to me, makes the error
> message
> harder to parse.

You've been using gcc for multiple decades and are used to the existing
behavior, though ;)

> But I can see how others would prefer to see the point where the
> punctuation was missing.  So I won't let my biases get in the way
> here.

FWIW the benefit is a *lot* clearer for the first case described, where
the two locations are in different source files due to the missing
semicolon being immediately before a #include (PR c/7356), or at the
end of a header; it replaces dozens of crazy-looking errors with a
single sane one.

> > gcc/c/ChangeLog:
> > 	PR c/7356
> > 	* c-parser.c (c_parser_declaration_or_fndef): Detect missing
> > 	semicolons.
> > 
> > gcc/testsuite/ChangeLog:
> > 	PR c/7356
> > 	PR c/44515
> > 	* c-c++-common/pr44515.c: New test case.
> > 	* gcc.dg/pr7356-2.c: New test case.
> > 	* gcc.dg/pr7356.c: New test case.
> > 	* gcc.dg/spellcheck-typenames.c: Update the "singed" char
> > "TODO"
> > 	case to reflect changes to output.
> > 	* gcc.dg/noncompile/920923-1.c: Add dg-warning to reflect
> > changes
> > 	to output.
> 
> OK.

Thanks; committed to trunk as r254093 (and closing out PR c/7356 after
15 years!)

Dave

Patch

diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index a5e3ec4..7c3b834 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -2241,11 +2241,37 @@  c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok,
 	}
       if (!start_function (specs, declarator, all_prefix_attrs))
 	{
-	  /* This can appear in many cases looking nothing like a
-	     function definition, so we don't give a more specific
-	     error suggesting there was one.  */
-	  c_parser_error (parser, "expected %<=%>, %<,%>, %<;%>, %<asm%> "
-			  "or %<__attribute__%>");
+	  /* At this point we've consumed:
+	       declaration-specifiers declarator
+	     and the next token isn't CPP_EQ, CPP_COMMA, CPP_SEMICOLON,
+	     RID_ASM, RID_ATTRIBUTE, or RID_IN,
+	     but the
+	       declaration-specifiers declarator
+	     aren't grokkable as a function definition, so we have
+	     an error.  */
+	  gcc_assert (!c_parser_next_token_is (parser, CPP_SEMICOLON));
+	  if (c_parser_next_token_starts_declspecs (parser))
+	    {
+	      /* If we have
+		   declaration-specifiers declarator decl-specs
+		 then assume we have a missing semicolon, which would
+		 give us:
+		   declaration-specifiers declarator  decl-specs
+						    ^
+						    ;
+		   <~~~~~~~~~ declaration ~~~~~~~~~~>
+		 Use c_parser_require to get an error with a fix-it hint.  */
+	      c_parser_require (parser, CPP_SEMICOLON, "expected %<;%>");
+	      parser->error = false;
+	    }
+	  else
+	    {
+	      /* This can appear in many cases looking nothing like a
+		 function definition, so we don't give a more specific
+		 error suggesting there was one.  */
+	      c_parser_error (parser, "expected %<=%>, %<,%>, %<;%>, %<asm%> "
+			      "or %<__attribute__%>");
+	    }
 	  if (nested)
 	    c_pop_function_context ();
 	  break;
diff --git a/gcc/testsuite/c-c++-common/pr44515.c b/gcc/testsuite/c-c++-common/pr44515.c
new file mode 100644
index 0000000..dbb77509
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/pr44515.c
@@ -0,0 +1,14 @@ 
+/* { dg-options "-fdiagnostics-show-caret" } */
+
+void bar(void);
+void foo(void)
+{
+  bar() /* { dg-error "expected ';' before '.' token" } */
+}
+/* { dg-begin-multiline-output "" }
+   bar()
+        ^
+        ;
+ }
+ ~       
+   { dg-end-multiline-output "" } */
diff --git a/gcc/testsuite/gcc.dg/noncompile/920923-1.c b/gcc/testsuite/gcc.dg/noncompile/920923-1.c
index 1cb140e..006a071 100644
--- a/gcc/testsuite/gcc.dg/noncompile/920923-1.c
+++ b/gcc/testsuite/gcc.dg/noncompile/920923-1.c
@@ -1,5 +1,6 @@ 
 /* { dg-message "undeclared identifier is reported only once" "reminder for mmu_base" { target *-*-* } 0 } */
 typedef BYTE unsigned char;	/* { dg-error "expected" } */
+/* { dg-warning "useless type name in empty declaration" ""  { target *-*-* } .-1 } */
 typedef int item_n;
 typedef int perm_set;
 struct PENT { caddr_t v_addr; };/* { dg-error "unknown type name" } */
diff --git a/gcc/testsuite/gcc.dg/pr7356-2.c b/gcc/testsuite/gcc.dg/pr7356-2.c
new file mode 100644
index 0000000..ad67975
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr7356-2.c
@@ -0,0 +1,33 @@ 
+/* { dg-options "-fdiagnostics-show-caret" } */
+
+int i /* { dg-error "6: expected ';' before 'int'" } */
+int j;
+/* { dg-begin-multiline-output "" }
+ int i 
+      ^
+      ;
+ int j;
+ ~~~   
+   { dg-end-multiline-output "" } */
+
+
+void test (void)
+{
+  int i /* { dg-error "8: expected ';' before 'int'" } */
+  int j;
+
+  /* { dg-begin-multiline-output "" }
+   int i 
+        ^
+        ;
+   int j;
+   ~~~   
+     { dg-end-multiline-output "" } */
+}
+
+int old_style_params (first, second)
+     int first;
+     int second;
+{
+  return first + second;
+}
diff --git a/gcc/testsuite/gcc.dg/pr7356.c b/gcc/testsuite/gcc.dg/pr7356.c
new file mode 100644
index 0000000..84baf07
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr7356.c
@@ -0,0 +1,17 @@ 
+/* { dg-options "-fdiagnostics-show-caret" } */
+
+a /* { dg-line stray_token } */
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+int main(int argc, char** argv)
+{
+  return 0;
+}
+
+/* { dg-error "expected ';' before '.*'" "" { target *-*-* } stray_token } */
+/* { dg-begin-multiline-output "" }
+ a
+  ^
+  ;
+   { dg-end-multiline-output "" } */
diff --git a/gcc/testsuite/gcc.dg/spellcheck-typenames.c b/gcc/testsuite/gcc.dg/spellcheck-typenames.c
index f3b8102..3717ad8 100644
--- a/gcc/testsuite/gcc.dg/spellcheck-typenames.c
+++ b/gcc/testsuite/gcc.dg/spellcheck-typenames.c
@@ -100,8 +100,9 @@  baz value; /* { dg-error "1: unknown type name .baz.; use .enum. keyword to refe
    { dg-end-multiline-output "" } */
 
 /* TODO: it would be better to detect the "singed" vs "signed" typo here.  */
-singed char ch; /* { dg-error "8: before .char." } */
+singed char ch; /* { dg-error "7: before .char." } */
 /* { dg-begin-multiline-output "" }
  singed char ch;
-        ^~~~
+       ^~~~~
+       ;
    { dg-end-multiline-output "" } */