[PATCH] sparse: add support for static assert

[PATCH] sparse: add support for static assert

[Lance Richardson]

This patch introduces support for _Static_assert() in global,
function, and struct/union declaration contexts (as currently supported
by gcc).

Tested via:
   - kernel build with C=1 CF=-D__CHECK_ENDIAN__
   - build/check large code base making heavy use of _Static_assert()
   - "make check" with added test case for static assert support

Signed-off-by: Lance Richardson <lrichard@redhat.com>
---
 parse.c                    | 33 ++++++++++++++++++++++++++++++++-
 validation/static_assert.c | 20 ++++++++++++++++++++
 2 files changed, 52 insertions(+), 1 deletion(-)
 create mode 100644 validation/static_assert.c

diff --git a/parse.c b/parse.c
index b43d683..0b1a7f2 100644
--- a/parse.c
+++ b/parse.c
@@ -57,7 +57,8 @@ static declarator_t
 	attribute_specifier, typeof_specifier, parse_asm_declarator,
 	typedef_specifier, inline_specifier, auto_specifier,
 	register_specifier, static_specifier, extern_specifier,
-	thread_specifier, const_qualifier, volatile_qualifier;
+	thread_specifier, const_qualifier, volatile_qualifier,
+	static_assert_specifier;
 
 static struct token *parse_if_statement(struct token *token, struct statement *stmt);
 static struct token *parse_return_statement(struct token *token, struct statement *stmt);
@@ -308,6 +309,11 @@ static struct symbol_op asm_op = {
 	.toplevel = toplevel_asm_declaration,
 };
 
+static struct symbol_op static_assert_op = {
+	.type = KW_ATTRIBUTE,
+	.declarator = static_assert_specifier,
+};
+
 static struct symbol_op packed_op = {
 	.attribute = attribute_packed,
 };
