| Message ID | 1452105317-5828-1-git-send-email-lrichard@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed, Jan 06, 2016 at 01:35:17PM -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 case for static assert support Nice, it was something that was indeed missing. I have a few remarks, mostly nitpicking, here under. Luc > @@ -437,6 +443,10 @@ static struct init_keyword { > { "__restrict", NS_TYPEDEF, .op = &restrict_op}, > { "__restrict__", NS_TYPEDEF, .op = &restrict_op}, > > + > + /* Static assertion */ > + { "_Static_assert", NS_TYPEDEF, .op = &static_assert_op }, > + It seems a bit strange to me to use NS_TYPEDEF, as this is unrelated types. OTOH, the other namespaces deosn't seems better suited, and yes C11 define this as sort of declaration, so ... > @@ -2004,6 +2014,27 @@ static struct token *parse_asm_declarator(struct token *token, struct decl_state > return token; > } > > + > +static struct token *static_assert_specifier(struct token *token, struct decl_state *ctx) > +{ > + struct expression *expr = NULL; > + int val; > + > + token = constant_expression(token->next, &expr); > + if (!expr) > + sparse_error(token->pos, "Expected constant expression"); > + val = get_expression_value(expr); > + token = expect(token, ',', "after first argument of _Static_assert"); > + token = parse_expression(token, &expr); > + token = expect(token, ')', "after second argument of _Static_assert"); > + > + if (!val) > + sparse_error(token->pos, "static assertion failed: %s", > + show_string(expr->string)); By using token->pos here, the error message will indicate that the problem is situated at the very end of the assertion; more exactly, at the ending ";". This is a little annoying I find. It would be better to use the some position as the expression (expr->pos, or the same as for "Expected constant expression"). I find also a bit strange to have this sort of semantic check inside the parser. > diff --git a/validation/static_assert.c b/validation/static_assert.c > new file mode 100644 > index 0000000..baab346 > --- /dev/null > +++ b/validation/static_assert.c > @@ -0,0 +1,20 @@ > +_Static_assert(1, "global ok"); > + > +struct foo { > + _Static_assert(1, "struct ok"); > +}; > + > +void bar(void) > +{ > + _Static_assert(1, " func ok"); > +} > + > +_Static_assert(0, "expected failure"); > +/* > + * check-name: static assertion > + * > + * check-error-start > +static_assert.c:12:38: error: static assertion failed: "expected failure" > + * check-error-end > + */ It would be nice to add a few more test cases, I'm thinbking to things like: 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 int array[4]; _Static_assert(sizeof(array), "sizeof expression"); static const char non_literal_string[] = "non literal string"; _Static_assert(0, non_literal_string); Also, what should happen with: _Static_assert(1 / 0, "invalid expression: should not show up?"); (The C11 standard is not clear to me, GCC outputs an "invalid expression" message and ignore the assertion). Finally, the standard also allow to place the assertion inside a struct or union declaration; like: struct s { char c; _Static_assert(1, "inside struct"); }; which is working fine. And also, I think: struct s2 { char c; _Static_assert(sizeof(struct s) == 1, "own struct sizeof"); }; which doesn't. Yours, 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 ----- > On Wed, Jan 06, 2016 at 01:35:17PM -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 case for static assert support > > Nice, it was something that was indeed missing. > > I have a few remarks, mostly nitpicking, here under. Hi Luc, thanks for taking the time to review this. > > > Luc > > > @@ -437,6 +443,10 @@ static struct init_keyword { > > { "__restrict", NS_TYPEDEF, .op = &restrict_op}, > > { "__restrict__", NS_TYPEDEF, .op = &restrict_op}, > > > > + > > + /* Static assertion */ > > + { "_Static_assert", NS_TYPEDEF, .op = &static_assert_op }, > > + > > It seems a bit strange to me to use NS_TYPEDEF, as this is unrelated types. > OTOH, the other namespaces deosn't seems better suited, > and yes C11 define this as sort of declaration, so ... Agreed. > > > @@ -2004,6 +2014,27 @@ static struct token *parse_asm_declarator(struct > > token *token, struct decl_state > > return token; > > } > > > > + > > +static struct token *static_assert_specifier(struct token *token, struct > > decl_state *ctx) > > +{ > > + struct expression *expr = NULL; > > + int val; > > + > > + token = constant_expression(token->next, &expr); > > + if (!expr) > > + sparse_error(token->pos, "Expected constant expression"); > > + val = get_expression_value(expr); > > + token = expect(token, ',', "after first argument of _Static_assert"); > > + token = parse_expression(token, &expr); > > + token = expect(token, ')', "after second argument of _Static_assert"); > > + > > + if (!val) > > + sparse_error(token->pos, "static assertion failed: %s", > > + show_string(expr->string)); > > By using token->pos here, the error message will indicate that the problem is > situated at the very end of the assertion; more exactly, at the ending ";". > This is a little annoying I find. > It would be better to use the some position as the expression > (expr->pos, or the same as for "Expected constant expression"). Will fix in v2. > > I find also a bit strange to have this sort of semantic check inside the > parser. This prompted me to look at the gcc implementation, it appears that gcc also processes _Static_assert() during parsing. > > > diff --git a/validation/static_assert.c b/validation/static_assert.c > > new file mode 100644 > > index 0000000..baab346 > > --- /dev/null > > +++ b/validation/static_assert.c > > @@ -0,0 +1,20 @@ > > +_Static_assert(1, "global ok"); > > + > > +struct foo { > > + _Static_assert(1, "struct ok"); > > +}; > > + > > +void bar(void) > > +{ > > + _Static_assert(1, " func ok"); > > +} > > + > > +_Static_assert(0, "expected failure"); > > +/* > > + * check-name: static assertion > > + * > > + * check-error-start > > +static_assert.c:12:38: error: static assertion failed: "expected failure" > > + * check-error-end > > + */ > > It would be nice to add a few more test cases, > I'm thinbking to things like: > 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 int array[4]; > _Static_assert(sizeof(array), "sizeof expression"); > > static const char non_literal_string[] = "non literal string"; > _Static_assert(0, non_literal_string); > Will do. > Also, what should happen with: > _Static_assert(1 / 0, "invalid expression: should not show up?"); > (The C11 standard is not clear to me, GCC outputs an "invalid expression" > message and ignore the assertion). v2 will output something like "invalid constant integer expression". > > Finally, the standard also allow to place the assertion > inside a struct or union declaration; like: > struct s { > char c; > _Static_assert(1, "inside struct"); > }; > which is working fine. > And also, I think: > struct s2 { > char c; > _Static_assert(sizeof(struct s) == 1, "own struct sizeof"); > }; > which doesn't. The second case should work (unless you intended "sizeof(struct s2)"), the v2 implementation will be a little different to address this issue. > > > Yours, > 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 > Thanks again for the feedback, I'll send v2 of the patch in the next few days. -- 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, Jan 10, 2016 at 09:54:24PM -0500, Lance Richardson wrote: > ----- Original Message ----- > > On Wed, Jan 06, 2016 at 01:35:17PM -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 case for static assert support > > > > Nice, it was something that was indeed missing. > > > > I have a few remarks, mostly nitpicking, here under. > > Hi Luc, thanks for taking the time to review this. Glad to help! ... > > > > I find also a bit strange to have this sort of semantic check inside the > > parser. > > This prompted me to look at the gcc implementation, it appears that gcc also > processes _Static_assert() during parsing. It can always be moved elsewhere if needed. > > Also, what should happen with: > > _Static_assert(1 / 0, "invalid expression: should not show up?"); > > (The C11 standard is not clear to me, GCC outputs an "invalid expression" > > message and ignore the assertion). > > v2 will output something like "invalid constant integer expression". Please don't. That's the job of constant_expression() to emit a good error message. Here, the best thing to do is to *not* emit an error message if the expression is not a valid expression, constant (for example, by forcing val to 1 when expr is null). > > > > Finally, the standard also allow to place the assertion > > inside a struct or union declaration; like: > > struct s { > > char c; > > _Static_assert(1, "inside struct"); > > }; > > which is working fine. > > And also, I think: > > struct s2 { > > char c; > > _Static_assert(sizeof(struct s) == 1, "own struct sizeof"); > > }; > > which doesn't. > > The second case should work (unless you intended "sizeof(struct s2)"), the v2 > implementation will be a little different to address this issue. Yep, I messed it up. I indeed meant "sizeof(struct s2)" and it seems to work fine but for the wrong reason. See what I mean with the following example: struct s3 { char c; _Static_assert(sizeof(struct s3) == 1, "own struct sizeof, partial 1"); _Static_assert(sizeof(struct s3) == 2, "own struct sizeof, partial 2"); char d; }; but this problem is unrelated to your patch. Yours, 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 b43d683..0b1a7f2 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_specifier; static struct token *parse_if_statement(struct token *token, struct statement *stmt); static struct token *parse_return_statement(struct token *token, struct statement *stmt); @@ -308,6 +309,11 @@ static struct symbol_op asm_op = { .toplevel = toplevel_asm_declaration, }; +static struct symbol_op static_assert_op = { + .type = KW_ATTRIBUTE, + .declarator = static_assert_specifier, +}; + static struct symbol_op packed_op = { .attribute = attribute_packed, }; @@ -437,6 +443,10 @@ static struct init_keyword { { "__restrict", NS_TYPEDEF, .op = &restrict_op}, { "__restrict__", NS_TYPEDEF, .op = &restrict_op}, + + /* Static assertion */ + { "_Static_assert", NS_TYPEDEF, .op = &static_assert_op }, + /* Storage class */ { "auto", NS_TYPEDEF, .op = &auto_op }, { "register", NS_TYPEDEF, .op = ®ister_op }, @@ -2004,6 +2014,27 @@ static struct token *parse_asm_declarator(struct token *token, struct decl_state return token; } + +static struct token *static_assert_specifier(struct token *token, struct decl_state *ctx) +{ + struct expression *expr = NULL; + int val; + + token = constant_expression(token->next, &expr); + if (!expr) + sparse_error(token->pos, "Expected constant expression"); + val = get_expression_value(expr); + token = expect(token, ',', "after first argument of _Static_assert"); + token = parse_expression(token, &expr); + token = expect(token, ')', "after second argument of _Static_assert"); + + if (!val) + sparse_error(token->pos, "static assertion failed: %s", + show_string(expr->string)); + + return token; +} + /* Make a statement out of an expression */ static struct statement *make_statement(struct expression *expr) { diff --git a/validation/static_assert.c b/validation/static_assert.c new file mode 100644 index 0000000..baab346 --- /dev/null +++ b/validation/static_assert.c @@ -0,0 +1,20 @@ +_Static_assert(1, "global ok"); + +struct foo { + _Static_assert(1, "struct ok"); +}; + +void bar(void) +{ + _Static_assert(1, " func ok"); +} + +_Static_assert(0, "expected failure"); +/* + * check-name: static assertion + * + * check-error-start +static_assert.c:12:38: error: static assertion failed: "expected failure" + * 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 case for static assert support Signed-off-by: Lance Richardson <lrichard@redhat.com> --- parse.c | 33 ++++++++++++++++++++++++++++++++- validation/static_assert.c | 20 ++++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 validation/static_assert.c