[PATCH v3] sparse: add support for static assert

[PATCH v3] 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: 
    - Added additional test cases.
    - Added additional validation for parameters to _Static_assert().
    - Reworked implementation to avoid impacting struct/union definition
      handling ( the v1 implementation, which treated _Static_assert()
      as an NS_TYPEDEF term, had the unfortunate side-effect of
      leaving an unnamed field with unknown size attached to structure
      definitions when a static assert was inside a structure definition).

v3:
    - Removed bogus test case introduced in v2 (static assertion on sizeof
      a structure within the definition of the structure).
  

 parse.c                    | 65 +++++++++++++++++++++++++++++++++++++++++++++-
 symbol.h                   |  1 +
 validation/static_assert.c | 57 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 122 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..a1ef68f
--- /dev/null
+++ b/validation/static_assert.c
@@ -0,0 +1,57 @@
+_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");
+};
+
+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 v3] sparse: add support for static assert

[Luc Van Oostenryck]

On Mon, Feb 08, 2016 at 10:37:10PM -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 cases for static assert support
> 
> Signed-off-by: Lance Richardson <lrichard@redhat.com>


Fine for me.

Feel free to add
  Reviewed-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>


Chris, I have already reviwed this in January and Richard have taken
my remarks in account.
Do you think you you will have some time soon to look at it?


Regards,
Luc

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

[Lance Richardson]

----- Original Message -----
> From: "Luc Van Oostenryck" <luc.vanoostenryck@gmail.com>
> To: "Lance Richardson" <lrichard@redhat.com>
> Cc: linux-sparse@vger.kernel.org
> Sent: Tuesday, March 15, 2016 6:52:03 PM
> Subject: Re: [PATCH v3] sparse: add support for static assert
> 
> On Mon, Feb 08, 2016 at 10:37:10PM -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 cases for static assert support
> > 
> > Signed-off-by: Lance Richardson <lrichard@redhat.com>
> 
> 
> Fine for me.
> 
> Feel free to add
>   Reviewed-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> 
> 
> Chris, I have already reviwed this in January and Richard have taken
> my remarks in account.
> Do you think you you will have some time soon to look at it?
> 
> 
> Regards,
> 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
> 

Dusting this one off... "ping"?

Thanks and Regards,

   Lance

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

[Luc Van Oostenryck]

On Tue, Jun 14, 2016 at 10:49:12AM -0400, Lance Richardson wrote:
> ----- Original Message -----
> > From: "Luc Van Oostenryck" <luc.vanoostenryck@gmail.com>
> > To: "Lance Richardson" <lrichard@redhat.com>
> > Cc: linux-sparse@vger.kernel.org
> > Sent: Tuesday, March 15, 2016 6:52:03 PM
> > Subject: Re: [PATCH v3] sparse: add support for static assert
> > 
> > On Mon, Feb 08, 2016 at 10:37:10PM -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 cases for static assert support
> > > 
> > > Signed-off-by: Lance Richardson <lrichard@redhat.com>
> > 
> > 
> > Fine for me.
> > 
> > Feel free to add
> >   Reviewed-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> > 
> > 
> > Chris, I have already reviwed this in January and Richard have taken
> > my remarks in account.
> > Do you think you you will have some time soon to look at it?
> > 
> > 
> > Regards,
> > Luc
> > 
> 
> Dusting this one off... "ping"?
> 
> Thanks and Regards,
> 
>    Lance

Chris, is is possible to have some feedback on this serie, please?

Is there anything more I can do to help?


Best regards,
Luc

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

[Christopher Li]

On Sat, Oct 29, 2016 at 4:38 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> Chris, is is possible to have some feedback on this serie, please?
>
I did go though your V3 static assert patch. It has some minor issue.

Over all pretty good.

> Is there anything more I can do to help?

Let me send you the feed back first.

Chris

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

[Luc Van Oostenryck]

On Sun, Oct 30, 2016 at 08:04:34AM +0800, Christopher Li wrote:
> On Sat, Oct 29, 2016 at 4:38 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> > Chris, is is possible to have some feedback on this serie, please?
> >
> I did go though your V3 static assert patch.
Thanks.

