[PATCH v2] sparse: add support for static assert

[PATCH v2] 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 cases for static assert support

Signed-off-by: Lance Richardson <lrichard@redhat.com>
---
v2: add additional test cases
    add additional validation for parameters to _Static_assert()
    rework implementation to avoid impacting struct/union definition handling

 parse.c                    | 65 +++++++++++++++++++++++++++++++++++++++++++++-
 symbol.h                   |  1 +
 validation/static_assert.c | 62 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 127 insertions(+), 1 deletion(-)
 create mode 100644 validation/static_assert.c

diff --git a/parse.c b/parse.c
index b43d683..866c99d 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_declarator;
 
 static struct token *parse_if_statement(struct token *token, struct statement *stmt);
 static struct token *parse_return_statement(struct token *token, struct statement *stmt);
@@ -73,6 +74,8 @@ static struct token *parse_context_statement(struct token *token, struct stateme
 static struct token *parse_range_statement(struct token *token, struct statement *stmt);
 static struct token *parse_asm_statement(struct token *token, struct statement *stmt);
 static struct token *toplevel_asm_declaration(struct token *token, struct symbol_list **list);
+static struct token *parse_static_assert_statement(struct token *token, struct statement *stmt);
+static struct token *toplevel_static_assert(struct token *token, struct symbol_list **list);
 
 typedef struct token *attr_t(struct token *, struct symbol *,
 			     struct decl_state *);
@@ -308,6 +311,13 @@ static struct symbol_op asm_op = {
 	.toplevel = toplevel_asm_declaration,
 };
 
+static struct symbol_op static_assert_op = {
+	.type = KW_ST_ASSERT,
+	.declarator = static_assert_declarator,
+	.statement = parse_static_assert_statement,
+	.toplevel = toplevel_static_assert,
+};
+
 static struct symbol_op packed_op = {
 	.attribute = attribute_packed,
 };
@@ -437,6 +447,10 @@ static struct init_keyword {
 	{ "__restrict",	NS_TYPEDEF, .op = &restrict_op},
 	{ "__restrict__",	NS_TYPEDEF, .op = &restrict_op},
 
+
+	/* Static assertion */
+	{ "_Static_assert", NS_KEYWORD, .op = &static_assert_op },
+
 	/* Storage class */
 	{ "auto",	NS_TYPEDEF, .op = &auto_op },
 	{ "register",	NS_TYPEDEF, .op = &register_op },
@@ -1856,6 +1870,13 @@ static struct token *declaration_list(struct token *token, struct symbol_list **
 static struct token *struct_declaration_list(struct token *token, struct symbol_list **list)
 {
 	while (!match_op(token, '}')) {
+		struct symbol *keyword;
+
+		if (token_type(token) == TOKEN_IDENT) {
+			keyword = lookup_keyword(token->ident, NS_KEYWORD);
+			if (keyword && keyword->op->type == KW_ST_ASSERT)
+				token = keyword->op->declarator(token, NULL);
+		}
 		if (!match_op(token, ';'))
 			token = declaration_list(token, list);
 		if (!match_op(token, ';')) {
@@ -2004,6 +2025,48 @@ static struct token *parse_asm_declarator(struct token *token, struct decl_state
 	return token;
 }
 
+
+static struct token *parse_static_assert(struct token *token, int expect_semi)
+{
+	struct expression *expr1 = NULL, *expr2 = NULL;
+	int val;
+
+	token = expect(token, '(', "after _Static_assert");
+	token = constant_expression(token, &expr1);
+	token = expect(token, ',', "after first argument of _Static_assert");
+	token = parse_expression(token, &expr2);
+	token = expect(token, ')', "after second argument of _Static_assert");
+
+	if (expect_semi)
+		token = expect(token, ';', "after _Static_assert()");
+
+	val = const_expression_value(expr1);
+
+	if (expr2->type != EXPR_STRING)
+		sparse_error(expr2->pos, "bad string literal");
+	else if (expr1 && (expr1->type == EXPR_VALUE)) {
+		if (!val)
+			sparse_error(expr1->pos, "static assertion failed: %s",
+				     show_string(expr2->string));
+	}
+
+	return token;
+}
+
+static struct token *static_assert_declarator(struct token *token, struct decl_state *ctx)
+{
+	return parse_static_assert(token->next, 0);
+}
+
+static struct token *parse_static_assert_statement(struct token *token, struct statement *stmt)
+{
+	return parse_static_assert(token->next, 1);
+}
+static struct token *toplevel_static_assert(struct token *token, struct symbol_list **list)
+{
+	return parse_static_assert(token->next, 1);
+}
+
 /* Make a statement out of an expression */
 static struct statement *make_statement(struct expression *expr)
 {
diff --git a/symbol.h b/symbol.h
index ccb5dcb..2822b0a 100644
--- a/symbol.h
+++ b/symbol.h
@@ -86,6 +86,7 @@ enum keyword {
 	KW_SHORT	= 1 << 7,
 	KW_LONG		= 1 << 8,
 	KW_EXACT	= 1 << 9,
+	KW_ST_ASSERT	= 1 << 10,
 };
 
 struct context {
diff --git a/validation/static_assert.c b/validation/static_assert.c
new file mode 100644
index 0000000..d3da954
--- /dev/null
+++ b/validation/static_assert.c
@@ -0,0 +1,62 @@
+_Static_assert(1, "global ok");
+
+struct foo {
+	_Static_assert(1, "struct ok");
+};
+
+void bar(void)
+{
+	_Static_assert(1, " func ok");
+}
+
+_Static_assert(0, "expected assertion failure");
+
+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 char array[4];
+_Static_assert(sizeof(array) == 4, "sizeof expression");
+        
+static const char non_literal_string[] = "non literal string";
+_Static_assert(0, non_literal_string);
+
+_Static_assert(1 / 0, "invalid expression: should not show up?");
+
+struct s {
+	char arr[16];
+	_Static_assert(1, "inside struct");
+};
+
+struct s2 {
+	char c;
+	_Static_assert(sizeof(struct s2) == 1, "struct sizeof");
+};
+
+union u {
+	char c;
+	int  i;
+	_Static_assert(1, "inside union");
+};
+
+_Static_assert(sizeof(struct s) == 16, "sizeof assertion");
+
+/*
+ * check-name: static assertion
+ *
+ * check-error-start
+static_assert.c:12:16: error: static assertion failed: "expected assertion failure"
+static_assert.c:15:16: error: bad constant expression
+static_assert.c:18:16: error: bad constant expression
+static_assert.c:20:16: error: bad constant expression
+static_assert.c:28:19: error: bad string literal
+static_assert.c:30:18: error: bad constant expression
+ * check-error-end
+ */
+
-- 
1.9.1

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

[Luc Van Oostenryck]

On Mon, Jan 11, 2016 at 05:31:22PM -0500, Lance Richardson wrote:


Hi,

I don't understand why tha parsing part have changed so much since v1.
Is it because I said
   > 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 ...
or because something related to handling it inside structs and unions or
for some other reason?

If because of the NS_TYPEDEF thing, sorry if I wasn't clear but I really think
it was fine, just that at first sight I found it strange.
If because the structs & unions, please explain why is it needed, what was wrong
with v1 and is fine now.


> diff --git a/validation/static_assert.c b/validation/static_assert.c
> new file mode 100644
> index 0000000..d3da954
> --- /dev/null
> +++ b/validation/static_assert.c
...
> +struct s2 {
> +	char c;
> +	_Static_assert(sizeof(struct s2) == 1, "struct sizeof");
> +};

This succeed but
	struct s2 {
		char c;
		_Static_assert(sizeof(struct s2) == 1, "struct sizeof");
		char d;
		_Static_assert(sizeof(struct s2) == 2, "struct sizeof");
	};
succeed also wich seems certainly very odd.

However it's not a problem with your patch but because of:
1) sparse is fine with the evaluation of sizeof(struct ...) while the struct
   is not yet completed (which is maybe usefull but certainly can also be
   considered as a bug)
2) those assertions are evaluated at parse time and not at some later time.
   My first thought was that we really should move the checking of those
   assertions at a later time, maybe after linearization and by introducing
   a new operation for it (like OP_ASSERT or so).
   But this is not a solution for the assertions inside structs & unions.
   I'll add a separate test case showing the problem and it's probably better
   to not put this test in your test cases.

Regards,
Luc

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

[Lance Richardson]

----- Original Message -----
> On Mon, Jan 11, 2016 at 05:31:22PM -0500, Lance Richardson wrote:
> 
> 
> Hi,
> 
> I don't understand why tha parsing part have changed so much since v1.
> Is it because I said
>    > 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 ...
> or because something related to handling it inside structs and unions or
> for some other reason?
> 
> If because of the NS_TYPEDEF thing, sorry if I wasn't clear but I really
> think
> it was fine, just that at first sight I found it strange.
> If because the structs & unions, please explain why is it needed, what was
> wrong
> with v1 and is fine now.

I discovered as I was adding additional test cases that the NS_TYPEDEF
approach was causing sizeof to report a zero size for structures with
embedded _Static_assert(); as part of processing NS_TYPEDEF within
a structure for _Static_assert(), a unnamed field with unknown size
was being attached to the structure definition.

So I decided to take a different approach, one that hopefully makes
more sense than handling _Static_assert() via NS_TYPEDEF. 

Apologies for not providing these details in the v2 commit log. 
> 
> 
> > diff --git a/validation/static_assert.c b/validation/static_assert.c
> > new file mode 100644
> > index 0000000..d3da954
> > --- /dev/null
> > +++ b/validation/static_assert.c
> ...
> > +struct s2 {
> > +	char c;
> > +	_Static_assert(sizeof(struct s2) == 1, "struct sizeof");
> > +};
> 
> This succeed but
> 	struct s2 {
> 		char c;
> 		_Static_assert(sizeof(struct s2) == 1, "struct sizeof");
> 		char d;
> 		_Static_assert(sizeof(struct s2) == 2, "struct sizeof");
> 	};
> succeed also wich seems certainly very odd.
> 

Yes, I believe they should both fail with something like "invalid use of
sizeof on incomplete type".

> However it's not a problem with your patch but because of:
> 1) sparse is fine with the evaluation of sizeof(struct ...) while the struct
>    is not yet completed (which is maybe usefull but certainly can also be
>    considered as a bug)

I think it's a bug.

> 2) those assertions are evaluated at parse time and not at some later time.
>    My first thought was that we really should move the checking of those
>    assertions at a later time, maybe after linearization and by introducing
>    a new operation for it (like OP_ASSERT or so).
>    But this is not a solution for the assertions inside structs & unions.
>    I'll add a separate test case showing the problem and it's probably better
>    to not put this test in your test cases.
> 

OK, I'll post a v3 with the invalid test case removed. Thanks for looking
at this.

   Lance

> Regards,
> Luc
> 

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

[Luc Van Oostenryck]

On Thu, Jan 28, 2016 at 09:53:29AM -0500, Lance Richardson wrote:
> ----- Original Message -----
> > On Mon, Jan 11, 2016 at 05:31:22PM -0500, Lance Richardson wrote:
> > 
> > 
> > Hi,
> > 
> > I don't understand why tha parsing part have changed so much since v1.
> 
> I discovered as I was adding additional test cases that the NS_TYPEDEF
> approach was causing sizeof to report a zero size for structures with
> embedded _Static_assert(); as part of processing NS_TYPEDEF within
> a structure for _Static_assert(), a unnamed field with unknown size
> was being attached to the structure definition.
> 
> So I decided to take a different approach, one that hopefully makes
> more sense than handling _Static_assert() via NS_TYPEDEF. 
> 
> Apologies for not providing these details in the v2 commit log. 

OK, I understand.
Yes, it's certainly worth to add that in the patch description.

> > > +struct s2 {
> > > +	char c;
> > > +	_Static_assert(sizeof(struct s2) == 1, "struct sizeof");
> > > +};
> > 
> > This succeed but
> > 	struct s2 {
> > 		char c;
> > 		_Static_assert(sizeof(struct s2) == 1, "struct sizeof");
> > 		char d;
> > 		_Static_assert(sizeof(struct s2) == 2, "struct sizeof");
> > 	};
> > succeed also wich seems certainly very odd.
> > 
> 
> Yes, I believe they should both fail with something like "invalid use of
> sizeof on incomplete type".

> 
> I think it's a bug.
> 

Absolutely.
 
> 
> OK, I'll post a v3 with the invalid test case removed. Thanks for looking
> at this.
> 
>    Lance

Good.
Please also add the explanation about the parsing in the patch description.

Luc