Patchwork Better error reporting for missing semicolons after a struct definition

Currently, when a struct, enum or union declaration lacks a terminating
semicolon, GCC usually goes on parsing what follows as a declspec and
prints the infamous "two or more data types in declaration specifiers"
error message.  This patch series instead makes GCC treat the declaration
separately if it includes a tagged type definition, and print a clearer
"expected ';', identifier or '('" error message.

An additional benefit is that when presented with something like

   struct f {}
   int a;

The variables will be given type "int" in the compiler instead of
"struct f", thus removing further cascading errors.

It should be possible to do something like this for C++ too, but I'm
not attempting it right now.  This patch depends on the -Wc++-compat
patch I posted because it also uses the typespec_kind field of struct
c_declspecs.

Bootstrapped and regtested x86_64-pc-linux-gnu, okay for mainline?

Paolo

2010-10-28  Paolo Bonzini  <bonzini@gnu.org>

        * c-parser.c (c_token_is_qualifier,
        c_parser_next_token_is_qualifier): New.
        (c_parser_declaration_or_fndef, c_parser_struct_declaration):
        Improve error message on specs->tagdef_seen_p.
        (c_parser_struct_or_union_specifier): Improve error recovery.
        (c_parser_declspecs): Move exit condition on C_ID_ID early.
        Reorganize exit condition for C_ID_TYPENAME/C_ID_CLASSNAME
        using c_parser_next_token_is_qualifier, and extend it to
        cover !typespec_ok in general, and also specs->tagdef_seen_p.



/* { dg-do compile } */
/* { dg-options "-std=gnu89" } // suppress default -pedantic-errors */

typedef int x, y;
x y z;			/* { dg-error "" "" } */


/* { dg-do compile } */
/* { dg-options "-std=gnu89" } // suppress default -pedantic-errors */

typedef int x, y;
x struct f z; /* { dg-error "two types " "" } */


/* { dg-do compile } */
/* { dg-options "-std=gnu89" } // suppress default -pedantic-errors */

struct f {
}
int z(); /* { dg-error "expected ';', identifier or " ""  { target *-*-* }  } */


/* { dg-do compile } */
/* { dg-options "-std=gnu89" } // suppress default -pedantic-errors */

int f()
{
  struct f {
  }
  int z; /* { dg-error "expected ';', identifier or " "" } */
}


/* { dg-do compile } */
/* { dg-options "-std=gnu89" } // suppress default -pedantic-errors */

struct f {}
struct g {} /* { dg-error "expected ';', identifier or " "" } */
int f(); /* { dg-error "expected ';', identifier or " "" } */


/* { dg-do compile } */
/* { dg-options "-std=gnu89" } // suppress default -pedantic-errors */

struct s {
  struct f {} /* dg-warning "does not declare anything" "" } */
  struct g {} x; /* { dg-error "expected ';', identifier or " "" } */
};


/* { dg-do compile } */
/* { dg-options "-std=gnu89" } // suppress default -pedantic-errors */

struct s {
  struct f {}
  enum a { X } /* { dg-error "expected ';', identifier or " "" } */
  struct g {} /* { dg-error "expected identifier " "" } */
}; /* { dg-warning "no semicolon" "" } */


/* { dg-do compile } */
/* { dg-options "-std=gnu89" } // suppress default -pedantic-errors */

enum x { XYZ }
struct g { enum x a; }; /* { dg-error "expected ';', identifier or " "" } */

int f(struct g *x)
{
  return x->a == XYZ; /* { dg-bogus " has no member " "" } */
}


/* { dg-do compile } */
/* { dg-options "-std=gnu89" } // suppress default -pedantic-errors */

struct f {}
static int a, b; /* { dg-error "expected ';', identifier or " "" } */

int f()
{
	return a - b; /* { dg-bogus "invalid operands " "" } */
}


/* { dg-do compile } */
/* { dg-options "-std=gnu89" } // suppress default -pedantic-errors */

typedef enum a { XYZ } a; /* { dg-message "previous declaration" } */
enum a a;       /* { dg-error "redeclared" } */
struct b { enum a a : 8; };


/* { dg-do compile } */
/* { dg-options "-std=gnu89" } // suppress default -pedantic-errors */

@interface foo
struct f {}
struct g { int a; }; /* { dg-error "expected ';', identifier or " "" } */

- (struct f *) a;
- (struct g *) b;
@end

int f(struct g *x)
{
  return x->a; /* { dg-bogus " has no member " "" } */
}

On Fri, 29 Oct 2010, Paolo Bonzini wrote:

> @@ -2400,6 +2469,21 @@ c_parser_struct_declaration (c_parser *p
>  	}
>        return ret;
>      }
> +
> +  /* Provide better error recovery.  Note that a type name here is valid,
> +     and will be treated as a field name.  */
> +  if (specs->typespec_kind == ctsk_tagdef
> +      && TREE_CODE (specs->type) != ENUMERAL_TYPE
> +      && c_parser_next_token_starts_declspecs (parser)
> +      && !c_parser_next_token_is (parser, CPP_NAME))
> +    {
> +      c_parser_error (parser, "expected %<;%>, identifier or %<(%>");
> +      parser->error = false;
> +      return NULL_TREE;
> +    }
> +  if (c_parser_next_token_is (parser, CPP_CLOSE_BRACE))
> +    return NULL_TREE;

I don't follow why you are returning NULL_TREE here (or why it is OK to do 
so).  It appears to me that it will quietly return NULL_TREE if you have 
declaration specifiers with no following declarator or semicolon.

Consider the following testcase.

struct s { struct { int a; } };
int *f(struct s *p) { return &p->a; }

Your patch doesn't apply cleanly to the sources I have, either because of 
subsequent changes or because of missing prerequisites, but it appears to 
me that with your patch the parser will accept the struct s declaration 
(diagnosing the missing semicolon at end of structure with a pedwarn) but 
not actually do anything with the anonymous structure element because the 
code above only handles anonymous structs with following semicolons.  The 
current compiler gives parse errors on this code.  The *correct* behavior 
I think is neither - it should be accepted with the anonymous structure 
processed (so the check above should allow for CPP_CLOSE_BRACE as well as 
CPP_SEMICOLON), like 4.0 and earlier did with the old parser.

Otherwise the changes in this patch look correct.

On 11/05/2010 07:04 PM, Joseph S. Myers wrote:
> struct s { struct { int a; } };
> int *f(struct s *p) { return &p->a; }
>
> The *correct*  behavior I think is neither - it should be accepted
> with the anonymous structure processed (so the check above should
> allow for CPP_CLOSE_BRACE as well as CPP_SEMICOLON), like 4.0 and
> earlier did with the old parser.

I agree, I'll take a look at this testcase.

Thanks for the review!

Paolo