diff mbox

[v3] sparse: add support for static assert

Message ID 1454989031-12240-1-git-send-email-lrichard@redhat.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Lance Richardson Feb. 9, 2016, 3:37 a.m. UTC
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

Comments

Luc Van Oostenryck March 15, 2016, 10:52 p.m. UTC | #1
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
Lance Richardson June 14, 2016, 2:49 p.m. UTC | #2
----- 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
--
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
Luc Van Oostenryck Oct. 28, 2016, 8:38 p.m. UTC | #3
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
--
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
Christopher Li Oct. 30, 2016, 12:04 a.m. UTC | #4
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
--
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
Luc Van Oostenryck Oct. 30, 2016, 9:20 a.m. UTC | #5
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
> --
--
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
Christopher Li Oct. 30, 2016, 4:43 p.m. UTC | #6
>+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
>> --
--
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
Lance Richardson Oct. 30, 2016, 6:25 p.m. UTC | #7
> 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
--
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
diff mbox

Patch

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
+ */
+