@@ -437,6 +443,10 @@ static struct init_keyword {
 	{ "__restrict",	NS_TYPEDEF, .op = &restrict_op},
 	{ "__restrict__",	NS_TYPEDEF, .op = &restrict_op},
 
+
+	/* Static assertion */
+	{ "_Static_assert", NS_TYPEDEF, .op = &static_assert_op },
+
 	/* Storage class */
 	{ "auto",	NS_TYPEDEF, .op = &auto_op },
 	{ "register",	NS_TYPEDEF, .op = &register_op },
@@ -2004,6 +2014,27 @@ static struct token *parse_asm_declarator(struct token *token, struct decl_state
 	return token;
 }
 
+
+static struct token *static_assert_specifier(struct token *token, struct decl_state *ctx)
+{
+	struct expression *expr = NULL;
+	int val;
+
+	token = constant_expression(token->next, &expr);
+	if (!expr)
+		sparse_error(token->pos, "Expected constant expression");
+	val = get_expression_value(expr);
+	token = expect(token, ',', "after first argument of _Static_assert");
+	token = parse_expression(token, &expr);
+	token = expect(token, ')', "after second argument of _Static_assert");
+
+	if (!val)
+		sparse_error(token->pos, "static assertion failed: %s",
+                             show_string(expr->string));
+
+	return token;
+}
+
 /* Make a statement out of an expression */
 static struct statement *make_statement(struct expression *expr)
 {
diff --git a/validation/static_assert.c b/validation/static_assert.c
new file mode 100644
index 0000000..baab346
--- /dev/null
+++ b/validation/static_assert.c
@@ -0,0 +1,20 @@
+_Static_assert(1, "global ok");
+
+struct foo {
+	_Static_assert(1, "struct ok");
+};
+
+void bar(void)
+{
+	_Static_assert(1, " func ok");
+}
+
+_Static_assert(0, "expected failure");
+/*
+ * check-name: static assertion
+ *
+ * check-error-start
+static_assert.c:12:38: error: static assertion failed: "expected failure"
+ * check-error-end
+ */
+
-- 
2.5.0

Re: [PATCH] sparse: add support for static assert

[Luc Van Oostenryck]

On Wed, Jan 06, 2016 at 01:35:17PM -0500, Lance Richardson wrote:
> This patch introduces support for _Static_assert() in global,
> function, and struct/union declaration contexts (as currently supported
> by gcc).
> 
> Tested via:
>    - kernel build with C=1 CF=-D__CHECK_ENDIAN__
>    - build/check large code base making heavy use of _Static_assert()
>    - "make check" with added test case for static assert support

Nice, it was something that was indeed missing.

I have a few remarks, mostly nitpicking, here under.


Luc

> @@ -437,6 +443,10 @@ static struct init_keyword {
>  	{ "__restrict",	NS_TYPEDEF, .op = &restrict_op},
>  	{ "__restrict__",	NS_TYPEDEF, .op = &restrict_op},
>  
> +
> +	/* Static assertion */
> +	{ "_Static_assert", NS_TYPEDEF, .op = &static_assert_op },
> +

It seems a bit strange to me to use NS_TYPEDEF, as this is unrelated types.
OTOH, the other namespaces deosn't seems better suited,
and yes C11 define this as sort of declaration, so ...

> @@ -2004,6 +2014,27 @@ static struct token *parse_asm_declarator(struct token *token, struct decl_state
>  	return token;
>  }
>  
> +
> +static struct token *static_assert_specifier(struct token *token, struct decl_state *ctx)
> +{
> +	struct expression *expr = NULL;
> +	int val;
> +
> +	token = constant_expression(token->next, &expr);
> +	if (!expr)
> +		sparse_error(token->pos, "Expected constant expression");
> +	val = get_expression_value(expr);
> +	token = expect(token, ',', "after first argument of _Static_assert");
> +	token = parse_expression(token, &expr);
> +	token = expect(token, ')', "after second argument of _Static_assert");
> +
> +	if (!val)
> +		sparse_error(token->pos, "static assertion failed: %s",
> +                             show_string(expr->string));

By using token->pos here, the error message will indicate that the problem is
situated at the very end of the assertion; more exactly, at the ending ";".
This is a little annoying I find.
It would be better to use the some position as the expression
(expr->pos, or the same as for "Expected constant expression").

I find also a bit strange to have this sort of semantic check inside the parser.
 
> diff --git a/validation/static_assert.c b/validation/static_assert.c
> new file mode 100644
> index 0000000..baab346
> --- /dev/null
> +++ b/validation/static_assert.c
> @@ -0,0 +1,20 @@
> +_Static_assert(1, "global ok");
> +
> +struct foo {
> +	_Static_assert(1, "struct ok");
> +};
> +
> +void bar(void)
> +{
> +	_Static_assert(1, " func ok");
> +}
> +
> +_Static_assert(0, "expected failure");
> +/*
> + * check-name: static assertion
> + *
> + * check-error-start
> +static_assert.c:12:38: error: static assertion failed: "expected failure"
> + * check-error-end
> + */

It would be nice to add a few more test cases,
I'm thinbking to things like:
	static int f;
	_Static_assert(f, "non-constant expression");
	static int *p;
	_Static_assert(p, "non-integer expression");
	_Static_assert(0.1, "float expression");
	
	_Static_assert(!0 == 1, "non-trivial expression");
	
	static int array[4];
	_Static_assert(sizeof(array), "sizeof expression");
	
	static const char non_literal_string[] = "non literal string";
	_Static_assert(0, non_literal_string);

Also, what should happen with:
	_Static_assert(1 / 0, "invalid expression: should not show up?");
(The C11 standard is not clear to me, GCC outputs an "invalid expression"
message and ignore the assertion).

Finally, the standard also allow to place the assertion
inside a struct or union declaration; like:
	struct s {
		char c;
		_Static_assert(1, "inside struct");
	};
which is working fine.
And also, I think:
	struct s2 {
		char c;
		_Static_assert(sizeof(struct s) == 1, "own struct sizeof");
	};
which doesn't.


Yours,
Luc

Re: [PATCH] sparse: add support for static assert

[Lance Richardson]

----- Original Message -----
> On Wed, Jan 06, 2016 at 01:35:17PM -0500, Lance Richardson wrote:
> > This patch introduces support for _Static_assert() in global,
> > function, and struct/union declaration contexts (as currently supported
> > by gcc).
> > 
> > Tested via:
> >    - kernel build with C=1 CF=-D__CHECK_ENDIAN__
> >    - build/check large code base making heavy use of _Static_assert()
> >    - "make check" with added test case for static assert support
> 
> Nice, it was something that was indeed missing.
> 
> I have a few remarks, mostly nitpicking, here under.

Hi Luc, thanks for taking the time to review this.

> 
> 
> Luc
> 
> > @@ -437,6 +443,10 @@ static struct init_keyword {
> >  	{ "__restrict",	NS_TYPEDEF, .op = &restrict_op},
> >  	{ "__restrict__",	NS_TYPEDEF, .op = &restrict_op},
> >  
> > +
> > +	/* Static assertion */
> > +	{ "_Static_assert", NS_TYPEDEF, .op = &static_assert_op },
> > +
> 
> It seems a bit strange to me to use NS_TYPEDEF, as this is unrelated types.
> OTOH, the other namespaces deosn't seems better suited,
> and yes C11 define this as sort of declaration, so ...

Agreed.

> 
> > @@ -2004,6 +2014,27 @@ static struct token *parse_asm_declarator(struct
> > token *token, struct decl_state
> >  	return token;
> >  }
> >  
> > +
> > +static struct token *static_assert_specifier(struct token *token, struct
> > decl_state *ctx)
> > +{
> > +	struct expression *expr = NULL;
> > +	int val;
> > +
> > +	token = constant_expression(token->next, &expr);
> > +	if (!expr)
> > +		sparse_error(token->pos, "Expected constant expression");
> > +	val = get_expression_value(expr);
> > +	token = expect(token, ',', "after first argument of _Static_assert");
> > +	token = parse_expression(token, &expr);
> > +	token = expect(token, ')', "after second argument of _Static_assert");
> > +
> > +	if (!val)
> > +		sparse_error(token->pos, "static assertion failed: %s",
> > +                             show_string(expr->string));
> 
> By using token->pos here, the error message will indicate that the problem is
> situated at the very end of the assertion; more exactly, at the ending ";".
> This is a little annoying I find.
> It would be better to use the some position as the expression
> (expr->pos, or the same as for "Expected constant expression").

Will fix in v2.

> 
> I find also a bit strange to have this sort of semantic check inside the
> parser.

This prompted me to look at the gcc implementation, it appears that gcc also
processes _Static_assert() during parsing.

>  
> > diff --git a/validation/static_assert.c b/validation/static_assert.c
> > new file mode 100644
> > index 0000000..baab346
> > --- /dev/null
> > +++ b/validation/static_assert.c
> > @@ -0,0 +1,20 @@
> > +_Static_assert(1, "global ok");
> > +
> > +struct foo {
> > +	_Static_assert(1, "struct ok");
> > +};
> > +
> > +void bar(void)
> > +{
> > +	_Static_assert(1, " func ok");
> > +}
> > +
> > +_Static_assert(0, "expected failure");
> > +/*
> > + * check-name: static assertion
> > + *
> > + * check-error-start
> > +static_assert.c:12:38: error: static assertion failed: "expected failure"
> > + * check-error-end
> > + */
> 
> It would be nice to add a few more test cases,
> I'm thinbking to things like:
> 	static int f;
> 	_Static_assert(f, "non-constant expression");
> 	static int *p;
> 	_Static_assert(p, "non-integer expression");
> 	_Static_assert(0.1, "float expression");
> 	
> 	_Static_assert(!0 == 1, "non-trivial expression");
> 	
> 	static int array[4];
> 	_Static_assert(sizeof(array), "sizeof expression");
> 	
> 	static const char non_literal_string[] = "non literal string";
> 	_Static_assert(0, non_literal_string);
> 

Will do.

> Also, what should happen with:
> 	_Static_assert(1 / 0, "invalid expression: should not show up?");
> (The C11 standard is not clear to me, GCC outputs an "invalid expression"
> message and ignore the assertion).

v2 will output something like "invalid constant integer expression".

> 
> Finally, the standard also allow to place the assertion
> inside a struct or union declaration; like:
> 	struct s {
> 		char c;
> 		_Static_assert(1, "inside struct");
> 	};
> which is working fine.
> And also, I think:
> 	struct s2 {
> 		char c;
> 		_Static_assert(sizeof(struct s) == 1, "own struct sizeof");
> 	};
> which doesn't.

The second case should work (unless you intended "sizeof(struct s2)"), the v2
implementation will be a little different to address this issue.

> 
> 
> Yours,
> Luc
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Thanks again for the feedback, I'll send v2 of the patch in the next few days.

Re: [PATCH] sparse: add support for static assert

[Luc Van Oostenryck]

On Sun, Jan 10, 2016 at 09:54:24PM -0500, Lance Richardson wrote:
> ----- Original Message -----
> > On Wed, Jan 06, 2016 at 01:35:17PM -0500, Lance Richardson wrote:
> > > This patch introduces support for _Static_assert() in global,
> > > function, and struct/union declaration contexts (as currently supported
> > > by gcc).
> > > 
> > > Tested via:
> > >    - kernel build with C=1 CF=-D__CHECK_ENDIAN__
> > >    - build/check large code base making heavy use of _Static_assert()
> > >    - "make check" with added test case for static assert support
> > 
> > Nice, it was something that was indeed missing.
> > 
> > I have a few remarks, mostly nitpicking, here under.
> 
> Hi Luc, thanks for taking the time to review this.

Glad to help!
 
...

> > 
> > I find also a bit strange to have this sort of semantic check inside the
> > parser.
> 
> This prompted me to look at the gcc implementation, it appears that gcc also
> processes _Static_assert() during parsing.

It can always be moved elsewhere if needed.
 

> > Also, what should happen with:
> > 	_Static_assert(1 / 0, "invalid expression: should not show up?");
> > (The C11 standard is not clear to me, GCC outputs an "invalid expression"
> > message and ignore the assertion).
> 
> v2 will output something like "invalid constant integer expression".

Please don't.
That's the job of constant_expression() to emit a good error message.
Here, the best thing to do is to *not* emit an error message if the
expression is not a valid expression, constant (for example,
by forcing val to 1 when expr is null).

> > 
> > Finally, the standard also allow to place the assertion
> > inside a struct or union declaration; like:
> > 	struct s {
> > 		char c;
> > 		_Static_assert(1, "inside struct");
> > 	};
> > which is working fine.
> > And also, I think:
> > 	struct s2 {
> > 		char c;
> > 		_Static_assert(sizeof(struct s) == 1, "own struct sizeof");
> > 	};
> > which doesn't.
> 
> The second case should work (unless you intended "sizeof(struct s2)"), the v2
> implementation will be a little different to address this issue.

Yep, I messed it up.
I indeed meant "sizeof(struct s2)" and it seems to work fine
but for the wrong reason. See what I mean with the following example:
	struct s3 {
	        char c;
	        _Static_assert(sizeof(struct s3) == 1, "own struct sizeof, partial 1");
	        _Static_assert(sizeof(struct s3) == 2, "own struct sizeof, partial 2");
	        char d;
	};
but this problem is unrelated to your patch.

Yours,
Luc