> It has some minor issue.
> 
> Over all pretty good.
> 
> > Is there anything more I can do to help?
> 
> Let me send you the feed back first.
Sure.

> Chris
> --

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

[Christopher Li]

>+static struct symbol_op static_assert_op = {
>+       .type = KW_ST_ASSERT,
>+       .declarator = static_assert_declarator,
>+       .statement = parse_static_assert_statement,
>+       .toplevel = toplevel_static_assert,
>+};
>+

There is no need to introduce the KW_ST_ASSERT. Will explain
it later.

static assert is not a declarator. It will introduce other
issue if you treat it as one. For example, it allow the
following illegal syntax: "_Static_assert(1, "") foo;"

I think with ".statement" and ".toplevel" call back, it is sufficient
to remove the ".declarator". Do I break any thing if I remove it?


>+
>+               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);
>+               }

There is much simpler way to do it. Please take a look at id-list.h.

if (token_type(token) == TOKEN_IDENT && token->ident == &_Static_assert_ident)
...

That is why there is no need for the KW_ST_ASSERT.

>+static struct token *parse_static_assert(struct token *token, int expect_semi)

I don't see which syntax can allow _Static_assert() not following the
semi column.
Do you have an example?

I can see const_expression_value will expand the expression as needed.
I am still wondering if there is any thing else need to process at
a later stage like expanding.

The test example is pretty good. We can use a little bit more test on
the path that trigger the assert.

Thanks for the patch and sorry for the late replay.

Chris

On Sun, Oct 30, 2016 at 2:20 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> On Sun, Oct 30, 2016 at 08:04:34AM +0800, Christopher Li wrote:
>> On Sat, Oct 29, 2016 at 4:38 AM, Luc Van Oostenryck
>> <luc.vanoostenryck@gmail.com> wrote:
>> > Chris, is is possible to have some feedback on this serie, please?
>> >
>> I did go though your V3 static assert patch.
> Thanks.
>
>> It has some minor issue.
>>
>> Over all pretty good.
>>
>> > Is there anything more I can do to help?
>>
>> Let me send you the feed back first.
> Sure.
>
>> Chris
>> --

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

[Lance Richardson]

> From: "Christopher Li" <sparse@chrisli.org>
> To: "Luc Van Oostenryck" <luc.vanoostenryck@gmail.com>
> Cc: "Lance Richardson" <lrichard@redhat.com>, "Linux-Sparse" <linux-sparse@vger.kernel.org>
> Sent: Sunday, October 30, 2016 12:43:32 PM
> Subject: Re: [PATCH v3] sparse: add support for static assert
> 
> >+static struct symbol_op static_assert_op = {
> >+       .type = KW_ST_ASSERT,
> >+       .declarator = static_assert_declarator,
> >+       .statement = parse_static_assert_statement,
> >+       .toplevel = toplevel_static_assert,
> >+};
> >+
> 
> There is no need to introduce the KW_ST_ASSERT. Will explain
> it later.
> 
> static assert is not a declarator. It will introduce other
> issue if you treat it as one. For example, it allow the
> following illegal syntax: "_Static_assert(1, "") foo;"
> 
> I think with ".statement" and ".toplevel" call back, it is sufficient
> to remove the ".declarator". Do I break any thing if I remove it?
> 
> 
> >+
> >+               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);
> >+               }
> 
> There is much simpler way to do it. Please take a look at id-list.h.
> 
> if (token_type(token) == TOKEN_IDENT && token->ident ==
> &_Static_assert_ident)
> ...
> 
> That is why there is no need for the KW_ST_ASSERT.
> 
> >+static struct token *parse_static_assert(struct token *token, int
> >expect_semi)
> 
> I don't see which syntax can allow _Static_assert() not following the
> semi column.
> Do you have an example?
> 
> I can see const_expression_value will expand the expression as needed.
> I am still wondering if there is any thing else need to process at
> a later stage like expanding.
> 
> The test example is pretty good. We can use a little bit more test on
> the path that trigger the assert.
> 
> Thanks for the patch and sorry for the late replay.
> 
> Chris
> 

Thanks for reviewing, Chris, I will work on a v4 to incorporate your feedback.

   Lance