diff mbox

[C] Warn for _Alignas in an array declarator (PR c/58267)

Message ID 20131016151956.GE10967@redhat.com
State New
Headers show

Commit Message

Marek Polacek Oct. 16, 2013, 3:19 p.m. UTC 
On Wed, Oct 16, 2013 at 12:28:32PM +0000, Joseph S. Myers wrote:
> On Wed, 16 Oct 2013, Marek Polacek wrote:
> 
> > This PR is about _Alignas in contexts like
> > char x[_Alignas (int) 20];
> > C grammar does not allow _Alignas to be in an array declarator, yet we
> > don't issue any warning or an error.  This patch implements a pedwarn
> > for this.  I'm however highly unsure whether we want pedwarn, error, or
> > normal warning for this - currently we just DTRT and ignore the
> > _Alignas altogether.
> 
> I think this should be an error.

Makes sense.
 
> I see this as a problem with how the parsing of array declarators uses 
> c_parser_declspecs to parse a sequence of qualifiers and attributes, with 
> scspec_ok=false and typespec_ok=false to ensure those kinds of specifiers 
> can't occur in the list, but without any measure to exclude alignment 
> specifiers.  That is, I think c_parser_declspecs should gain an argument 
> specifying whether alignment specifiers are OK, and the relevant calls 
> from c_parser_direct_declarator_inner would pass false for that argument, 
> and uses of alignment specifiers here would result in parse errors.

Thanks, should be done in the attached patch.

> (Incidentally, the comments in c-parser.c that are supposed to give the 
> grammar accepted are missing the syntax of array-declarator.)

I've fixed that up.

> I think this should also be tested after "static" (given that there are 
> two separate calls to c_parser_declspecs involved), as well as in an 
> abstract declarator.

Done.

Regtested/bootstrapped on x86_64-linux, ok for trunk?

2013-10-16  Marek Polacek  <polacek@redhat.com>

	PR c/58267
c/
	* c-parser.c (c_parser_declspecs): Add alignspec_ok parameter.
	Document syntax of the array-declarator.
	(c_parser_declspecs) <RID_ALIGNAS>: Bail out if alignment specs
	are not permitted.
	(c_parser_declaration_or_fndef): Adjust c_parser_declspecs call.
	(c_parser_struct_declaration): Likewise.
	(c_parser_declarator): Likewise.
	(c_parser_direct_declarator_inner): Likewise.
	(c_parser_parameter_declaration): Likewise.
	(c_parser_type_name): Likewise.
testsuite/
	* gcc.dg/c1x-align-5.c: New test.


	Marek

Comments

Joseph Myers Oct. 16, 2013, 5:02 p.m. UTC | #1 
On Wed, 16 Oct 2013, Marek Polacek wrote:

> Regtested/bootstrapped on x86_64-linux, ok for trunk?
> 
> 2013-10-16  Marek Polacek  <polacek@redhat.com>
> 
> 	PR c/58267
> c/
> 	* c-parser.c (c_parser_declspecs): Add alignspec_ok parameter.
> 	Document syntax of the array-declarator.
> 	(c_parser_declspecs) <RID_ALIGNAS>: Bail out if alignment specs
> 	are not permitted.
> 	(c_parser_declaration_or_fndef): Adjust c_parser_declspecs call.
> 	(c_parser_struct_declaration): Likewise.
> 	(c_parser_declarator): Likewise.
> 	(c_parser_direct_declarator_inner): Likewise.
> 	(c_parser_parameter_declaration): Likewise.
> 	(c_parser_type_name): Likewise.
> testsuite/
> 	* gcc.dg/c1x-align-5.c: New test.

OK.
Joseph Myers Oct. 19, 2013, 7:23 p.m. UTC | #2 
On Wed, 16 Oct 2013, Marek Polacek wrote:

> @@ -2946,7 +2957,8 @@ c_parser_declarator (c_parser *parser, b
>        struct c_declspecs *quals_attrs = build_null_declspecs ();
>        struct c_declarator *inner;
>        c_parser_consume_token (parser);
> -      c_parser_declspecs (parser, quals_attrs, false, false, true, cla_prefer_id);
> +      c_parser_declspecs (parser, quals_attrs, false, false, true,
> +			  true, cla_prefer_id);
>        inner = c_parser_declarator (parser, type_seen_p, kind, seen_id);
>        if (inner == NULL)
>  	return NULL;

Looking at this again, shouldn't the new argument be "false" (with 
associated testcase)?  This is parsing pointer declarators, and _Alignas 
isn't allowed there any more than it is in array declarators....

