Message ID | 1454989031-12240-1-git-send-email-lrichard@redhat.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
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
----- 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
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
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
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
>+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
> 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 --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 = ®ister_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 + */ +
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