* [PATCH v6] sparse: add support for _Static_assert
@ 2017-05-08 21:15 Lance Richardson
2017-05-08 23:25 ` Luc Van Oostenryck
2017-05-09 15:53 ` Christopher Li
0 siblings, 2 replies; 6+ messages in thread
From: Lance Richardson @ 2017-05-08 21:15 UTC (permalink / raw)
To: linux-sparse
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
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
+ */
--
2.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v6] sparse: add support for _Static_assert
2017-05-08 21:15 [PATCH v6] sparse: add support for _Static_assert Lance Richardson
@ 2017-05-08 23:25 ` Luc Van Oostenryck
2017-05-08 23:45 ` Lance Richardson
2017-05-09 15:53 ` Christopher Li
1 sibling, 1 reply; 6+ messages in thread
From: Luc Van Oostenryck @ 2017-05-08 23:25 UTC (permalink / raw)
To: Lance Richardson; +Cc: linux-sparse
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v6] sparse: add support for _Static_assert
2017-05-08 23:25 ` Luc Van Oostenryck
@ 2017-05-08 23:45 ` Lance Richardson
0 siblings, 0 replies; 6+ messages in thread
From: Lance Richardson @ 2017-05-08 23:45 UTC (permalink / raw)
To: Luc Van Oostenryck; +Cc: linux-sparse
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v6] sparse: add support for _Static_assert
2017-05-08 21:15 [PATCH v6] sparse: add support for _Static_assert Lance Richardson
2017-05-08 23:25 ` Luc Van Oostenryck
@ 2017-05-09 15:53 ` Christopher Li
2017-05-09 17:42 ` Lance Richardson
1 sibling, 1 reply; 6+ messages in thread
From: Christopher Li @ 2017-05-09 15:53 UTC (permalink / raw)
To: Lance Richardson; +Cc: Linux-Sparse
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v6] sparse: add support for _Static_assert
2017-05-09 15:53 ` Christopher Li
@ 2017-05-09 17:42 ` Lance Richardson
2017-05-09 23:21 ` Christopher Li
0 siblings, 1 reply; 6+ messages in thread
From: Lance Richardson @ 2017-05-09 17:42 UTC (permalink / raw)
To: Christopher Li; +Cc: Linux-Sparse
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v6] sparse: add support for _Static_assert
2017-05-09 17:42 ` Lance Richardson
@ 2017-05-09 23:21 ` Christopher Li
0 siblings, 0 replies; 6+ messages in thread
From: Christopher Li @ 2017-05-09 23:21 UTC (permalink / raw)
To: Lance Richardson; +Cc: Linux-Sparse
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-09 23:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-08 21:15 [PATCH v6] sparse: add support for _Static_assert Lance Richardson
2017-05-08 23:25 ` Luc Van Oostenryck
2017-05-08 23:45 ` Lance Richardson
2017-05-09 15:53 ` Christopher Li
2017-05-09 17:42 ` Lance Richardson
2017-05-09 23:21 ` Christopher Li
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.