> @@ -3715,7 +3730,8 @@ c_parser_type_name (c_parser *parser)
>    struct c_declarator *declarator;
>    struct c_type_name *ret;
>    bool dummy = false;
> -  c_parser_declspecs (parser, specs, false, true, true, cla_prefer_type);
> +  c_parser_declspecs (parser, specs, false, true, true, false,
> +		      cla_prefer_type);
>    if (!specs->declspecs_seen_p)
>      {
>        c_parser_error (parser, "expected specifier-qualifier-list");

And this should get a testcase added, that _Alignas is correctly rejected 
in type names where previously it would have been wrongly accepted.

(Strictly by the standard it should be "false" in 
c_parser_struct_declaration as well - the syntax there doesn't allow 
_Alignas - but it appears to have been intended to allow it there, so 
probably best not to change anything there until WG14 reaches a conclusion 
on the issues I raised in N1731.)
diff mbox

Patch

--- gcc/c/c-parser.c.mp	2013-10-16 15:00:36.774530501 +0200
+++ gcc/c/c-parser.c	2013-10-16 16:31:30.731611508 +0200
@@ -1124,7 +1124,7 @@  static void c_parser_declaration_or_fnde
 static void c_parser_static_assert_declaration_no_semi (c_parser *);
 static void c_parser_static_assert_declaration (c_parser *);
 static void c_parser_declspecs (c_parser *, struct c_declspecs *, bool, bool,
-				bool, enum c_lookahead_kind);
+				bool, bool, enum c_lookahead_kind);
 static struct c_typespec c_parser_enum_specifier (c_parser *);
 static struct c_typespec c_parser_struct_or_union_specifier (c_parser *);
 static tree c_parser_struct_declaration (c_parser *);
@@ -1494,7 +1494,8 @@  c_parser_declaration_or_fndef (c_parser
       fndef_ok = !nested;
     }
 
