Message ID | 20170503165518.7625-1-lrichard@redhat.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
On Wed, May 03, 2017 at 12:55:18PM -0400, Lance Richardson wrote: > This patch introduces support for the C11 _Static_assert() construct. Great! > diff --git a/parse.c b/parse.c ... > index b52c6ab..f1b96cc 100644 > --- a/parse.c > +++ b/parse.c > @@ -1864,13 +1872,21 @@ 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, '}')) { > - if (!match_op(token, ';')) > - token = declaration_list(token, list); > - if (!match_op(token, ';')) { > - sparse_error(token->pos, "expected ; at end of declaration"); > - break; > + struct symbol *keyword = NULL; > + > + if (token_type(token) == TOKEN_IDENT) > + keyword = lookup_keyword(token->ident, NS_KEYWORD); > + if (keyword && keyword->op == &static_assert_op) Is it possible to move this test in a helper? Something like static int match_static_assert(struct token *token) { struct symbol *keyword; if (token_type(token) != TOKEN_IDENT) return 0; keyword = lookup_keyword(token->ident, NS_KEYWORD); return keyword && keyword->op == &static_assert_op; } > @@ -2389,6 +2436,10 @@ static struct token * statement_list(struct token *token, struct statement_list > } > stmt = alloc_statement(token->pos, STMT_DECLARATION); > token = external_declaration(token, &stmt->declaration); > + } else if (token_type(token) == TOKEN_IDENT && > + (keyword = lookup_keyword(token->ident, NS_KEYWORD)) && > + keyword->op == &static_assert_op) { > + token = parse_static_assert(token, NULL); The helper can be reused here too. But then I saw that code like: extern int _Static_assert; didn't gave a warning because '_Static_assert' is not reserved keyword as it should. And then I wonder how this can be simplified. For the test cases, it would also be good to add a small test using the assert not on the top of a block (thus not where declarations normally are). -- 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 Thu, May 4, 2017 at 1:54 AM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > On Wed, May 03, 2017 at 12:55:18PM -0400, Lance Richardson wrote: >> This patch introduces support for the C11 _Static_assert() construct. > > Great! > >> diff --git a/parse.c b/parse.c > ... >> index b52c6ab..f1b96cc 100644 >> --- a/parse.c >> +++ b/parse.c >> @@ -1864,13 +1872,21 @@ 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, '}')) { >> - if (!match_op(token, ';')) >> - token = declaration_list(token, list); >> - if (!match_op(token, ';')) { >> - sparse_error(token->pos, "expected ; at end of declaration"); >> - break; >> + struct symbol *keyword = NULL; >> + >> + if (token_type(token) == TOKEN_IDENT) >> + keyword = lookup_keyword(token->ident, NS_KEYWORD); >> + if (keyword && keyword->op == &static_assert_op) > > Is it possible to move this test in a helper? Something like > static int match_static_assert(struct token *token) > { > struct symbol *keyword; > if (token_type(token) != TOKEN_IDENT) > return 0; > keyword = lookup_keyword(token->ident, NS_KEYWORD); > return keyword && keyword->op == &static_assert_op; > } > >> @@ -2389,6 +2436,10 @@ static struct token * statement_list(struct token *token, struct statement_list >> } >> stmt = alloc_statement(token->pos, STMT_DECLARATION); >> token = external_declaration(token, &stmt->declaration); >> + } else if (token_type(token) == TOKEN_IDENT && >> + (keyword = lookup_keyword(token->ident, NS_KEYWORD)) && >> + keyword->op == &static_assert_op) { >> + token = parse_static_assert(token, NULL); There is another problem here. With a few more line of context, we have: >> @@ -2389,6 +2436,10 @@ static struct token * statement_list(struct token *token, struct statement_list >> } >> stmt = alloc_statement(token->pos, STMT_DECLARATION); >> token = external_declaration(token, &stmt->declaration); >> + } else if (token_type(token) == TOKEN_IDENT && >> + (keyword = lookup_keyword(token->ident, NS_KEYWORD)) && >> + keyword->op == &static_assert_op) { >> + token = parse_static_assert(token, NULL); >> } >> add_statement(list, stmt); but when the static assert matches 'stmt' is not initialized and still added to the statement list (with sporadic crashes). -- 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 Wed, May 3, 2017 at 12:55 PM, Lance Richardson <lrichard@redhat.com> wrote: > + } else if (token_type(token) == TOKEN_IDENT && > + (keyword = lookup_keyword(token->ident, NS_KEYWORD)) && > + keyword->op == &static_assert_op) { I think you should add _Static_assert into header file "ident-list.h" then you can simplify this test as: else if (token_type(token) == TOKEN_IDENT && token->ident == &_Static_assert_ident) We don't need to go through a keyword look up then test the keyword->op == &static_assert_op. 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: "Luc Van Oostenryck" <luc.vanoostenryck@gmail.com> > To: "Lance Richardson" <lrichard@redhat.com> > Cc: "Linux-Sparse" <linux-sparse@vger.kernel.org> > Sent: Wednesday, 3 May, 2017 10:29:27 PM > Subject: Re: [PATCH v4] sparse: add support for _Static_assert > > On Thu, May 4, 2017 at 1:54 AM, Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: > > On Wed, May 03, 2017 at 12:55:18PM -0400, Lance Richardson wrote: > >> This patch introduces support for the C11 _Static_assert() construct. > > > > Great! > > > >> diff --git a/parse.c b/parse.c > > ... > >> index b52c6ab..f1b96cc 100644 > >> --- a/parse.c > >> +++ b/parse.c > >> @@ -1864,13 +1872,21 @@ 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, '}')) { > >> - if (!match_op(token, ';')) > >> - token = declaration_list(token, list); > >> - if (!match_op(token, ';')) { > >> - sparse_error(token->pos, "expected ; at end of > >> declaration"); > >> - break; > >> + struct symbol *keyword = NULL; > >> + > >> + if (token_type(token) == TOKEN_IDENT) > >> + keyword = lookup_keyword(token->ident, NS_KEYWORD); > >> + if (keyword && keyword->op == &static_assert_op) > > > > Is it possible to move this test in a helper? Something like > > static int match_static_assert(struct token *token) > > { > > struct symbol *keyword; > > if (token_type(token) != TOKEN_IDENT) > > return 0; > > keyword = lookup_keyword(token->ident, NS_KEYWORD); > > return keyword && keyword->op == &static_assert_op; > > } > > > >> @@ -2389,6 +2436,10 @@ static struct token * statement_list(struct token > >> *token, struct statement_list > >> } > >> stmt = alloc_statement(token->pos, > >> STMT_DECLARATION); > >> token = external_declaration(token, > >> &stmt->declaration); > >> + } else if (token_type(token) == TOKEN_IDENT && > >> + (keyword = lookup_keyword(token->ident, > >> NS_KEYWORD)) && > >> + keyword->op == &static_assert_op) { > >> + token = parse_static_assert(token, NULL); > > There is another problem here. With a few more line of context, we have: > >> @@ -2389,6 +2436,10 @@ static struct token * statement_list(struct token > >> *token, struct statement_list > >> } > >> stmt = alloc_statement(token->pos, > >> STMT_DECLARATION); > >> token = external_declaration(token, > >> &stmt->declaration); > >> + } else if (token_type(token) == TOKEN_IDENT && > >> + (keyword = lookup_keyword(token->ident, > >> NS_KEYWORD)) && > >> + keyword->op == &static_assert_op) { > >> + token = parse_static_assert(token, NULL); > >> } > >> add_statement(list, stmt); > > but when the static assert matches 'stmt' is not initialized and still added > to the statement list (with sporadic crashes). > Oof, I recall noticing that but apparently forgot to do anything about it. Thanks for the quick feedback, will incorporate in v5. > -- 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
> From: "Christopher Li" <sparse@chrisli.org> > To: "Lance Richardson" <lrichard@redhat.com> > Cc: "Linux-Sparse" <linux-sparse@vger.kernel.org> > Sent: Wednesday, 3 May, 2017 11:03:22 PM > Subject: Re: [PATCH v4] sparse: add support for _Static_assert > > On Wed, May 3, 2017 at 12:55 PM, Lance Richardson <lrichard@redhat.com> > wrote: > > + } else if (token_type(token) == TOKEN_IDENT && > > + (keyword = lookup_keyword(token->ident, > > NS_KEYWORD)) && > > + keyword->op == &static_assert_op) { > > I think you should add _Static_assert into header file "ident-list.h" > then you can simplify this test as: > > else if (token_type(token) == TOKEN_IDENT && > token->ident == &_Static_assert_ident) > > We don't need to go through a keyword look up then test > the keyword->op == &static_assert_op. > > Chris > Makes sense, thanks for the suggestion. This should also address Luc's comment about "extern int _Static_assert;" being accepted in the v4 version. Will fold into v5. Thanks, 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 Thu, May 04, 2017 at 09:53:50AM -0400, Lance Richardson wrote:
> Thanks for the quick feedback, will incorporate in v5.
I also saw a small issue, possibly related to the problem with
the mixup between 'declaration-list' and 'struct-declarator-list'
that you noticed. In the following code, the static assert is not
recognized:
void foo(void)
{
int i = 0;
for (_Static_assert(1, "ok"); 1; )
;
for (_Static_assert(0, "ko"); 1; )
;
}
It should because (since C99) the first part of the for-statement
is just a 'declaration', which include the static assert.
Not that it's very important, though.
Probably, it's best to leave it as is for the moment and just
add a new test case, annotated with 'check-known-to-fail'.
-- 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
> From: "Luc Van Oostenryck" <luc.vanoostenryck@gmail.com> > To: "Lance Richardson" <lrichard@redhat.com> > Cc: "Linux-Sparse" <linux-sparse@vger.kernel.org> > Sent: Thursday, 4 May, 2017 10:58:35 AM > Subject: Re: [PATCH v4] sparse: add support for _Static_assert > > On Thu, May 04, 2017 at 09:53:50AM -0400, Lance Richardson wrote: > > Thanks for the quick feedback, will incorporate in v5. > > I also saw a small issue, possibly related to the problem with > the mixup between 'declaration-list' and 'struct-declarator-list' > that you noticed. In the following code, the static assert is not > recognized: > void foo(void) > { > int i = 0; > for (_Static_assert(1, "ok"); 1; ) > ; > for (_Static_assert(0, "ko"); 1; ) > ; > } > > It should because (since C99) the first part of the for-statement > is just a 'declaration', which include the static assert. > Not that it's very important, though. > > Probably, it's best to leave it as is for the moment and just > add a new test case, annotated with 'check-known-to-fail'. > > -- Luc > Hi Luc, I'm not sure it should be accepted; the standard doc says: "The declaration part of a for statement shall only declare identifiers for objects having storage class auto or register." But _Static_assert() doesn't declare an identifier. gcc accepts this syntax, although clang does not. I did find this discussion: https://reviews.llvm.org/D9113 Which points to a working group discussion that made it sound as though _Static_assert() should not be accepted in for-loop declarations: www.open-std.org/JTC1/SC22/WG14/13677 As you say, probably best to leave this for now. 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 Thu, May 4, 2017 at 5:46 PM, Lance Richardson <lrichard@redhat.com> wrote: >> From: "Luc Van Oostenryck" <luc.vanoostenryck@gmail.com> >> To: "Lance Richardson" <lrichard@redhat.com> >> Cc: "Linux-Sparse" <linux-sparse@vger.kernel.org> >> Sent: Thursday, 4 May, 2017 10:58:35 AM >> Subject: Re: [PATCH v4] sparse: add support for _Static_assert >> >> On Thu, May 04, 2017 at 09:53:50AM -0400, Lance Richardson wrote: >> > Thanks for the quick feedback, will incorporate in v5. >> >> I also saw a small issue, possibly related to the problem with >> the mixup between 'declaration-list' and 'struct-declarator-list' >> that you noticed. In the following code, the static assert is not >> recognized: >> void foo(void) >> { >> int i = 0; >> for (_Static_assert(1, "ok"); 1; ) >> ; >> for (_Static_assert(0, "ko"); 1; ) >> ; >> } >> >> It should because (since C99) the first part of the for-statement >> is just a 'declaration', which include the static assert. >> Not that it's very important, though. >> >> Probably, it's best to leave it as is for the moment and just >> add a new test case, annotated with 'check-known-to-fail'. >> >> -- Luc >> > > Hi Luc, > > I'm not sure it should be accepted; the standard doc says: > > "The declaration part of a for statement shall only declare > identifiers for objects having storage class auto or register." > > But _Static_assert() doesn't declare an identifier. Indeed. > gcc accepts this syntax, although clang does not. I did find this discussion: > > https://reviews.llvm.org/D9113 > > Which points to a working group discussion that made it sound as > though _Static_assert() should not be accepted in for-loop declarations: It makes a lot of sense to not accept it. > www.open-std.org/JTC1/SC22/WG14/13677 > > As you say, probably best to leave this for now. Yes, even more so now. Thanks for the info. -- 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
diff --git a/parse.c b/parse.c index b52c6ab..f1b96cc 100644 --- a/parse.c +++ b/parse.c @@ -73,6 +73,7 @@ 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(struct token *token, struct symbol_list **unused); typedef struct token *attr_t(struct token *, struct symbol *, struct decl_state *); @@ -308,6 +309,10 @@ static struct symbol_op asm_op = { .toplevel = toplevel_asm_declaration, }; +static struct symbol_op static_assert_op = { + .toplevel = parse_static_assert, +}; + static struct symbol_op packed_op = { .attribute = attribute_packed, }; @@ -437,6 +442,9 @@ 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 }, @@ -1864,13 +1872,21 @@ 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, '}')) { - if (!match_op(token, ';')) - token = declaration_list(token, list); - if (!match_op(token, ';')) { - sparse_error(token->pos, "expected ; at end of declaration"); - break; + struct symbol *keyword = NULL; + + if (token_type(token) == TOKEN_IDENT) + keyword = lookup_keyword(token->ident, NS_KEYWORD); + if (keyword && keyword->op == &static_assert_op) + token = parse_static_assert(token, NULL); + else { + if (!match_op(token, ';')) + token = declaration_list(token, list); + if (!match_op(token, ';')) { + sparse_error(token->pos, "expected ; at end of declaration"); + break; + } + token = token->next; } - token = token->next; } return token; } @@ -2012,6 +2028,36 @@ static struct token *parse_asm_declarator(struct token *token, struct decl_state return token; } + +static struct token *parse_static_assert(struct token *token, struct symbol_list **unused) +{ + struct expression *expr1 = NULL, *expr2 = NULL; + int val; + + token = expect(token->next, '(', "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"); + + token = expect(token, ';', "after _Static_assert()"); + + if (!expr1 || !expr2) + return token; + + 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; +} + /* Make a statement out of an expression */ static struct statement *make_statement(struct expression *expr) { @@ -2378,6 +2424,7 @@ static struct token * statement_list(struct token *token, struct statement_list token = label_statement(token->next); for (;;) { struct statement * stmt; + struct symbol *keyword; if (eof_token(token)) break; if (match_op(token, '}')) @@ -2389,6 +2436,10 @@ static struct token * statement_list(struct token *token, struct statement_list } stmt = alloc_statement(token->pos, STMT_DECLARATION); token = external_declaration(token, &stmt->declaration); + } else if (token_type(token) == TOKEN_IDENT && + (keyword = lookup_keyword(token->ident, NS_KEYWORD)) && + keyword->op == &static_assert_op) { + token = parse_static_assert(token, NULL); } else { seen_statement = Wdeclarationafterstatement; token = statement(token, &stmt); @@ -2726,7 +2777,7 @@ struct token *external_declaration(struct token *token, struct symbol_list **lis unsigned long mod; int is_typedef; - /* Top-level inline asm? */ + /* Top-level inline asm or static assertion? */ if (token_type(token) == TOKEN_IDENT) { struct symbol *s = lookup_keyword(token->ident, NS_KEYWORD); if (s && s->op->toplevel) 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 the C11 _Static_assert() construct. Per the N1539 draft standard, the syntax changes for this construct include: declaration: <declaration-specifiers> <init-declarator-list>[opt] ; <static_assert-declaration> struct-declaration: <specifier-qualifier-list> <struct-declarator-list>[opt] ; <static_assert-declaration> static_assert-declaration: _Static_assert ( <constant-expression> , <string-literal> ) ; Signed-off-by: Lance Richardson <lrichard@redhat.com> --- v4: Addressed feedback, simplified and restructured to better model description in draft standard. v3: - Removed bogus test case introduced in v2 (static assertion on sizeof a structure within the definition of the structure). 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). parse.c | 65 +++++++++++++++++++++++++++++++++++++++++----- validation/static_assert.c | 57 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 7 deletions(-) create mode 100644 validation/static_assert.c