| Message ID | 20170508211543.20681-1-lrichard@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, May 08, 2017 at 05:15:43PM -0400, Lance Richardson wrote: > This patch introduces support for the C11 _Static_assert() construct. For me, it's fine. I just have a small remarks (see below). > @@ -1945,13 +1953,17 @@ 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; > + if (match_ident(token, &_Static_assert_ident)) > + token = parse_static_assert(token, NULL); I find it better with a 'continue' here > + else { so, this 'else' become unneeded and there is no more needs to move the previous content of the loop (which help a lot when reviewing patches or when digging in the history). -- 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@vger.kernel.org > Sent: Monday, 8 May, 2017 7:25:10 PM > Subject: Re: [PATCH v6] sparse: add support for _Static_assert > > On Mon, May 08, 2017 at 05:15:43PM -0400, Lance Richardson wrote: > > This patch introduces support for the C11 _Static_assert() construct. > > For me, it's fine. > I just have a small remarks (see below). > > > @@ -1945,13 +1953,17 @@ 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; > > + if (match_ident(token, &_Static_assert_ident)) > > + token = parse_static_assert(token, NULL); > > I find it better with a 'continue' here > > > + else { > > so, this 'else' become unneeded and there is no > more needs to move the previous content of the loop > (which help a lot when reviewing patches or when > digging in the history). > > -- Luc > That does seem better. I'll wait a bit for any further feedback from Chris and post a new spin. 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
Hi Lance, thanks for the patch. Looks very good. Have some very minor comment below: On Mon, May 8, 2017 at 2:15 PM, Lance Richardson <lrichard@redhat.com> wrote: > @@ -1945,13 +1953,17 @@ 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; > + if (match_ident(token, &_Static_assert_ident)) > + token = parse_static_assert(token, NULL); I agree with Luc that his better with a "continue" then the following "else" section does not need to changed. > @@ -2093,6 +2105,37 @@ 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 *cond = NULL, *message = NULL; > + struct token *errtok; > + int valid = 1; > + > + token = expect(token->next, '(', "after _Static_assert"); > + errtok = token; > + token = constant_expression(token, &cond); > + if (!cond) { I think errtok is not needed here. If there is an expression, the normal case. Then errtok will holding the start of the expression. errok->pos == cond->pos. Call it errtok is a bit miss leading. If cond == NULL, that means the expression is empty. Then we have errtok == token. Either way errtok is not very useful. > + sparse_error(errtok->pos, "Expected constant expression"); BTW, when the error happen here, "errtok" is actually the next token after the missing expression, normally the ','. So the error actually happen before that token. > + valid = 0; > + } > + token = expect(token, ',', "after conditional expression in _Static_assert"); > + errtok = token; > + token = parse_expression(token, &message); > + if (!message || message->type != EXPR_STRING) { > + sparse_error(errtok->pos, "bad or missing string literal"); > + valid = 0; > + } > + token = expect(token, ')', "after diagnostic message in _Static_assert"); > + > + token = expect(token, ';', "after _Static_assert()"); There is some white space mix with tab in this line. > + > + if (valid && !const_expression_value(cond) && cond->type == EXPR_VALUE) I am tempted to get rid of the "valid" variable. BTW, the "cond->type == EXPR_VALUE" should take place *before* the "!const_expression_value(cond)", other wise it will try to get const expression for non value expression, due to the evaluate order. Some thing like: if (cond && cond->type == EXPR_VALUE && !const_expression_value(cond)) > + sparse_error(cond->pos, "static assertion failed: %s", > + show_string(message->string)); Then there: message ? show_string(message->string) : ""); I think the assert failed without message string, we already report the error on empty string expression. Raising the assert here might be acceptable? 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: "Lance Richardson" <lrichard@redhat.com> > Cc: "Linux-Sparse" <linux-sparse@vger.kernel.org> > Sent: Tuesday, 9 May, 2017 11:53:43 AM > Subject: Re: [PATCH v6] sparse: add support for _Static_assert > > Hi Lance, > > thanks for the patch. Looks very good. > Have some very minor comment below: > > > On Mon, May 8, 2017 at 2:15 PM, Lance Richardson <lrichard@redhat.com> wrote: > > @@ -1945,13 +1953,17 @@ 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; > > + if (match_ident(token, &_Static_assert_ident)) > > + token = parse_static_assert(token, NULL); > > I agree with Luc that his better with a "continue" then > the following "else" section does not need to changed. > > > > @@ -2093,6 +2105,37 @@ 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 *cond = NULL, *message = NULL; > > + struct token *errtok; > > + int valid = 1; > > + > > + token = expect(token->next, '(', "after _Static_assert"); > > + errtok = token; > > + token = constant_expression(token, &cond); > > + if (!cond) { > > I think errtok is not needed here. If there is an expression, the normal > case. Then errtok will holding the start of the expression. errok->pos > == cond->pos. > Call it errtok is a bit miss leading. > > If cond == NULL, that means the expression is empty. Then we have > errtok == token. Either way errtok is not very useful. > > > + sparse_error(errtok->pos, "Expected constant expression"); > > BTW, when the error happen here, "errtok" is actually the next token after > the missing expression, normally the ','. So the error actually happen before > that token. > OK > > + valid = 0; > > + } > > + token = expect(token, ',', "after conditional expression in > > _Static_assert"); > > + errtok = token; > > + token = parse_expression(token, &message); > > + if (!message || message->type != EXPR_STRING) { > > + sparse_error(errtok->pos, "bad or missing string literal"); > > + valid = 0; > > + } > > + token = expect(token, ')', "after diagnostic message in > > _Static_assert"); > > + > > + token = expect(token, ';', "after _Static_assert()"); > > There is some white space mix with tab in this line. > > > + > > + if (valid && !const_expression_value(cond) && cond->type == > > EXPR_VALUE) > > I am tempted to get rid of the "valid" variable. BTW, the "cond->type > == EXPR_VALUE" > should take place *before* the "!const_expression_value(cond)", other > wise it will try > to get const expression for non value expression, due to the evaluate order. > > Some thing like: > > if (cond && cond->type == EXPR_VALUE && > !const_expression_value(cond)) This doesn't work for some cases. E.g. for an expression like "sizeof(struct s) == 16", cond->type is EXPR_COMPARE before const_expression_value(cond) is called and is only set to EXPR_VALUE after the call has reduced the expression to a value. I was looking at this test as a way to verify that const_expression_value() was successful. I do think we can get rid of the "valid" variable though, as you suggest. > > > + sparse_error(cond->pos, "static assertion failed: %s", > > + show_string(message->string)); > > Then there: > message ? show_string(message->string) : ""); > > I think the assert failed without message string, we already report > the error on empty string > expression. Raising the assert here might be acceptable? > I was taking the more conservative approach of not assuming anything about the interpretation of the two expressions if the assertion is not syntactically correct. This seems like a better way to go. > Chris > Thanks for the feedback, I will roll a new version shortly. 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, May 9, 2017 at 10:42 AM, Lance Richardson <lrichard@redhat.com> wrote: >> if (cond && cond->type == EXPR_VALUE && >> !const_expression_value(cond)) > > This doesn't work for some cases. E.g. for an expression like > "sizeof(struct s) == 16", cond->type is EXPR_COMPARE before > const_expression_value(cond) is called and is only set to EXPR_VALUE after > the call has reduced the expression to a value. I was looking at this test > as a way to verify that const_expression_value() was successful. OK. That make sense. >> >> > + sparse_error(cond->pos, "static assertion failed: %s", >> > + show_string(message->string)); >> >> Then there: >> message ? show_string(message->string) : ""); >> >> I think the assert failed without message string, we already report >> the error on empty string >> expression. Raising the assert here might be acceptable? >> > > I was taking the more conservative approach of not assuming anything > about the interpretation of the two expressions if the assertion is not > syntactically correct. This seems like a better way to go. That is fine you chose that way. 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
diff --git a/ident-list.h b/ident-list.h index 8cc66a5..3c75477 100644 --- a/ident-list.h +++ b/ident-list.h @@ -16,6 +16,7 @@ IDENT_RESERVED(for); IDENT_RESERVED(while); IDENT_RESERVED(do); IDENT_RESERVED(goto); +IDENT_RESERVED(_Static_assert); /* C typenames. They get marked as reserved when initialized */ IDENT(struct); diff --git a/parse.c b/parse.c index 80f0337..f570f07 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 *); @@ -328,6 +329,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, }; @@ -466,6 +471,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 }, @@ -1945,13 +1953,17 @@ 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; + if (match_ident(token, &_Static_assert_ident)) + 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; } @@ -2093,6 +2105,37 @@ 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 *cond = NULL, *message = NULL; + struct token *errtok; + int valid = 1; + + token = expect(token->next, '(', "after _Static_assert"); + errtok = token; + token = constant_expression(token, &cond); + if (!cond) { + sparse_error(errtok->pos, "Expected constant expression"); + valid = 0; + } + token = expect(token, ',', "after conditional expression in _Static_assert"); + errtok = token; + token = parse_expression(token, &message); + if (!message || message->type != EXPR_STRING) { + sparse_error(errtok->pos, "bad or missing string literal"); + valid = 0; + } + token = expect(token, ')', "after diagnostic message in _Static_assert"); + + token = expect(token, ';', "after _Static_assert()"); + + if (valid && !const_expression_value(cond) && cond->type == EXPR_VALUE) + sparse_error(cond->pos, "static assertion failed: %s", + show_string(message->string)); + + return token; +} + /* Make a statement out of an expression */ static struct statement *make_statement(struct expression *expr) { @@ -2474,6 +2517,10 @@ static struct token * statement_list(struct token *token, struct statement_list break; if (match_op(token, '}')) break; + if (match_ident(token, &_Static_assert_ident)) { + token = parse_static_assert(token, NULL); + continue; + } if (lookup_type(token)) { if (seen_statement) { warning(token->pos, "mixing declarations and code"); @@ -2819,7 +2866,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..237f33a --- /dev/null +++ b/validation/static_assert.c @@ -0,0 +1,71 @@ +_Static_assert(1, "global ok"); + +struct foo { + _Static_assert(1, "struct ok"); +}; + +void bar(void) +{ + _Static_assert(1, " func1 ok"); + int i; + i = 0; + _Static_assert(1, " func2 ok"); + + if (1) { + _Static_assert(1, " func3 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"); + +_Static_assert(1, ); +_Static_assert(, ""); +_Static_assert(,); + +/* + * check-name: static assertion + * + * check-error-start +static_assert.c:19:16: error: static assertion failed: "expected assertion failure" +static_assert.c:22:16: error: bad constant expression +static_assert.c:25:16: error: bad constant expression +static_assert.c:27:16: error: bad constant expression +static_assert.c:35:19: error: bad or missing string literal +static_assert.c:37:18: error: bad constant expression +static_assert.c:52:19: error: bad or missing string literal +static_assert.c:53:16: error: Expected constant expression +static_assert.c:54:16: error: Expected constant expression +static_assert.c:54:17: error: bad or missing string literal + * 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> --- v6: Incorporated feedback from Christopher Li, improved tests. - rebased on sparse-next branch - use match_ident(), eliminated match_static_assert() - reworked parse_static_assert() for better error reporting. introduced "errtok" variable to accurately report error column. - Moved static assert parsing in statement_list() for better readability, use "continue" to avoid duplicating lines. - Fixed whitespace issues (from "git am") in static_assert.c. - Added test cases for missing conditional expression and missing diagnostic string. v5: Incorporated feedback from Christopher Li and Luc van Oostenryck: - Made _Static_assert a reserved identifier - Simplified check for _Static_assert keyword, consolidated into a common function. - Improved the "static assert within a function body" test case by adding a static assertion intermingled with code and adding a static assertion within a compound statement block. - Fixed use of initialized stmt variable. Tested by using sparse on entire kernel tree and a similarly-sized code tree which makes use of _Static_assert(). 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). ident-list.h | 1 + parse.c | 61 ++++++++++++++++++++++++++++++++++----- validation/static_assert.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 126 insertions(+), 7 deletions(-) create mode 100644 validation/static_assert.c