-  c_parser_declspecs (parser, specs, true, true, start_attr_ok, cla_nonabstract_decl);
+  c_parser_declspecs (parser, specs, true, true, start_attr_ok,
+		      true, cla_nonabstract_decl);
   if (parser->error)
     {
       c_parser_skip_to_end_of_block_or_statement (parser);
@@ -1942,8 +1943,9 @@  c_parser_static_assert_declaration_no_se
 /* Parse some declaration specifiers (possibly none) (C90 6.5, C99
    6.7), adding them to SPECS (which may already include some).
    Storage class specifiers are accepted iff SCSPEC_OK; type
-   specifiers are accepted iff TYPESPEC_OK; attributes are accepted at
-   the start iff START_ATTR_OK.
+   specifiers are accepted iff TYPESPEC_OK; alignment specifiers are
+   accepted iff ALIGNSPEC_OK; attributes are accepted at the start
+   iff START_ATTR_OK.
 
    declaration-specifiers:
      storage-class-specifier declaration-specifiers[opt]
@@ -2039,7 +2041,7 @@  c_parser_static_assert_declaration_no_se
 static void
 c_parser_declspecs (c_parser *parser, struct c_declspecs *specs,
 		    bool scspec_ok, bool typespec_ok, bool start_attr_ok,
-		    enum c_lookahead_kind la)
+		    bool alignspec_ok, enum c_lookahead_kind la)
 {
   bool attrs_ok = start_attr_ok;
   bool seen_type = specs->typespec_kind != ctsk_none;
@@ -2235,6 +2237,8 @@  c_parser_declspecs (c_parser *parser, st
 	  declspecs_add_attrs (loc, specs, attrs);
 	  break;
 	case RID_ALIGNAS:
+	  if (!alignspec_ok)
+	    goto out;
 	  align = c_parser_alignas_specifier (parser);
 	  declspecs_add_alignas (loc, specs, align);
 	  break;
@@ -2640,7 +2644,8 @@  c_parser_struct_declaration (c_parser *p
     }
   specs = build_null_declspecs ();
   decl_loc = c_parser_peek_token (parser)->location;
-  c_parser_declspecs (parser, specs, false, true, true, cla_nonabstract_decl);
+  c_parser_declspecs (parser, specs, false, true, true,
+		      true, cla_nonabstract_decl);
   if (parser->error)
     return NULL_TREE;
   if (!specs->declspecs_seen_p)
@@ -2888,6 +2893,12 @@  c_parser_alignas_specifier (c_parser * p
      type-qualifier-list type-qualifier
      type-qualifier-list attributes
 
+   array-declarator:
+     [ type-qualifier-list[opt] assignment-expression[opt] ]
+     [ static type-qualifier-list[opt] assignment-expression ]
+     [ type-qualifier-list static assignment-expression ]
+     [ type-qualifier-list[opt] * ]
+
    parameter-type-list:
      parameter-list
      parameter-list , ...
@@ -2946,7 +2957,8 @@  c_parser_declarator (c_parser *parser, b
       struct c_declspecs *quals_attrs = build_null_declspecs ();
       struct c_declarator *inner;
       c_parser_consume_token (parser);
-      c_parser_declspecs (parser, quals_attrs, false, false, true, cla_prefer_id);
+      c_parser_declspecs (parser, quals_attrs, false, false, true,
+			  true, cla_prefer_id);
       inner = c_parser_declarator (parser, type_seen_p, kind, seen_id);
       if (inner == NULL)
 	return NULL;
@@ -3098,12 +3110,14 @@  c_parser_direct_declarator_inner (c_pars
       bool star_seen;
       tree dimen;
       c_parser_consume_token (parser);
-      c_parser_declspecs (parser, quals_attrs, false, false, true, cla_prefer_id);
+      c_parser_declspecs (parser, quals_attrs, false, false, true,
+			  false, cla_prefer_id);
       static_seen = c_parser_next_token_is_keyword (parser, RID_STATIC);
       if (static_seen)
 	c_parser_consume_token (parser);
       if (static_seen && !quals_attrs->declspecs_seen_p)
-	c_parser_declspecs (parser, quals_attrs, false, false, true, cla_prefer_id);
+	c_parser_declspecs (parser, quals_attrs, false, false, true,
+			    false, cla_prefer_id);
       if (!quals_attrs->declspecs_seen_p)
 	quals_attrs = NULL;
       /* If "static" is present, there must be an array dimension.
@@ -3406,7 +3420,8 @@  c_parser_parameter_declaration (c_parser
       declspecs_add_attrs (input_location, specs, attrs);
       attrs = NULL_TREE;
     }
-  c_parser_declspecs (parser, specs, true, true, true, cla_nonabstract_decl);
+  c_parser_declspecs (parser, specs, true, true, true, true,
+		      cla_nonabstract_decl);
   finish_declspecs (specs);
   pending_xref_error ();
   prefix_attrs = specs->attrs;
@@ -3715,7 +3730,8 @@  c_parser_type_name (c_parser *parser)
   struct c_declarator *declarator;
   struct c_type_name *ret;
   bool dummy = false;
-  c_parser_declspecs (parser, specs, false, true, true, cla_prefer_type);
+  c_parser_declspecs (parser, specs, false, true, true, false,
+		      cla_prefer_type);
   if (!specs->declspecs_seen_p)
     {
       c_parser_error (parser, "expected specifier-qualifier-list");
--- gcc/testsuite/gcc.dg/c1x-align-5.c.mp	2013-10-16 16:15:09.704916186 +0200
+++ gcc/testsuite/gcc.dg/c1x-align-5.c	2013-10-16 16:27:50.920786400 +0200
@@ -0,0 +1,20 @@ 
+/* Test C1X alignment support.  Test invalid code.  */
+/* { dg-do compile } */
+/* { dg-options "-std=c1x -pedantic-errors" } */
+
+void foo (int []);
+void bar1 (int [_Alignas (double) 10]); /* { dg-error "expected expression before" } */
+void bar2 (int [static _Alignas (double) 10]); /* { dg-error "expected expression before" } */
+void bar3 (int [static const _Alignas (double) 10]); /* { dg-error "expected expression before" } */
+void bar4 (int [const _Alignas (double) 10]); /* { dg-error "expected expression before" } */
+void bar5 (int [_Alignas (0) *]); /* { dg-error "expected expression before" } */
+
+void foo (int a[_Alignas (0) 10]) { } /* { dg-error "expected expression before" } */
+
+void
+test (void)
+{
+  int a[_Alignas (int) 10]; /* { dg-error "expected expression before" } */
+  int b[10];
+  foo (b);
+}