* [PATCH v2 01/13] expression: introduce additional expression constness tracking flags
2016-01-25 14:47 [PATCH v2 00/13] improve constexpr handling Nicolai Stange
@ 2016-01-25 14:49 ` Nicolai Stange
2016-01-25 21:51 ` Luc Van Oostenryck
2016-01-25 14:51 ` [PATCH v2 02/13] expression: examine constness of casts at evaluation only Nicolai Stange
` (12 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Nicolai Stange @ 2016-01-25 14:49 UTC (permalink / raw)
To: linux-sparse
Cc: Nicolai Stange, Christopher Li, Josh Triplett, Luc Van Oostenryck
Prepare for a more fine-grained tracking of expression constness in the
sense of C99 [6.4.4, 6.6].
User-visible behaviour remains unchanged.
The current implementation tags an expression with either combination
of the flags Int_const_expr and Float_literal, the latter being only
used to tell that
(int).0
is indeed an integer constant expression.
Even if sparse attempted to verify that initializers for static storage
duration objects are constant expressions [6.7.8(4)] (which it
currently does not), it could not tell reliably.
Examples:
1.)
static float a = { (float)0 }; /* allowed by C99 */
'(float)0' is not an integer constant expression, but an arithmetic
one.
2.)
enum { b = 0 };
static void *c = { (void*)b }; /* disallowed by C99 */
References to enum members are not allowed in address constants
[6.6(9)] and thus, the initializer is not a constant expression at
all.
Introduce a broader set of constness tracking flags, resembling the
four types of constants [6.4.4] (integer, floating, enumeration,
character) and the three types of constant expressions [6.6] (integer,
arithmetic, address). Use helper functions to consistently set and
clear these flags as they are not completely independent.
Finally, make alloc_expression() and alloc_const_expression()
initialize the new expression's flags to zero.
Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
evaluate.c | 72 ++++++++++++++++++++--------------
expand.c | 2 +-
expression.c | 67 ++++++++++++++++++++------------
expression.h | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
4 files changed, 206 insertions(+), 58 deletions(-)
diff --git a/evaluate.c b/evaluate.c
index 035e448..0cf85ae 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -400,7 +400,7 @@ static struct symbol *bad_expr_type(struct expression *expr)
break;
}
- expr->flags = 0;
+ expr->flags = EXPR_FLAG_NONE;
return expr->ctype = &bad_ctype;
}
@@ -880,8 +880,9 @@ static struct symbol *evaluate_logical(struct expression *expr)
/* the result is int [6.5.13(3), 6.5.14(3)] */
expr->ctype = &int_ctype;
if (expr->flags) {
- if (!(expr->left->flags & expr->right->flags & Int_const_expr))
- expr->flags = 0;
+ if (!(expr->left->flags & expr->right->flags &
+ EXPR_FLAG_INT_CONST_EXPR))
+ expr->flags = EXPR_FLAG_NONE;
}
return &int_ctype;
}
@@ -894,8 +895,9 @@ static struct symbol *evaluate_binop(struct expression *expr)
int op = expr->op;
if (expr->flags) {
- if (!(expr->left->flags & expr->right->flags & Int_const_expr))
- expr->flags = 0;
+ if (!(expr->left->flags & expr->right->flags &
+ EXPR_FLAG_INT_CONST_EXPR))
+ expr->flags = EXPR_FLAG_NONE;
}
/* number op number */
@@ -986,7 +988,7 @@ static inline int is_null_pointer_constant(struct expression *e)
{
if (e->ctype == &null_ctype)
return 1;
- if (!(e->flags & Int_const_expr))
+ if (!(e->flags & EXPR_FLAG_INT_CONST_EXPR))
return 0;
return is_zero_constant(e) ? 2 : 0;
}
@@ -1001,8 +1003,9 @@ static struct symbol *evaluate_compare(struct expression *expr)
const char *typediff;
if (expr->flags) {
- if (!(expr->left->flags & expr->right->flags & Int_const_expr))
- expr->flags = 0;
+ if (!(expr->left->flags & expr->right->flags &
+ EXPR_FLAG_INT_CONST_EXPR))
+ expr->flags = EXPR_FLAG_NONE;
}
/* Type types? */
@@ -1120,10 +1123,14 @@ static struct symbol *evaluate_conditional_expression(struct expression *expr)
}
if (expr->flags) {
- int flags = expr->conditional->flags & Int_const_expr;
+ unsigned char flags_mask = 0;
+ unsigned char flags;
+
+ expr_set_flag(&flags_mask, EXPR_FLAG_INT_CONST_EXPR);
+ flags = (expr->conditional->flags & flags_mask);
flags &= (*true)->flags & expr->cond_false->flags;
if (!flags)
- expr->flags = 0;
+ expr->flags = EXPR_FLAG_NONE;
}
lclass = classify_type(ltype, <ype);
@@ -1681,7 +1688,7 @@ static struct symbol *evaluate_addressof(struct expression *expr)
}
ctype = op->ctype;
*expr = *op->unop;
- expr->flags = 0;
+ expr->flags = EXPR_FLAG_NONE;
if (expr->type == EXPR_SYMBOL) {
struct symbol *sym = expr->symbol;
@@ -1709,7 +1716,7 @@ static struct symbol *evaluate_dereference(struct expression *expr)
/* Simplify: *&(expr) => (expr) */
if (op->type == EXPR_PREOP && op->op == '&') {
*expr = *op->unop;
- expr->flags = 0;
+ expr->flags = EXPR_FLAG_NONE;
return expr->ctype;
}
@@ -1799,8 +1806,8 @@ static struct symbol *evaluate_sign(struct expression *expr)
{
struct symbol *ctype = expr->unop->ctype;
int class = classify_type(ctype, &ctype);
- if (expr->flags && !(expr->unop->flags & Int_const_expr))
- expr->flags = 0;
+ if (expr->flags && !(expr->unop->flags & EXPR_FLAG_INT_CONST_EXPR))
+ expr->flags = EXPR_FLAG_NONE;
/* should be an arithmetic type */
if (!(class & TYPE_NUM))
return bad_expr_type(expr);
@@ -1854,8 +1861,9 @@ static struct symbol *evaluate_preop(struct expression *expr)
return evaluate_postop(expr);
case '!':
- if (expr->flags && !(expr->unop->flags & Int_const_expr))
- expr->flags = 0;
+ if (expr->flags && !(expr->unop->flags &
+ EXPR_FLAG_INT_CONST_EXPR))
+ expr->flags = EXPR_FLAG_NONE;
if (is_safe_type(ctype))
warning(expr->pos, "testing a 'safe expression'");
if (is_float_type(ctype)) {
@@ -2734,14 +2742,18 @@ static struct symbol *evaluate_cast(struct expression *expr)
class1 = classify_type(ctype, &t1);
- /* cast to non-integer type -> not an integer constant expression */
- if (!is_int(class1))
- expr->flags = 0;
+ /* cast to non-numeric type -> not an arithmetic expression */
+ if (!(class1 & TYPE_NUM))
+ expr_clear_flag(&expr->flags, EXPR_FLAG_ARITH_CONST_EXPR);
+ /* cast to float type -> not an integer constant expression */
+ else if (class1 & TYPE_FLOAT)
+ expr_clear_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
/* if argument turns out to be not an integer constant expression *and*
it was not a floating literal to start with -> too bad */
- else if (expr->flags == Int_const_expr &&
- !(target->flags & Int_const_expr))
- expr->flags = 0;
+ else if (!(target->flags &
+ (EXPR_FLAG_INT_CONST_EXPR | EXPR_FLAG_FP_CONST)))
+ expr_clear_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
+
/*
* You can always throw a value away by casting to
* "void" - that's an implicit "force". Note that
@@ -2803,7 +2815,7 @@ static struct symbol *evaluate_cast(struct expression *expr)
"cast adds address space to expression (<asn:%d>)", as1);
if (!(t1->ctype.modifiers & MOD_PTRINHERIT) && class1 == TYPE_PTR &&
- !as1 && (target->flags & Int_const_expr)) {
+ !as1 && (target->flags & EXPR_FLAG_INT_CONST_EXPR)) {
if (t1->ctype.base_type == &void_ctype) {
if (is_zero_constant(target)) {
/* NULL */
@@ -2933,7 +2945,8 @@ static struct symbol *evaluate_offsetof(struct expression *expr)
}
ctype = field;
expr->type = EXPR_VALUE;
- expr->flags = Int_const_expr;
+ expr->flags = EXPR_FLAG_NONE;
+ expr_set_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
expr->value = offset;
expr->taint = 0;
expr->ctype = size_t_ctype;
@@ -2951,7 +2964,8 @@ static struct symbol *evaluate_offsetof(struct expression *expr)
ctype = ctype->ctype.base_type;
if (!expr->index) {
expr->type = EXPR_VALUE;
- expr->flags = Int_const_expr;
+ expr->flags = EXPR_FLAG_NONE;
+ expr_set_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
expr->value = 0;
expr->taint = 0;
expr->ctype = size_t_ctype;
@@ -2968,24 +2982,26 @@ static struct symbol *evaluate_offsetof(struct expression *expr)
m = alloc_const_expression(expr->pos,
bits_to_bytes(ctype->bit_size));
m->ctype = size_t_ctype;
- m->flags = Int_const_expr;
+ expr_set_flag(&m->flags, EXPR_FLAG_INT_CONST_EXPR);
expr->type = EXPR_BINOP;
expr->left = idx;
expr->right = m;
expr->op = '*';
expr->ctype = size_t_ctype;
- expr->flags = m->flags & idx->flags & Int_const_expr;
+ expr->flags = m->flags & idx->flags;
}
}
if (e) {
struct expression *copy = __alloc_expression(0);
+ unsigned char flags_mask = EXPR_FLAG_NONE;
*copy = *expr;
if (e->type == EXPR_OFFSETOF)
e->in = ctype;
if (!evaluate_expression(e))
return NULL;
expr->type = EXPR_BINOP;
- expr->flags = e->flags & copy->flags & Int_const_expr;
+ expr_set_flag(&flags_mask, EXPR_FLAG_INT_CONST_EXPR);
+ expr->flags = e->flags & copy->flags & flags_mask;
expr->op = '+';
expr->ctype = size_t_ctype;
expr->left = copy;
diff --git a/expand.c b/expand.c
index 0f6720c..afca611 100644
--- a/expand.c
+++ b/expand.c
@@ -1212,7 +1212,7 @@ static int expand_statement(struct statement *stmt)
static inline int bad_integer_constant_expression(struct expression *expr)
{
- if (!(expr->flags & Int_const_expr))
+ if (!(expr->flags & EXPR_FLAG_INT_CONST_EXPR))
return 1;
if (expr->taint & Taint_comma)
return 1;
diff --git a/expression.c b/expression.c
index 7293d47..eccdb5a 100644
--- a/expression.c
+++ b/expression.c
@@ -131,7 +131,7 @@ static struct token *parse_type(struct token *token, struct expression **tree)
{
struct symbol *sym;
*tree = alloc_expression(token->pos, EXPR_TYPE);
- (*tree)->flags = Int_const_expr; /* sic */
+ expr_set_flag(&(*tree)->flags, EXPR_FLAG_INT_CONST_EXPR); /* sic */
token = typename(token, &sym, NULL);
if (sym->ident)
sparse_error(token->pos,
@@ -146,7 +146,7 @@ static struct token *builtin_types_compatible_p_expr(struct token *token,
{
struct expression *expr = alloc_expression(
token->pos, EXPR_COMPARE);
- expr->flags = Int_const_expr;
+ expr_set_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
expr->op = SPECIAL_EQUAL;
token = token->next;
if (!match_op(token, '('))
@@ -200,7 +200,7 @@ static struct token *builtin_offsetof_expr(struct token *token,
return expect(token, ')', "at end of __builtin_offset");
case SPECIAL_DEREFERENCE:
e = alloc_expression(token->pos, EXPR_OFFSETOF);
- e->flags = Int_const_expr;
+ expr_set_flag(&e->flags, EXPR_FLAG_INT_CONST_EXPR);
e->op = '[';
*p = e;
p = &e->down;
@@ -208,7 +208,7 @@ static struct token *builtin_offsetof_expr(struct token *token,
case '.':
token = token->next;
e = alloc_expression(token->pos, EXPR_OFFSETOF);
- e->flags = Int_const_expr;
+ expr_set_flag(&e->flags, EXPR_FLAG_INT_CONST_EXPR);
e->op = '.';
if (token_type(token) != TOKEN_IDENT) {
sparse_error(token->pos, "Expected member name");
@@ -220,7 +220,7 @@ static struct token *builtin_offsetof_expr(struct token *token,
case '[':
token = token->next;
e = alloc_expression(token->pos, EXPR_OFFSETOF);
- e->flags = Int_const_expr;
+ expr_set_flag(&e->flags, EXPR_FLAG_INT_CONST_EXPR);
e->op = '[';
token = parse_expression(token, &e->index);
token = expect(token, ']',
@@ -336,7 +336,8 @@ got_it:
"likely to produce unsigned long (and a warning) here",
show_token(token));
expr->type = EXPR_VALUE;
- expr->flags = Int_const_expr;
+ expr->flags = EXPR_FLAG_NONE;
+ expr_set_flag(&expr->flags, EXPR_FLAG_INT_CONST);
expr->ctype = ctype_integer(size, want_unsigned);
expr->value = value;
return;
@@ -361,7 +362,8 @@ Float:
else
goto Enoint;
- expr->flags = Float_literal;
+ expr->flags = EXPR_FLAG_NONE;
+ expr_set_flag(&expr->flags, EXPR_FLAG_FP_CONST);
expr->type = EXPR_FVALUE;
return;
@@ -376,7 +378,7 @@ struct token *primary_expression(struct token *token, struct expression **tree)
switch (token_type(token)) {
case TOKEN_CHAR ... TOKEN_WIDE_CHAR_EMBEDDED_3:
expr = alloc_expression(token->pos, EXPR_VALUE);
- expr->flags = Int_const_expr;
+ expr_set_flag(&expr->flags, EXPR_FLAG_CHAR_CONST);
expr->ctype = token_type(token) < TOKEN_WIDE_CHAR ? &int_ctype : &long_ctype;
get_char_constant(token, &expr->value);
token = token->next;
@@ -390,7 +392,7 @@ struct token *primary_expression(struct token *token, struct expression **tree)
case TOKEN_ZERO_IDENT: {
expr = alloc_expression(token->pos, EXPR_SYMBOL);
- expr->flags = Int_const_expr;
+ expr_set_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
expr->ctype = &int_ctype;
expr->symbol = &zero_int;
expr->symbol_name = token->ident;
@@ -417,7 +419,7 @@ struct token *primary_expression(struct token *token, struct expression **tree)
*expr = *sym->initializer;
/* we want the right position reported, thus the copy */
expr->pos = token->pos;
- expr->flags = Int_const_expr;
+ expr_set_flag(&expr->flags, EXPR_FLAG_ENUM_CONST);
token = next;
break;
}
@@ -457,7 +459,8 @@ struct token *primary_expression(struct token *token, struct expression **tree)
}
if (token->special == '[' && lookup_type(token->next)) {
expr = alloc_expression(token->pos, EXPR_TYPE);
- expr->flags = Int_const_expr; /* sic */
+ /* sic */
+ expr_set_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
token = typename(token->next, &expr->symbol, NULL);
token = expect(token, ']', "in type expression");
break;
@@ -573,7 +576,8 @@ static struct token *type_info_expression(struct token *token,
struct token *p;
*tree = expr;
- expr->flags = Int_const_expr; /* XXX: VLA support will need that changed */
+ /* XXX: VLA support will need that changed */
+ expr_set_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
token = token->next;
if (!match_op(token, '(') || !lookup_type(token->next))
return unary_expression(token, &expr->cast_expression);
@@ -662,7 +666,8 @@ static struct token *unary_expression(struct token *token, struct expression **t
unary = alloc_expression(token->pos, EXPR_PREOP);
unary->op = token->special;
unary->unop = unop;
- unary->flags = unop->flags & Int_const_expr;
+ unary->flags = unop->flags;
+ expr_flags_decay_consts(&unary->flags);
*tree = unary;
return next;
}
@@ -720,10 +725,25 @@ static struct token *cast_expression(struct token *token, struct expression **tr
if (!v)
return token;
cast->cast_expression = v;
- if (v->flags & Int_const_expr)
- cast->flags = Int_const_expr;
- else if (v->flags & Float_literal) /* and _not_ int */
- cast->flags = Int_const_expr | Float_literal;
+
+ cast->flags = v->flags;
+ expr_flags_decay_consts(&cast->flags);
+ /*
+ * Up to now, we missed the (int).0 case here
+ * which should really get a
+ * EXPR_FLAG_INT_CONST_EXPR marker. Also,
+ * conversion to non-numeric types is not
+ * properly reflected up to this point.
+ * However, we do not know until evaluation.
+ * For the moment, in order to preserve
+ * semantics, speculatively set
+ * EXPR_FLAG_INT_CONST_EXPR if
+ * EXPR_FLAG_FP_CONST is set. evaluate_cast()
+ * will unset inappropriate flags again after
+ * examining type information.
+ */
+ if (v->flags & EXPR_FLAG_FP_CONST)
+ cast->flags |= EXPR_FLAG_INT_CONST_EXPR;
return token;
}
}
@@ -760,8 +780,8 @@ static struct token *cast_expression(struct token *token, struct expression **tr
sparse_error(next->pos, "No right hand side of '%s'-expression", show_special(op)); \
break; \
} \
- top->flags = left->flags & right->flags \
- & Int_const_expr; \
+ top->flags = left->flags & right->flags; \
+ expr_flags_decay_consts(&top->flags); \
top->op = op; \
top->left = left; \
top->right = right; \
@@ -865,12 +885,11 @@ struct token *conditional_expression(struct token *token, struct expression **tr
token = expect(token, ':', "in conditional expression");
token = conditional_expression(token, &expr->cond_false);
if (expr->left && expr->cond_false) {
- int is_const = expr->left->flags &
- expr->cond_false->flags &
- Int_const_expr;
+ expr->flags = expr->left->flags &
+ expr->cond_false->flags;
if (expr->cond_true)
- is_const &= expr->cond_true->flags;
- expr->flags = is_const;
+ expr->flags &= expr->cond_true->flags;
+ expr_flags_decay_consts(&expr->flags);
}
}
return token;
diff --git a/expression.h b/expression.h
index 80b3be5..b9f6f08 100644
--- a/expression.h
+++ b/expression.h
@@ -66,10 +66,121 @@ enum expression_type {
EXPR_OFFSETOF,
};
-enum {
- Int_const_expr = 1,
- Float_literal = 2,
-}; /* for expr->flags */
+
+/*
+ * Flags for tracking the promotion of various attributes from
+ * subexpressions to their parents.
+ *
+ * Currently, they only cope with an expression's constness as defined
+ * by C99.
+ *
+ * The flags are not independent as one might imply another. Use
+ * expr_set_flag() and expr_clear_flag() for setting and clearing a
+ * particular flag.
+ */
+enum expression_flags {
+ EXPR_FLAG_NONE = 0,
+ /*
+ * A constant in the sense of [6.4.4]:
+ * - Integer constant [6.4.4.1]
+ * - Floating point constant [6.4.4.2]
+ * - Enumeration constant [6.4.4.3]
+ * - Character constant [6.4.4.4]
+ */
+ EXPR_FLAG_INT_CONST = (1 << 0),
+ EXPR_FLAG_FP_CONST = (1 << 1),
+ EXPR_FLAG_ENUM_CONST = (1 << 2),
+ EXPR_FLAG_CHAR_CONST = (1 << 3),
+
+ /*
+ * A constant expression in the sense of [6.6]:
+ * - integer constant expression [6.6(6)]
+ * - arithmetic constant expression [6.6(8)]
+ * - address constanr [6.6(9)]
+ */
+ EXPR_FLAG_INT_CONST_EXPR = (1 << 4),
+ EXPR_FLAG_ARITH_CONST_EXPR = (1 << 5),
+ EXPR_FLAG_ADDR_CONST_EXPR = (1 << 6),
+};
+
+/*
+ * Set a particular flag among an expression's ones.
+ *
+ * The set of flags defined is not completely independent. Take care
+ * that implications keep obeyed.
+ *
+ * Only one single flag from enum expression_flags is allowed for
+ * the flag argument at a time.
+ */
+static inline void expr_set_flag(unsigned char * const flags,
+ enum expression_flags flag)
+{
+ /* obey the implications */
+ switch (flag) {
+ case EXPR_FLAG_INT_CONST:
+ case EXPR_FLAG_ENUM_CONST:
+ case EXPR_FLAG_CHAR_CONST:
+ flag |= EXPR_FLAG_INT_CONST_EXPR;
+ /* fallthrough */
+ case EXPR_FLAG_FP_CONST:
+ case EXPR_FLAG_INT_CONST_EXPR:
+ flag |= EXPR_FLAG_ARITH_CONST_EXPR;
+ /* fallthrough */
+ case EXPR_FLAG_ARITH_CONST_EXPR:
+ case EXPR_FLAG_ADDR_CONST_EXPR:
+ case EXPR_FLAG_NONE:
+ break;
+ }
+
+ *flags |= flag;
+}
+
+/*
+ * Clear a particular flag among an expression's ones.
+ *
+ * The set of flags defined is not completely independent. Take care
+ * that implications keep obeyed.
+ *
+ * Only one single flag from enum expression_flags is allowed for
+ * the flag argument at a time.
+ */
+static inline void expr_clear_flag(unsigned char * const flags,
+ enum expression_flags flag)
+{
+ /* obey the implications */
+ switch (flag) {
+ case EXPR_FLAG_ARITH_CONST_EXPR:
+ flag |= EXPR_FLAG_INT_CONST_EXPR;
+ flag |= EXPR_FLAG_FP_CONST;
+ /* fallthrough */
+ case EXPR_FLAG_INT_CONST_EXPR:
+ flag |= EXPR_FLAG_INT_CONST;
+ flag |= EXPR_FLAG_ENUM_CONST;
+ flag |= EXPR_FLAG_CHAR_CONST;
+ /* fallthrough */
+ case EXPR_FLAG_ADDR_CONST_EXPR:
+ case EXPR_FLAG_INT_CONST:
+ case EXPR_FLAG_FP_CONST:
+ case EXPR_FLAG_ENUM_CONST:
+ case EXPR_FLAG_CHAR_CONST:
+ case EXPR_FLAG_NONE:
+ break;
+ }
+
+ *flags &= ~flag;
+}
+
+/*
+ * Remove any "Constant" [6.4.4] flag, but retain the "constant
+ * expression" [6.6] flags.
+ * Used to merge the constantness flags of primary subexpressions
+ * into their parent expressions' ones.
+ */
+static inline void expr_flags_decay_consts(unsigned char * const flags)
+{
+ *flags &= ~(EXPR_FLAG_INT_CONST | EXPR_FLAG_FP_CONST |
+ EXPR_FLAG_ENUM_CONST | EXPR_FLAG_CHAR_CONST);
+}
enum {
Taint_comma = 1,
@@ -77,7 +188,7 @@ enum {
struct expression {
enum expression_type type:8;
- unsigned flags:8;
+ unsigned char flags;
int op;
struct position pos;
struct symbol *ctype;
@@ -199,6 +310,7 @@ static inline struct expression *alloc_expression(struct position pos, int type)
{
struct expression *expr = __alloc_expression(0);
expr->type = type;
+ expr->flags = EXPR_FLAG_NONE;
expr->pos = pos;
return expr;
}
@@ -207,6 +319,7 @@ static inline struct expression *alloc_const_expression(struct position pos, int
{
struct expression *expr = __alloc_expression(0);
expr->type = EXPR_VALUE;
+ expr->flags = EXPR_FLAG_NONE;
expr->pos = pos;
expr->value = value;
expr->ctype = &int_ctype;
--
2.7.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 01/13] expression: introduce additional expression constness tracking flags
2016-01-25 14:49 ` [PATCH v2 01/13] expression: introduce additional expression constness tracking flags Nicolai Stange
@ 2016-01-25 21:51 ` Luc Van Oostenryck
2016-01-26 15:26 ` Nicolai Stange
0 siblings, 1 reply; 43+ messages in thread
From: Luc Van Oostenryck @ 2016-01-25 21:51 UTC (permalink / raw)
To: Nicolai Stange; +Cc: linux-sparse, Christopher Li, Josh Triplett
On Mon, Jan 25, 2016 at 03:49:28PM +0100, Nicolai Stange wrote:
> Prepare for a more fine-grained tracking of expression constness in the
> sense of C99 [6.4.4, 6.6].
This part could be moved at the end of the patch description
> User-visible behaviour remains unchanged.
>
> The current implementation tags an expression with either combination
> of the flags Int_const_expr and Float_literal, the latter being only
> used to tell that
> (int).0
> is indeed an integer constant expression.
This part should be dropped, I think, and maybe moved to the cover letter.
> Even if sparse attempted to verify that initializers for static storage
> duration objects are constant expressions [6.7.8(4)] (which it
> currently does not), it could not tell reliably.
> Examples:
> 1.)
> static float a = { (float)0 }; /* allowed by C99 */
> '(float)0' is not an integer constant expression, but an arithmetic
> one.
> 2.)
> enum { b = 0 };
> static void *c = { (void*)b }; /* disallowed by C99 */
> References to enum members are not allowed in address constants
> [6.6(9)] and thus, the initializer is not a constant expression at
> all.
This could be moved to the top of the patch description
> Introduce a broader set of constness tracking flags, resembling the
> four types of constants [6.4.4] (integer, floating, enumeration,
> character) and the three types of constant expressions [6.6] (integer,
> arithmetic, address). Use helper functions to consistently set and
> clear these flags as they are not completely independent.
> Finally, make alloc_expression() and alloc_const_expression()
> initialize the new expression's flags to zero.
I think this patch should be split into 2-4 smaller patches:
0) introduce the new flags, maybe first only NONE, INT_EXPR & FP_CONST
1) replace the old use of 0, Int_const_expr, ... (can be folded with 0)
2) initialize the flag in alloc_expression() (can be folded with 1)..
3) introduce the the new flags if needed, the expr_..._flag() helpers
and begin them
> --- a/expression.c
> +++ b/expression.c
> @@ -131,7 +131,7 @@ static struct token *parse_type(struct token *token, struct expression **tree)
> {
> struct symbol *sym;
> *tree = alloc_expression(token->pos, EXPR_TYPE);
> - (*tree)->flags = Int_const_expr; /* sic */
> + expr_set_flag(&(*tree)->flags, EXPR_FLAG_INT_CONST_EXPR); /* sic */
Better to remove those 'sic'. To which text do they refer?
> @@ -457,7 +459,8 @@ struct token *primary_expression(struct token *token, struct expression **tree)
> }
> if (token->special == '[' && lookup_type(token->next)) {
> expr = alloc_expression(token->pos, EXPR_TYPE);
> - expr->flags = Int_const_expr; /* sic */
> + /* sic */
> + expr_set_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
Ditto for the 'sic'
> +
> +/*
> + * Set a particular flag among an expression's ones.
> + *
> + * The set of flags defined is not completely independent. Take care
> + * that implications keep obeyed.
> + *
> + * Only one single flag from enum expression_flags is allowed for
> + * the flag argument at a time.
> + */
> +static inline void expr_set_flag(unsigned char * const flags,
> + enum expression_flags flag)
> +{
> + /* obey the implications */
> + switch (flag) {
> + case EXPR_FLAG_INT_CONST:
> + case EXPR_FLAG_ENUM_CONST:
> + case EXPR_FLAG_CHAR_CONST:
> + flag |= EXPR_FLAG_INT_CONST_EXPR;
> + /* fallthrough */
> + case EXPR_FLAG_FP_CONST:
> + case EXPR_FLAG_INT_CONST_EXPR:
> + flag |= EXPR_FLAG_ARITH_CONST_EXPR;
> + /* fallthrough */
> + case EXPR_FLAG_ARITH_CONST_EXPR:
> + case EXPR_FLAG_ADDR_CONST_EXPR:
> + case EXPR_FLAG_NONE:
> + break;
> + }
> +
> + *flags |= flag;
> +}
> +
> +/*
> + * Clear a particular flag among an expression's ones.
> + *
> + * The set of flags defined is not completely independent. Take care
> + * that implications keep obeyed.
> + *
> + * Only one single flag from enum expression_flags is allowed for
> + * the flag argument at a time.
> + */
> +static inline void expr_clear_flag(unsigned char * const flags,
> + enum expression_flags flag)
> +{
> + /* obey the implications */
> + switch (flag) {
> + case EXPR_FLAG_ARITH_CONST_EXPR:
> + flag |= EXPR_FLAG_INT_CONST_EXPR;
> + flag |= EXPR_FLAG_FP_CONST;
> + /* fallthrough */
> + case EXPR_FLAG_INT_CONST_EXPR:
> + flag |= EXPR_FLAG_INT_CONST;
> + flag |= EXPR_FLAG_ENUM_CONST;
> + flag |= EXPR_FLAG_CHAR_CONST;
> + /* fallthrough */
> + case EXPR_FLAG_ADDR_CONST_EXPR:
> + case EXPR_FLAG_INT_CONST:
> + case EXPR_FLAG_FP_CONST:
> + case EXPR_FLAG_ENUM_CONST:
> + case EXPR_FLAG_CHAR_CONST:
> + case EXPR_FLAG_NONE:
> + break;
> + }
> +
> + *flags &= ~flag;
> +}
> +
> +/*
> + * Remove any "Constant" [6.4.4] flag, but retain the "constant
> + * expression" [6.6] flags.
> + * Used to merge the constantness flags of primary subexpressions
> + * into their parent expressions' ones.
> + */
> +static inline void expr_flags_decay_consts(unsigned char * const flags)
> +{
> + *flags &= ~(EXPR_FLAG_INT_CONST | EXPR_FLAG_FP_CONST |
> + EXPR_FLAG_ENUM_CONST | EXPR_FLAG_CHAR_CONST);
> +}
I think it's already much better so!
But honestly, I still think that it could be improved:
- the fact that it need a pointer for the update, means that it can only be used
with a specific type, here unsigned char, and not arbitrary expressions
- expr_flags_decay_consts(flags) could be simply be replaced by something like:
#define EXPR_FLAG_CONSTANTS (EXPR_FLAG_INT_CONST|EXPR_FLAG_FP_CONST|...)
flags &= ~EXPR_FLAG_CONSTANTS;
- expr_clear_flag() is only used 5 times:
-) 4 times with ADDR_CONST_EXPR where it can be replaced by a simple &= ~
-) once with INT_CONST_EXPR
So, with only a few defines for the set/union, one for the 'clear' and one
one for the decay you can get rid of these helpers wich IMO improve readability
and is consistent that elsewhere in the code there is anyway manipulations of
expr->flags done simply with '|' and '&'.
> enum {
> Taint_comma = 1,
> @@ -77,7 +188,7 @@ enum {
>
> struct expression {
> enum expression_type type:8;
> - unsigned flags:8;
> + unsigned char flags;
I suppose that initialy this 'flags' was foreseen for other things than
'Int_const_expr' & 'Float_literal' but now I think it should be better
to rename it with something more explicit, like 'constness' or something
(same then for the EXPR_FLAG_... of course).
Luc
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 01/13] expression: introduce additional expression constness tracking flags
2016-01-25 21:51 ` Luc Van Oostenryck
@ 2016-01-26 15:26 ` Nicolai Stange
2016-01-26 15:37 ` Nicolai Stange
0 siblings, 1 reply; 43+ messages in thread
From: Nicolai Stange @ 2016-01-26 15:26 UTC (permalink / raw)
To: Luc Van Oostenryck
Cc: Nicolai Stange, linux-sparse, Christopher Li, Josh Triplett
Luc Van Oostenryck <luc.vanoostenryck@gmail.com> writes:
> On Mon, Jan 25, 2016 at 03:49:28PM +0100, Nicolai Stange wrote:
>> Prepare for a more fine-grained tracking of expression constness in the
>> sense of C99 [6.4.4, 6.6].
>
> This part could be moved at the end of the patch description
Yes.
>> User-visible behaviour remains unchanged.
>>
>> The current implementation tags an expression with either combination
>> of the flags Int_const_expr and Float_literal, the latter being only
>> used to tell that
>> (int).0
>> is indeed an integer constant expression.
>
> This part should be dropped, I think, and maybe moved to the cover letter.
>
I included this description of the current state of the art in order to
make the problem addressed by this patch very clear.
Maybe you're right and my problem statement is too much blah blah,
i.e. the paragraph immediately following would suffice.
>> Even if sparse attempted to verify that initializers for static storage
>> duration objects are constant expressions [6.7.8(4)] (which it
>> currently does not), it could not tell reliably.
>> Examples:
>> 1.)
>> static float a = { (float)0 }; /* allowed by C99 */
>> '(float)0' is not an integer constant expression, but an arithmetic
>> one.
>> 2.)
>> enum { b = 0 };
>> static void *c = { (void*)b }; /* disallowed by C99 */
>> References to enum members are not allowed in address constants
>> [6.6(9)] and thus, the initializer is not a constant expression at
>> all.
>
> This could be moved to the top of the patch description
Yes.
>> Introduce a broader set of constness tracking flags, resembling the
>> four types of constants [6.4.4] (integer, floating, enumeration,
>> character) and the three types of constant expressions [6.6] (integer,
>> arithmetic, address). Use helper functions to consistently set and
>> clear these flags as they are not completely independent.
>> Finally, make alloc_expression() and alloc_const_expression()
>> initialize the new expression's flags to zero.
>
> I think this patch should be split into 2-4 smaller patches:
> 0) introduce the new flags, maybe first only NONE, INT_EXPR & FP_CONST
> 1) replace the old use of 0, Int_const_expr, ... (can be folded with 0)
> 2) initialize the flag in alloc_expression() (can be folded with 1)..
> 3) introduce the the new flags if needed, the expr_..._flag() helpers
> and begin them
I'll drop the expr_..._flag() helpers as recommended by you (see below)
and introduce the additional flags (arithm. constness, addr constants,
?) as needed in later patches.
Thus, no split of this patch since 3.) would be addressed by deferring
the introduction of additional flags and 1)-3) "can be folded" anyway.
>
>> --- a/expression.c
>> +++ b/expression.c
>> @@ -131,7 +131,7 @@ static struct token *parse_type(struct token *token, struct expression **tree)
>> {
>> struct symbol *sym;
>> *tree = alloc_expression(token->pos, EXPR_TYPE);
>> - (*tree)->flags = Int_const_expr; /* sic */
>> + expr_set_flag(&(*tree)->flags, EXPR_FLAG_INT_CONST_EXPR); /* sic */
>
> Better to remove those 'sic'. To which text do they refer?
>
>> @@ -457,7 +459,8 @@ struct token *primary_expression(struct token *token, struct expression **tree)
>> }
>> if (token->special == '[' && lookup_type(token->next)) {
>> expr = alloc_expression(token->pos, EXPR_TYPE);
>> - expr->flags = Int_const_expr; /* sic */
>> + /* sic */
>> + expr_set_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
>
> Ditto for the 'sic'
That 'sic' had been introduced in
a24a3adf3f0e ("fix handling of integer constant expressions")
Quote from the commit message:
EXPR_TYPE also gets marked int_const_expr (to make it DTRT on the
builtin_same_type_p() et.al.)
Quote end.
It is a hack to turn __builtin_types_compatible_p() into an integer
constant expression and works this way:
builtin_types_compatible_p_expr() builds an EXPR_COMPARE expression node
with its two leafs being the two EXPR_TYPE arguments.
Now, at the evaluation of the EXPR_COMPARE expression, the two leafs
should better be flagged constant in order for the EXPR_COMPARE
expression to remain constant.
Remember: before this series, EXPR_COMPARE *removes* constness flags.
After this series, it would *add* them, provided the subexpressions,
i.e. the EXPR_TYPE leafs are flagged constant.
So yes, since one of the goals of this series is to promote expression
constness only in one direction, namely from subexpressions to a parent
expression, this gives the opportunity to clean up that sic-thing: don't
flag the EXPR_TYPE expressions individually, but the parent
__builtin_types_compatible_p() expression.
Nice catch! I'll see where it fits.
>> +
>> +/*
>> + * Set a particular flag among an expression's ones.
>> + *
>> + * The set of flags defined is not completely independent. Take care
>> + * that implications keep obeyed.
>> + *
>> + * Only one single flag from enum expression_flags is allowed for
>> + * the flag argument at a time.
>> + */
>> +static inline void expr_set_flag(unsigned char * const flags,
>> + enum expression_flags flag)
>> +{
>> + /* obey the implications */
>> + switch (flag) {
>> + case EXPR_FLAG_INT_CONST:
>> + case EXPR_FLAG_ENUM_CONST:
>> + case EXPR_FLAG_CHAR_CONST:
>> + flag |= EXPR_FLAG_INT_CONST_EXPR;
>> + /* fallthrough */
>> + case EXPR_FLAG_FP_CONST:
>> + case EXPR_FLAG_INT_CONST_EXPR:
>> + flag |= EXPR_FLAG_ARITH_CONST_EXPR;
>> + /* fallthrough */
>> + case EXPR_FLAG_ARITH_CONST_EXPR:
>> + case EXPR_FLAG_ADDR_CONST_EXPR:
>> + case EXPR_FLAG_NONE:
>> + break;
>> + }
>> +
>> + *flags |= flag;
>> +}
>> +
>> +/*
>> + * Clear a particular flag among an expression's ones.
>> + *
>> + * The set of flags defined is not completely independent. Take care
>> + * that implications keep obeyed.
>> + *
>> + * Only one single flag from enum expression_flags is allowed for
>> + * the flag argument at a time.
>> + */
>> +static inline void expr_clear_flag(unsigned char * const flags,
>> + enum expression_flags flag)
>> +{
>> + /* obey the implications */
>> + switch (flag) {
>> + case EXPR_FLAG_ARITH_CONST_EXPR:
>> + flag |= EXPR_FLAG_INT_CONST_EXPR;
>> + flag |= EXPR_FLAG_FP_CONST;
>> + /* fallthrough */
>> + case EXPR_FLAG_INT_CONST_EXPR:
>> + flag |= EXPR_FLAG_INT_CONST;
>> + flag |= EXPR_FLAG_ENUM_CONST;
>> + flag |= EXPR_FLAG_CHAR_CONST;
>> + /* fallthrough */
>> + case EXPR_FLAG_ADDR_CONST_EXPR:
>> + case EXPR_FLAG_INT_CONST:
>> + case EXPR_FLAG_FP_CONST:
>> + case EXPR_FLAG_ENUM_CONST:
>> + case EXPR_FLAG_CHAR_CONST:
>> + case EXPR_FLAG_NONE:
>> + break;
>> + }
>> +
>> + *flags &= ~flag;
>> +}
>> +
>> +/*
>> + * Remove any "Constant" [6.4.4] flag, but retain the "constant
>> + * expression" [6.6] flags.
>> + * Used to merge the constantness flags of primary subexpressions
>> + * into their parent expressions' ones.
>> + */
>> +static inline void expr_flags_decay_consts(unsigned char * const flags)
>> +{
>> + *flags &= ~(EXPR_FLAG_INT_CONST | EXPR_FLAG_FP_CONST |
>> + EXPR_FLAG_ENUM_CONST | EXPR_FLAG_CHAR_CONST);
>> +}
>
>
> I think it's already much better so!
> But honestly, I still think that it could be improved:
> - the fact that it need a pointer for the update, means that it can only be used
> with a specific type, here unsigned char, and not arbitrary expressions
> - expr_flags_decay_consts(flags) could be simply be replaced by something like:
> #define EXPR_FLAG_CONSTANTS (EXPR_FLAG_INT_CONST|EXPR_FLAG_FP_CONST|...)
> flags &= ~EXPR_FLAG_CONSTANTS;
> - expr_clear_flag() is only used 5 times:
> -) 4 times with ADDR_CONST_EXPR where it can be replaced by a simple &= ~
> -) once with INT_CONST_EXPR
>
> So, with only a few defines for the set/union, one for the 'clear' and one
> one for the decay you can get rid of these helpers wich IMO improve readability
> and is consistent that elsewhere in the code there is anyway manipulations of
> expr->flags done simply with '|' and '&'.
To be honest, I don't know exactly why I'm clinging so much to the
getters/setters. Certainly because they're pretty much self-documenting
while the EXPR_CONSTEXPR_FLAG_INT_CONST #define obviously deserves a
detailed comment. That's not really an argument though while your point
about the pointer to flags requirement is.
So it would be certainly better to stick to your recommendation.
Your enumeration of the actual expr_{set,clear}_flag() usages made me
recognize another point: the current EXPR_FLAG_ARITH_CONST_EXPR is
redundant in that it being set is equivalent to either of
EXPR_FLAG_FP_CONST and EXPR_FLAG_INT_CONST_EXPR being set. Wouldn't it
be better to drop that EXPR_FLAG_ARITH_CONST_EXPR and introduce an
#define EXPR_CONSTEXPR_FLAG_ARITH_CONST_MASK \ (EXPR_FLAG_FP_CONST |
EXPR_FLAG_INT_CONST_EXPR) ?
>> enum {
>> Taint_comma = 1,
>> @@ -77,7 +188,7 @@ enum {
>>
>> struct expression {
>> enum expression_type type:8;
>> - unsigned flags:8;
>> + unsigned char flags;
>
> I suppose that initialy this 'flags' was foreseen for other things than
> 'Int_const_expr' & 'Float_literal' but now I think it should be better
> to rename it with something more explicit, like 'constness' or something
> (same then for the EXPR_FLAG_... of course).
Oh thank you! Actually I've thought about this since the beginning. Only
I haven't been brave enough...
Will be part of the next iteration of this patch [00/13].
>
>
> Luc
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 01/13] expression: introduce additional expression constness tracking flags
2016-01-26 15:26 ` Nicolai Stange
@ 2016-01-26 15:37 ` Nicolai Stange
0 siblings, 0 replies; 43+ messages in thread
From: Nicolai Stange @ 2016-01-26 15:37 UTC (permalink / raw)
To: Nicolai Stange
Cc: Luc Van Oostenryck, linux-sparse, Christopher Li, Josh Triplett
Nicolai Stange <nicstange@gmail.com> writes:
> Luc Van Oostenryck <luc.vanoostenryck@gmail.com> writes:
>
>> On Mon, Jan 25, 2016 at 03:49:28PM +0100, Nicolai Stange wrote:
>>> Prepare for a more fine-grained tracking of expression constness in the
>>> sense of C99 [6.4.4, 6.6].
>>
>> This part could be moved at the end of the patch description
>
> Yes.
>
>>> User-visible behaviour remains unchanged.
>>>
>>> The current implementation tags an expression with either combination
>>> of the flags Int_const_expr and Float_literal, the latter being only
>>> used to tell that
>>> (int).0
>>> is indeed an integer constant expression.
>>
>> This part should be dropped, I think, and maybe moved to the cover letter.
>>
>
> I included this description of the current state of the art in order to
> make the problem addressed by this patch very clear.
>
> Maybe you're right and my problem statement is too much blah blah,
> i.e. the paragraph immediately following would suffice.
>
>>> Even if sparse attempted to verify that initializers for static storage
>>> duration objects are constant expressions [6.7.8(4)] (which it
>>> currently does not), it could not tell reliably.
>>> Examples:
>>> 1.)
>>> static float a = { (float)0 }; /* allowed by C99 */
>>> '(float)0' is not an integer constant expression, but an arithmetic
>>> one.
>>> 2.)
>>> enum { b = 0 };
>>> static void *c = { (void*)b }; /* disallowed by C99 */
>>> References to enum members are not allowed in address constants
>>> [6.6(9)] and thus, the initializer is not a constant expression at
>>> all.
>>
>> This could be moved to the top of the patch description
>
> Yes.
>
>>> Introduce a broader set of constness tracking flags, resembling the
>>> four types of constants [6.4.4] (integer, floating, enumeration,
>>> character) and the three types of constant expressions [6.6] (integer,
>>> arithmetic, address). Use helper functions to consistently set and
>>> clear these flags as they are not completely independent.
>>> Finally, make alloc_expression() and alloc_const_expression()
>>> initialize the new expression's flags to zero.
>>
>> I think this patch should be split into 2-4 smaller patches:
>> 0) introduce the new flags, maybe first only NONE, INT_EXPR & FP_CONST
>> 1) replace the old use of 0, Int_const_expr, ... (can be folded with 0)
>> 2) initialize the flag in alloc_expression() (can be folded with 1)..
>> 3) introduce the the new flags if needed, the expr_..._flag() helpers
>> and begin them
>
> I'll drop the expr_..._flag() helpers as recommended by you (see below)
> and introduce the additional flags (arithm. constness, addr constants,
> ?) as needed in later patches.
>
> Thus, no split of this patch since 3.) would be addressed by deferring
> the introduction of additional flags and 1)-3) "can be folded" anyway.
>
>>
>>> --- a/expression.c
>>> +++ b/expression.c
>>> @@ -131,7 +131,7 @@ static struct token *parse_type(struct token *token, struct expression **tree)
>>> {
>>> struct symbol *sym;
>>> *tree = alloc_expression(token->pos, EXPR_TYPE);
>>> - (*tree)->flags = Int_const_expr; /* sic */
>>> + expr_set_flag(&(*tree)->flags, EXPR_FLAG_INT_CONST_EXPR); /* sic */
>>
>> Better to remove those 'sic'. To which text do they refer?
>>
>>> @@ -457,7 +459,8 @@ struct token *primary_expression(struct token *token, struct expression **tree)
>>> }
>>> if (token->special == '[' && lookup_type(token->next)) {
>>> expr = alloc_expression(token->pos, EXPR_TYPE);
>>> - expr->flags = Int_const_expr; /* sic */
>>> + /* sic */
>>> + expr_set_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
>>
>> Ditto for the 'sic'
>
> That 'sic' had been introduced in
> a24a3adf3f0e ("fix handling of integer constant expressions")
>
> Quote from the commit message:
>
> EXPR_TYPE also gets marked int_const_expr (to make it DTRT on the
> builtin_same_type_p() et.al.)
>
> Quote end.
>
> It is a hack to turn __builtin_types_compatible_p() into an integer
> constant expression and works this way:
> builtin_types_compatible_p_expr() builds an EXPR_COMPARE expression node
> with its two leafs being the two EXPR_TYPE arguments.
>
> Now, at the evaluation of the EXPR_COMPARE expression, the two leafs
> should better be flagged constant in order for the EXPR_COMPARE
> expression to remain constant.
> Remember: before this series, EXPR_COMPARE *removes* constness flags.
> After this series, it would *add* them, provided the subexpressions,
> i.e. the EXPR_TYPE leafs are flagged constant.
>
> So yes, since one of the goals of this series is to promote expression
> constness only in one direction, namely from subexpressions to a parent
> expression, this gives the opportunity to clean up that sic-thing: don't
> flag the EXPR_TYPE expressions individually, but the parent
> __builtin_types_compatible_p() expression.
>
> Nice catch! I'll see where it fits.
>
>
>>> +
>>> +/*
>>> + * Set a particular flag among an expression's ones.
>>> + *
>>> + * The set of flags defined is not completely independent. Take care
>>> + * that implications keep obeyed.
>>> + *
>>> + * Only one single flag from enum expression_flags is allowed for
>>> + * the flag argument at a time.
>>> + */
>>> +static inline void expr_set_flag(unsigned char * const flags,
>>> + enum expression_flags flag)
>>> +{
>>> + /* obey the implications */
>>> + switch (flag) {
>>> + case EXPR_FLAG_INT_CONST:
>>> + case EXPR_FLAG_ENUM_CONST:
>>> + case EXPR_FLAG_CHAR_CONST:
>>> + flag |= EXPR_FLAG_INT_CONST_EXPR;
>>> + /* fallthrough */
>>> + case EXPR_FLAG_FP_CONST:
>>> + case EXPR_FLAG_INT_CONST_EXPR:
>>> + flag |= EXPR_FLAG_ARITH_CONST_EXPR;
>>> + /* fallthrough */
>>> + case EXPR_FLAG_ARITH_CONST_EXPR:
>>> + case EXPR_FLAG_ADDR_CONST_EXPR:
>>> + case EXPR_FLAG_NONE:
>>> + break;
>>> + }
>>> +
>>> + *flags |= flag;
>>> +}
>>> +
>>> +/*
>>> + * Clear a particular flag among an expression's ones.
>>> + *
>>> + * The set of flags defined is not completely independent. Take care
>>> + * that implications keep obeyed.
>>> + *
>>> + * Only one single flag from enum expression_flags is allowed for
>>> + * the flag argument at a time.
>>> + */
>>> +static inline void expr_clear_flag(unsigned char * const flags,
>>> + enum expression_flags flag)
>>> +{
>>> + /* obey the implications */
>>> + switch (flag) {
>>> + case EXPR_FLAG_ARITH_CONST_EXPR:
>>> + flag |= EXPR_FLAG_INT_CONST_EXPR;
>>> + flag |= EXPR_FLAG_FP_CONST;
>>> + /* fallthrough */
>>> + case EXPR_FLAG_INT_CONST_EXPR:
>>> + flag |= EXPR_FLAG_INT_CONST;
>>> + flag |= EXPR_FLAG_ENUM_CONST;
>>> + flag |= EXPR_FLAG_CHAR_CONST;
>>> + /* fallthrough */
>>> + case EXPR_FLAG_ADDR_CONST_EXPR:
>>> + case EXPR_FLAG_INT_CONST:
>>> + case EXPR_FLAG_FP_CONST:
>>> + case EXPR_FLAG_ENUM_CONST:
>>> + case EXPR_FLAG_CHAR_CONST:
>>> + case EXPR_FLAG_NONE:
>>> + break;
>>> + }
>>> +
>>> + *flags &= ~flag;
>>> +}
>>> +
>>> +/*
>>> + * Remove any "Constant" [6.4.4] flag, but retain the "constant
>>> + * expression" [6.6] flags.
>>> + * Used to merge the constantness flags of primary subexpressions
>>> + * into their parent expressions' ones.
>>> + */
>>> +static inline void expr_flags_decay_consts(unsigned char * const flags)
>>> +{
>>> + *flags &= ~(EXPR_FLAG_INT_CONST | EXPR_FLAG_FP_CONST |
>>> + EXPR_FLAG_ENUM_CONST | EXPR_FLAG_CHAR_CONST);
>>> +}
>>
>>
>> I think it's already much better so!
>> But honestly, I still think that it could be improved:
>> - the fact that it need a pointer for the update, means that it can only be used
>> with a specific type, here unsigned char, and not arbitrary expressions
>> - expr_flags_decay_consts(flags) could be simply be replaced by something like:
>> #define EXPR_FLAG_CONSTANTS (EXPR_FLAG_INT_CONST|EXPR_FLAG_FP_CONST|...)
>> flags &= ~EXPR_FLAG_CONSTANTS;
>> - expr_clear_flag() is only used 5 times:
>> -) 4 times with ADDR_CONST_EXPR where it can be replaced by a simple &= ~
>> -) once with INT_CONST_EXPR
>>
>> So, with only a few defines for the set/union, one for the 'clear' and one
>> one for the decay you can get rid of these helpers wich IMO improve readability
>> and is consistent that elsewhere in the code there is anyway manipulations of
>> expr->flags done simply with '|' and '&'.
>
> To be honest, I don't know exactly why I'm clinging so much to the
> getters/setters. Certainly because they're pretty much self-documenting
> while the EXPR_CONSTEXPR_FLAG_INT_CONST #define obviously deserves a
> detailed comment. That's not really an argument though while your point
> about the pointer to flags requirement is.
>
> So it would be certainly better to stick to your recommendation.
>
> Your enumeration of the actual expr_{set,clear}_flag() usages made me
> recognize another point: the current EXPR_FLAG_ARITH_CONST_EXPR is
> redundant in that it being set is equivalent to either of
> EXPR_FLAG_FP_CONST and EXPR_FLAG_INT_CONST_EXPR being set. Wouldn't it
> be better to drop that EXPR_FLAG_ARITH_CONST_EXPR and introduce an
> #define EXPR_CONSTEXPR_FLAG_ARITH_CONST_MASK \ (EXPR_FLAG_FP_CONST |
> EXPR_FLAG_INT_CONST_EXPR) ?
Sorry, complete nonsense, forget about that last paragraph.
Counterexample: 0. + 0.
Arithmetic constant expression, but neither a fp literal nor an integer
constant expression.
>
>>> enum {
>>> Taint_comma = 1,
>>> @@ -77,7 +188,7 @@ enum {
>>>
>>> struct expression {
>>> enum expression_type type:8;
>>> - unsigned flags:8;
>>> + unsigned char flags;
>>
>> I suppose that initialy this 'flags' was foreseen for other things than
>> 'Int_const_expr' & 'Float_literal' but now I think it should be better
>> to rename it with something more explicit, like 'constness' or something
>> (same then for the EXPR_FLAG_... of course).
>
> Oh thank you! Actually I've thought about this since the beginning. Only
> I haven't been brave enough...
>
> Will be part of the next iteration of this patch [00/13].
>
>>
>>
>> Luc
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 02/13] expression: examine constness of casts at evaluation only
2016-01-25 14:47 [PATCH v2 00/13] improve constexpr handling Nicolai Stange
2016-01-25 14:49 ` [PATCH v2 01/13] expression: introduce additional expression constness tracking flags Nicolai Stange
@ 2016-01-25 14:51 ` Nicolai Stange
2016-01-25 22:02 ` Luc Van Oostenryck
2016-01-25 14:52 ` [PATCH v2 03/13] expression: examine constness of binops and alike " Nicolai Stange
` (11 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Nicolai Stange @ 2016-01-25 14:51 UTC (permalink / raw)
To: linux-sparse
Cc: Nicolai Stange, Christopher Li, Josh Triplett, Luc Van Oostenryck
Currently, the propagation of expressions' constness flags through
cast expressions is done in two steps:
- Several flags are speculatively set at cast expression parsing time
- and possibly cleared again at evaluation time.
Set aside this unfortunate split of code, the early propagation of
constness flags is not able to recognize constant expressions such as
(int)__builtin_choose_expr(0, 0, 0)
since the final expression to be thrown into the cast is known only
after evaluation.
Move the whole calculation of cast expressions' constness flags to the
evaluation phase.
Introduce support for tracking arithmetic constness propagation through
cast expressions.
Introduce support for recognizing address constants created by casting
an integer constant to pointer type.
Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
evaluate.c | 42 ++++++++++++++++++++++++++++++------------
expression.c | 19 -------------------
validation/constexpr-cast.c | 25 +++++++++++++++++++++++++
3 files changed, 55 insertions(+), 31 deletions(-)
create mode 100644 validation/constexpr-cast.c
diff --git a/evaluate.c b/evaluate.c
index 0cf85ae..11917ee 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -323,7 +323,6 @@ static struct expression * cast_to(struct expression *old, struct symbol *type)
}
expr = alloc_expression(old->pos, EXPR_IMPLIED_CAST);
- expr->flags = old->flags;
expr->ctype = type;
expr->cast_type = type;
expr->cast_expression = old;
@@ -2742,17 +2741,36 @@ static struct symbol *evaluate_cast(struct expression *expr)
class1 = classify_type(ctype, &t1);
- /* cast to non-numeric type -> not an arithmetic expression */
- if (!(class1 & TYPE_NUM))
- expr_clear_flag(&expr->flags, EXPR_FLAG_ARITH_CONST_EXPR);
- /* cast to float type -> not an integer constant expression */
- else if (class1 & TYPE_FLOAT)
- expr_clear_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
- /* if argument turns out to be not an integer constant expression *and*
- it was not a floating literal to start with -> too bad */
- else if (!(target->flags &
- (EXPR_FLAG_INT_CONST_EXPR | EXPR_FLAG_FP_CONST)))
- expr_clear_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
+ if (!(class1 & TYPE_NUM)) {
+ /*
+ * Casts of integer literals to pointer type yield
+ * address constants [6.6(9)].
+ */
+ if (class1 & TYPE_PTR &&
+ (target->flags & EXPR_FLAG_INT_CONST)) {
+ expr_set_flag(&expr->flags, EXPR_FLAG_ADDR_CONST_EXPR);
+ }
+ } else {
+ expr->flags = target->flags;
+ expr_flags_decay_consts(&expr->flags);
+ /*
+ * Casts to numeric types never result in address
+ * constants [6.6(9)].
+ */
+ expr_clear_flag(&expr->flags, EXPR_FLAG_ADDR_CONST_EXPR);
+ /*
+ * Cast to float type -> not an integer constant
+ * expression [6.6(6)].
+ */
+ if (class1 & TYPE_FLOAT)
+ expr_clear_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
+ /*
+ * Casts of float literals to integer type results in
+ * a constant integer expression [6.6(6)].
+ */
+ else if (target->flags & EXPR_FLAG_FP_CONST)
+ expr_set_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
+ }
/*
* You can always throw a value away by casting to
diff --git a/expression.c b/expression.c
index eccdb5a..33f4581 100644
--- a/expression.c
+++ b/expression.c
@@ -725,25 +725,6 @@ static struct token *cast_expression(struct token *token, struct expression **tr
if (!v)
return token;
cast->cast_expression = v;
-
- cast->flags = v->flags;
- expr_flags_decay_consts(&cast->flags);
- /*
- * Up to now, we missed the (int).0 case here
- * which should really get a
- * EXPR_FLAG_INT_CONST_EXPR marker. Also,
- * conversion to non-numeric types is not
- * properly reflected up to this point.
- * However, we do not know until evaluation.
- * For the moment, in order to preserve
- * semantics, speculatively set
- * EXPR_FLAG_INT_CONST_EXPR if
- * EXPR_FLAG_FP_CONST is set. evaluate_cast()
- * will unset inappropriate flags again after
- * examining type information.
- */
- if (v->flags & EXPR_FLAG_FP_CONST)
- cast->flags |= EXPR_FLAG_INT_CONST_EXPR;
return token;
}
}
diff --git a/validation/constexpr-cast.c b/validation/constexpr-cast.c
new file mode 100644
index 0000000..2706961
--- /dev/null
+++ b/validation/constexpr-cast.c
@@ -0,0 +1,25 @@
+static int a[] = {
+ [(int)0] = 0, // OK
+ [(int)(int)0] = 0, // OK
+ [(int)0.] = 0, // OK
+ [(int)(int)0.] = 0, // OK
+ [(int)__builtin_choose_expr(0, 0, 0)] = 0, // OK
+ [(int)__builtin_choose_expr(0, 0, 0.)] = 0, // OK
+
+ [(int)(float)0] = 0, // KO
+ [(int)(float)0.] = 0, // KO
+
+ [(int)(void*)0] = 0, // KO
+ [(int)(void*)0.] = 0, // KO
+
+};
+/*
+ * check-name: Expression constness propagation in casts
+ *
+ * check-error-start
+constexpr-cast.c:9:11: error: bad integer constant expression
+constexpr-cast.c:10:11: error: bad integer constant expression
+constexpr-cast.c:12:11: error: bad integer constant expression
+constexpr-cast.c:13:11: error: bad integer constant expression
+ * check-error-end
+ */
--
2.7.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 02/13] expression: examine constness of casts at evaluation only
2016-01-25 14:51 ` [PATCH v2 02/13] expression: examine constness of casts at evaluation only Nicolai Stange
@ 2016-01-25 22:02 ` Luc Van Oostenryck
2016-01-26 16:11 ` Nicolai Stange
0 siblings, 1 reply; 43+ messages in thread
From: Luc Van Oostenryck @ 2016-01-25 22:02 UTC (permalink / raw)
To: Nicolai Stange; +Cc: linux-sparse, Christopher Li, Josh Triplett
On Mon, Jan 25, 2016 at 03:51:03PM +0100, Nicolai Stange wrote:
> Currently, the propagation of expressions' constness flags through
> cast expressions is done in two steps:
> - Several flags are speculatively set at cast expression parsing time
> - and possibly cleared again at evaluation time.
>
> Set aside this unfortunate split of code, the early propagation of
> constness flags is not able to recognize constant expressions such as
> (int)__builtin_choose_expr(0, 0, 0)
> since the final expression to be thrown into the cast is known only
> after evaluation.
>
> Move the whole calculation of cast expressions' constness flags to the
> evaluation phase.
>
> Introduce support for tracking arithmetic constness propagation through
> cast expressions.
>
> Introduce support for recognizing address constants created by casting
> an integer constant to pointer type.
The description show that 3 things are done in the patch.
Can it be splitted in 3?
Also, it describes the current situation and what is changed but the
'why' part is not so clear.
The changes themselves are very fine.
And the comments help :)
Luc
> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
> ---
> evaluate.c | 42 ++++++++++++++++++++++++++++++------------
> expression.c | 19 -------------------
> validation/constexpr-cast.c | 25 +++++++++++++++++++++++++
> 3 files changed, 55 insertions(+), 31 deletions(-)
> create mode 100644 validation/constexpr-cast.c
>
> diff --git a/evaluate.c b/evaluate.c
> index 0cf85ae..11917ee 100644
> --- a/evaluate.c
> +++ b/evaluate.c
> @@ -323,7 +323,6 @@ static struct expression * cast_to(struct expression *old, struct symbol *type)
> }
>
> expr = alloc_expression(old->pos, EXPR_IMPLIED_CAST);
> - expr->flags = old->flags;
> expr->ctype = type;
> expr->cast_type = type;
> expr->cast_expression = old;
> @@ -2742,17 +2741,36 @@ static struct symbol *evaluate_cast(struct expression *expr)
>
> class1 = classify_type(ctype, &t1);
>
> - /* cast to non-numeric type -> not an arithmetic expression */
> - if (!(class1 & TYPE_NUM))
> - expr_clear_flag(&expr->flags, EXPR_FLAG_ARITH_CONST_EXPR);
> - /* cast to float type -> not an integer constant expression */
> - else if (class1 & TYPE_FLOAT)
> - expr_clear_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
> - /* if argument turns out to be not an integer constant expression *and*
> - it was not a floating literal to start with -> too bad */
> - else if (!(target->flags &
> - (EXPR_FLAG_INT_CONST_EXPR | EXPR_FLAG_FP_CONST)))
> - expr_clear_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
> + if (!(class1 & TYPE_NUM)) {
> + /*
> + * Casts of integer literals to pointer type yield
> + * address constants [6.6(9)].
> + */
> + if (class1 & TYPE_PTR &&
> + (target->flags & EXPR_FLAG_INT_CONST)) {
> + expr_set_flag(&expr->flags, EXPR_FLAG_ADDR_CONST_EXPR);
> + }
> + } else {
> + expr->flags = target->flags;
> + expr_flags_decay_consts(&expr->flags);
> + /*
> + * Casts to numeric types never result in address
> + * constants [6.6(9)].
> + */
> + expr_clear_flag(&expr->flags, EXPR_FLAG_ADDR_CONST_EXPR);
> + /*
> + * Cast to float type -> not an integer constant
> + * expression [6.6(6)].
> + */
> + if (class1 & TYPE_FLOAT)
> + expr_clear_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
> + /*
> + * Casts of float literals to integer type results in
> + * a constant integer expression [6.6(6)].
> + */
> + else if (target->flags & EXPR_FLAG_FP_CONST)
> + expr_set_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
> + }
>
> /*
> * You can always throw a value away by casting to
> diff --git a/expression.c b/expression.c
> index eccdb5a..33f4581 100644
> --- a/expression.c
> +++ b/expression.c
> @@ -725,25 +725,6 @@ static struct token *cast_expression(struct token *token, struct expression **tr
> if (!v)
> return token;
> cast->cast_expression = v;
> -
> - cast->flags = v->flags;
> - expr_flags_decay_consts(&cast->flags);
> - /*
> - * Up to now, we missed the (int).0 case here
> - * which should really get a
> - * EXPR_FLAG_INT_CONST_EXPR marker. Also,
> - * conversion to non-numeric types is not
> - * properly reflected up to this point.
> - * However, we do not know until evaluation.
> - * For the moment, in order to preserve
> - * semantics, speculatively set
> - * EXPR_FLAG_INT_CONST_EXPR if
> - * EXPR_FLAG_FP_CONST is set. evaluate_cast()
> - * will unset inappropriate flags again after
> - * examining type information.
> - */
> - if (v->flags & EXPR_FLAG_FP_CONST)
> - cast->flags |= EXPR_FLAG_INT_CONST_EXPR;
> return token;
> }
> }
> diff --git a/validation/constexpr-cast.c b/validation/constexpr-cast.c
> new file mode 100644
> index 0000000..2706961
> --- /dev/null
> +++ b/validation/constexpr-cast.c
> @@ -0,0 +1,25 @@
> +static int a[] = {
> + [(int)0] = 0, // OK
> + [(int)(int)0] = 0, // OK
> + [(int)0.] = 0, // OK
> + [(int)(int)0.] = 0, // OK
> + [(int)__builtin_choose_expr(0, 0, 0)] = 0, // OK
> + [(int)__builtin_choose_expr(0, 0, 0.)] = 0, // OK
> +
> + [(int)(float)0] = 0, // KO
> + [(int)(float)0.] = 0, // KO
> +
> + [(int)(void*)0] = 0, // KO
> + [(int)(void*)0.] = 0, // KO
> +
> +};
> +/*
> + * check-name: Expression constness propagation in casts
> + *
> + * check-error-start
> +constexpr-cast.c:9:11: error: bad integer constant expression
> +constexpr-cast.c:10:11: error: bad integer constant expression
> +constexpr-cast.c:12:11: error: bad integer constant expression
> +constexpr-cast.c:13:11: error: bad integer constant expression
> + * check-error-end
> + */
> --
> 2.7.0
>
> --
> 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
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 02/13] expression: examine constness of casts at evaluation only
2016-01-25 22:02 ` Luc Van Oostenryck
@ 2016-01-26 16:11 ` Nicolai Stange
0 siblings, 0 replies; 43+ messages in thread
From: Nicolai Stange @ 2016-01-26 16:11 UTC (permalink / raw)
To: Luc Van Oostenryck
Cc: Nicolai Stange, linux-sparse, Christopher Li, Josh Triplett
Luc Van Oostenryck <luc.vanoostenryck@gmail.com> writes:
> On Mon, Jan 25, 2016 at 03:51:03PM +0100, Nicolai Stange wrote:
>> Currently, the propagation of expressions' constness flags through
>> cast expressions is done in two steps:
>> - Several flags are speculatively set at cast expression parsing time
>> - and possibly cleared again at evaluation time.
>>
>> Set aside this unfortunate split of code, the early propagation of
>> constness flags is not able to recognize constant expressions such as
>> (int)__builtin_choose_expr(0, 0, 0)
>> since the final expression to be thrown into the cast is known only
>> after evaluation.
>>
>> Move the whole calculation of cast expressions' constness flags to the
>> evaluation phase.
>>
>> Introduce support for tracking arithmetic constness propagation through
>> cast expressions.
>>
>> Introduce support for recognizing address constants created by casting
>> an integer constant to pointer type.
>
>
> The description show that 3 things are done in the patch.
> Can it be splitted in 3?
That tracking "arithmetic constness propagation" is a stupid assertion anyway
since it is an implication of the change listed before ("calculate
constness at evaluation"). I'll drop that or rewrite it.
And yes, the third thing should be a separate patch.
>
> Also, it describes the current situation and what is changed but the
> 'why' part is not so clear.
>
Well, because I want (int)__builtin_choose_expr(0, 0, 0) to being
recognized as being a constant expression?
> The changes themselves are very fine.
> And the comments help :)
>
> Luc
>
>> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
>> ---
>> evaluate.c | 42 ++++++++++++++++++++++++++++++------------
>> expression.c | 19 -------------------
>> validation/constexpr-cast.c | 25 +++++++++++++++++++++++++
>> 3 files changed, 55 insertions(+), 31 deletions(-)
>> create mode 100644 validation/constexpr-cast.c
>>
>> diff --git a/evaluate.c b/evaluate.c
>> index 0cf85ae..11917ee 100644
>> --- a/evaluate.c
>> +++ b/evaluate.c
>> @@ -323,7 +323,6 @@ static struct expression * cast_to(struct expression *old, struct symbol *type)
>> }
>>
>> expr = alloc_expression(old->pos, EXPR_IMPLIED_CAST);
>> - expr->flags = old->flags;
>> expr->ctype = type;
>> expr->cast_type = type;
>> expr->cast_expression = old;
>> @@ -2742,17 +2741,36 @@ static struct symbol *evaluate_cast(struct expression *expr)
>>
>> class1 = classify_type(ctype, &t1);
>>
>> - /* cast to non-numeric type -> not an arithmetic expression */
>> - if (!(class1 & TYPE_NUM))
>> - expr_clear_flag(&expr->flags, EXPR_FLAG_ARITH_CONST_EXPR);
>> - /* cast to float type -> not an integer constant expression */
>> - else if (class1 & TYPE_FLOAT)
>> - expr_clear_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
>> - /* if argument turns out to be not an integer constant expression *and*
>> - it was not a floating literal to start with -> too bad */
>> - else if (!(target->flags &
>> - (EXPR_FLAG_INT_CONST_EXPR | EXPR_FLAG_FP_CONST)))
>> - expr_clear_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
>> + if (!(class1 & TYPE_NUM)) {
>> + /*
>> + * Casts of integer literals to pointer type yield
>> + * address constants [6.6(9)].
>> + */
>> + if (class1 & TYPE_PTR &&
>> + (target->flags & EXPR_FLAG_INT_CONST)) {
>> + expr_set_flag(&expr->flags, EXPR_FLAG_ADDR_CONST_EXPR);
>> + }
>> + } else {
>> + expr->flags = target->flags;
>> + expr_flags_decay_consts(&expr->flags);
>> + /*
>> + * Casts to numeric types never result in address
>> + * constants [6.6(9)].
>> + */
>> + expr_clear_flag(&expr->flags, EXPR_FLAG_ADDR_CONST_EXPR);
>> + /*
>> + * Cast to float type -> not an integer constant
>> + * expression [6.6(6)].
>> + */
>> + if (class1 & TYPE_FLOAT)
>> + expr_clear_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
>> + /*
>> + * Casts of float literals to integer type results in
>> + * a constant integer expression [6.6(6)].
>> + */
>> + else if (target->flags & EXPR_FLAG_FP_CONST)
>> + expr_set_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
>> + }
>>
>> /*
>> * You can always throw a value away by casting to
>> diff --git a/expression.c b/expression.c
>> index eccdb5a..33f4581 100644
>> --- a/expression.c
>> +++ b/expression.c
>> @@ -725,25 +725,6 @@ static struct token *cast_expression(struct token *token, struct expression **tr
>> if (!v)
>> return token;
>> cast->cast_expression = v;
>> -
>> - cast->flags = v->flags;
>> - expr_flags_decay_consts(&cast->flags);
>> - /*
>> - * Up to now, we missed the (int).0 case here
>> - * which should really get a
>> - * EXPR_FLAG_INT_CONST_EXPR marker. Also,
>> - * conversion to non-numeric types is not
>> - * properly reflected up to this point.
>> - * However, we do not know until evaluation.
>> - * For the moment, in order to preserve
>> - * semantics, speculatively set
>> - * EXPR_FLAG_INT_CONST_EXPR if
>> - * EXPR_FLAG_FP_CONST is set. evaluate_cast()
>> - * will unset inappropriate flags again after
>> - * examining type information.
>> - */
>> - if (v->flags & EXPR_FLAG_FP_CONST)
>> - cast->flags |= EXPR_FLAG_INT_CONST_EXPR;
>> return token;
>> }
>> }
>> diff --git a/validation/constexpr-cast.c b/validation/constexpr-cast.c
>> new file mode 100644
>> index 0000000..2706961
>> --- /dev/null
>> +++ b/validation/constexpr-cast.c
>> @@ -0,0 +1,25 @@
>> +static int a[] = {
>> + [(int)0] = 0, // OK
>> + [(int)(int)0] = 0, // OK
>> + [(int)0.] = 0, // OK
>> + [(int)(int)0.] = 0, // OK
>> + [(int)__builtin_choose_expr(0, 0, 0)] = 0, // OK
>> + [(int)__builtin_choose_expr(0, 0, 0.)] = 0, // OK
>> +
>> + [(int)(float)0] = 0, // KO
>> + [(int)(float)0.] = 0, // KO
>> +
>> + [(int)(void*)0] = 0, // KO
>> + [(int)(void*)0.] = 0, // KO
>> +
>> +};
>> +/*
>> + * check-name: Expression constness propagation in casts
>> + *
>> + * check-error-start
>> +constexpr-cast.c:9:11: error: bad integer constant expression
>> +constexpr-cast.c:10:11: error: bad integer constant expression
>> +constexpr-cast.c:12:11: error: bad integer constant expression
>> +constexpr-cast.c:13:11: error: bad integer constant expression
>> + * check-error-end
>> + */
>> --
>> 2.7.0
>>
>> --
>> 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
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 03/13] expression: examine constness of binops and alike at evaluation only
2016-01-25 14:47 [PATCH v2 00/13] improve constexpr handling Nicolai Stange
2016-01-25 14:49 ` [PATCH v2 01/13] expression: introduce additional expression constness tracking flags Nicolai Stange
2016-01-25 14:51 ` [PATCH v2 02/13] expression: examine constness of casts at evaluation only Nicolai Stange
@ 2016-01-25 14:52 ` Nicolai Stange
2016-01-26 0:14 ` Luc Van Oostenryck
2016-01-26 0:59 ` Luc Van Oostenryck
2016-01-25 14:53 ` [PATCH v2 04/13] expression: examine constness of preops " Nicolai Stange
` (10 subsequent siblings)
13 siblings, 2 replies; 43+ messages in thread
From: Nicolai Stange @ 2016-01-25 14:52 UTC (permalink / raw)
To: linux-sparse
Cc: Nicolai Stange, Christopher Li, Josh Triplett, Luc Van Oostenryck
Currently, the propagation of expressions' constness flags through
binary operations, compare and logical expressions is done in two
steps:
- Several flags are speculatively set at expression parsing time
- and possibly cleared again at evaluation time.
Set aside this unfortunate split of code, the early propagation of
constness flags is not able to recognize constant expressions such as
0 + __builtin_choose_expr(0, 0, 0)
0 < __builtin_choose_expr(0, 0, 0)
0 && __builtin_choose_expr(0, 0, 0)
since the final expression to be thrown into the binop-like expression
is known only after evaluation.
Move the whole calculation of binary operations', compare and logical
expressions' constness flags to the evaluation phase.
Introduce support for tracking arithmetic constness propagation through
binop-like expressions.
Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
evaluate.c | 25 +++++++++----------------
expression.c | 3 ---
validation/constexpr-binop.c | 33 +++++++++++++++++++++++++++++++++
3 files changed, 42 insertions(+), 19 deletions(-)
create mode 100644 validation/constexpr-binop.c
diff --git a/evaluate.c b/evaluate.c
index 11917ee..5e3e2ca 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -878,11 +878,9 @@ static struct symbol *evaluate_logical(struct expression *expr)
/* the result is int [6.5.13(3), 6.5.14(3)] */
expr->ctype = &int_ctype;
- if (expr->flags) {
- if (!(expr->left->flags & expr->right->flags &
- EXPR_FLAG_INT_CONST_EXPR))
- expr->flags = EXPR_FLAG_NONE;
- }
+ expr->flags = expr->left->flags & expr->right->flags;
+ expr_flags_decay_consts(&expr->flags);
+ expr_clear_flag(&expr->flags, EXPR_FLAG_ADDR_CONST_EXPR);
return &int_ctype;
}
@@ -893,14 +891,11 @@ static struct symbol *evaluate_binop(struct expression *expr)
int rclass = classify_type(expr->right->ctype, &rtype);
int op = expr->op;
- if (expr->flags) {
- if (!(expr->left->flags & expr->right->flags &
- EXPR_FLAG_INT_CONST_EXPR))
- expr->flags = EXPR_FLAG_NONE;
- }
-
/* number op number */
if (lclass & rclass & TYPE_NUM) {
+ expr->flags = expr->left->flags & expr->right->flags;
+ expr_flags_decay_consts(&expr->flags);
+
if ((lclass | rclass) & TYPE_FLOAT) {
switch (op) {
case '+': case '-': case '*': case '/':
@@ -1001,11 +996,9 @@ static struct symbol *evaluate_compare(struct expression *expr)
struct symbol *ctype;
const char *typediff;
- if (expr->flags) {
- if (!(expr->left->flags & expr->right->flags &
- EXPR_FLAG_INT_CONST_EXPR))
- expr->flags = EXPR_FLAG_NONE;
- }
+ expr->flags = left->flags & right->flags;
+ expr_flags_decay_consts(&expr->flags);
+ expr_clear_flag(&expr->flags, EXPR_FLAG_ADDR_CONST_EXPR);
/* Type types? */
if (is_type_type(ltype) && is_type_type(rtype))
diff --git a/expression.c b/expression.c
index 33f4581..e49a19d 100644
--- a/expression.c
+++ b/expression.c
@@ -146,7 +146,6 @@ static struct token *builtin_types_compatible_p_expr(struct token *token,
{
struct expression *expr = alloc_expression(
token->pos, EXPR_COMPARE);
- expr_set_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
expr->op = SPECIAL_EQUAL;
token = token->next;
if (!match_op(token, '('))
@@ -761,8 +760,6 @@ static struct token *cast_expression(struct token *token, struct expression **tr
sparse_error(next->pos, "No right hand side of '%s'-expression", show_special(op)); \
break; \
} \
- top->flags = left->flags & right->flags; \
- expr_flags_decay_consts(&top->flags); \
top->op = op; \
top->left = left; \
top->right = right; \
diff --git a/validation/constexpr-binop.c b/validation/constexpr-binop.c
new file mode 100644
index 0000000..85a88e3
--- /dev/null
+++ b/validation/constexpr-binop.c
@@ -0,0 +1,33 @@
+static int a[] = {
+ [0 + 0] = 0, // OK
+ [0 + 0.] = 0, // KO
+ [(void*)0 + 0] = 0, // KO
+ [0 + __builtin_choose_expr(0, 0, 0)] = 0, // OK
+ [0 + __builtin_choose_expr(0, 0., 0)] = 0, // OK
+ [0 + __builtin_choose_expr(0, 0, 0.)] = 0, // KO
+ [0 < 0] = 0, // OK
+ [0 < 0.] = 0, // KO
+ [0 < __builtin_choose_expr(0, 0, 0)] = 0, // OK
+ [0 < __builtin_choose_expr(0, 0., 0)] = 0, // OK
+ [0 < __builtin_choose_expr(0, 0, 0.)] = 0, // KO
+ [0 && 0] = 0, // OK
+ [0 && 0.] = 0, // KO
+ [0 && __builtin_choose_expr(0, 0, 0)] = 0, // OK
+ [0 && __builtin_choose_expr(0, 0., 0)] = 0, // OK
+ [0 && __builtin_choose_expr(0, 0, 0.)] = 0, // KO
+ [0 + __builtin_types_compatible_p(int, float)] = 0, // OK
+};
+
+/*
+ * check-name: Expression constness propagation in binops and alike
+ *
+ * check-error-start
+constexpr-binop.c:3:12: error: bad constant expression
+constexpr-binop.c:4:19: error: bad integer constant expression
+constexpr-binop.c:7:12: error: bad constant expression
+constexpr-binop.c:9:12: error: bad integer constant expression
+constexpr-binop.c:12:12: error: bad integer constant expression
+constexpr-binop.c:14:12: error: bad integer constant expression
+constexpr-binop.c:17:12: error: bad integer constant expression
+ * check-error-end
+ */
--
2.7.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 03/13] expression: examine constness of binops and alike at evaluation only
2016-01-25 14:52 ` [PATCH v2 03/13] expression: examine constness of binops and alike " Nicolai Stange
@ 2016-01-26 0:14 ` Luc Van Oostenryck
2016-01-26 15:50 ` Nicolai Stange
2016-01-26 0:59 ` Luc Van Oostenryck
1 sibling, 1 reply; 43+ messages in thread
From: Luc Van Oostenryck @ 2016-01-26 0:14 UTC (permalink / raw)
To: Nicolai Stange; +Cc: linux-sparse, Christopher Li, Josh Triplett
On Mon, Jan 25, 2016 at 03:52:14PM +0100, Nicolai Stange wrote:
> Currently, the propagation of expressions' constness flags through
> binary operations, compare and logical expressions is done in two
> steps:
> - Several flags are speculatively set at expression parsing time
> - and possibly cleared again at evaluation time.
>
> Set aside this unfortunate split of code, the early propagation of
> constness flags is not able to recognize constant expressions such as
> 0 + __builtin_choose_expr(0, 0, 0)
> 0 < __builtin_choose_expr(0, 0, 0)
> 0 && __builtin_choose_expr(0, 0, 0)
> since the final expression to be thrown into the binop-like expression
> is known only after evaluation.
>
> Move the whole calculation of binary operations', compare and logical
> expressions' constness flags to the evaluation phase.
>
> Introduce support for tracking arithmetic constness propagation through
> binop-like expressions.
Same as previous patch regarding the description and splitness of the patch.
> @@ -893,14 +891,11 @@ static struct symbol *evaluate_binop(struct expression *expr)
> int rclass = classify_type(expr->right->ctype, &rtype);
> int op = expr->op;
>
> - if (expr->flags) {
> - if (!(expr->left->flags & expr->right->flags &
> - EXPR_FLAG_INT_CONST_EXPR))
> - expr->flags = EXPR_FLAG_NONE;
> - }
> -
> /* number op number */
> if (lclass & rclass & TYPE_NUM) {
> + expr->flags = expr->left->flags & expr->right->flags;
> + expr_flags_decay_consts(&expr->flags);
> +
What about expr->flags now if not TYPE_NUM?
Are we sure it's always initialized to NONE by the alloctor?
> diff --git a/validation/constexpr-binop.c b/validation/constexpr-binop.c
> @@ -0,0 +1,33 @@
> +static int a[] = {
> + [0 + 0] = 0, // OK
> + [0 + 0.] = 0, // KO
> + [(void*)0 + 0] = 0, // KO
> + [0 + __builtin_choose_expr(0, 0, 0)] = 0, // OK
> + [0 + __builtin_choose_expr(0, 0., 0)] = 0, // OK
> + [0 + __builtin_choose_expr(0, 0, 0.)] = 0, // KO
> + [0 < 0] = 0, // OK
> + [0 < 0.] = 0, // KO
It's not clear to me what the standrad says about this case.
What about the constness of 'usual artihmetic conversions' ?
Also GCC don't complain on this one.
> + [0 && 0.] = 0, // KO
Same here.
Luc
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 03/13] expression: examine constness of binops and alike at evaluation only
2016-01-26 0:14 ` Luc Van Oostenryck
@ 2016-01-26 15:50 ` Nicolai Stange
2016-01-26 17:24 ` Luc Van Oostenryck
0 siblings, 1 reply; 43+ messages in thread
From: Nicolai Stange @ 2016-01-26 15:50 UTC (permalink / raw)
To: Luc Van Oostenryck
Cc: Nicolai Stange, linux-sparse, Christopher Li, Josh Triplett
Luc Van Oostenryck <luc.vanoostenryck@gmail.com> writes:
> On Mon, Jan 25, 2016 at 03:52:14PM +0100, Nicolai Stange wrote:
>> Currently, the propagation of expressions' constness flags through
>> binary operations, compare and logical expressions is done in two
>> steps:
>> - Several flags are speculatively set at expression parsing time
>> - and possibly cleared again at evaluation time.
>>
>> Set aside this unfortunate split of code, the early propagation of
>> constness flags is not able to recognize constant expressions such as
>> 0 + __builtin_choose_expr(0, 0, 0)
>> 0 < __builtin_choose_expr(0, 0, 0)
>> 0 && __builtin_choose_expr(0, 0, 0)
>> since the final expression to be thrown into the binop-like expression
>> is known only after evaluation.
>>
>> Move the whole calculation of binary operations', compare and logical
>> expressions' constness flags to the evaluation phase.
>>
>> Introduce support for tracking arithmetic constness propagation through
>> binop-like expressions.
>
> Same as previous patch regarding the description and splitness of the
> patch.
And again, tracking arithmetic constness is more or less an implication
rather than an explicit change.
I will reformulate.
>
>> @@ -893,14 +891,11 @@ static struct symbol *evaluate_binop(struct expression *expr)
>> int rclass = classify_type(expr->right->ctype, &rtype);
>> int op = expr->op;
>>
>> - if (expr->flags) {
>> - if (!(expr->left->flags & expr->right->flags &
>> - EXPR_FLAG_INT_CONST_EXPR))
>> - expr->flags = EXPR_FLAG_NONE;
>> - }
>> -
>> /* number op number */
>> if (lclass & rclass & TYPE_NUM) {
>> + expr->flags = expr->left->flags & expr->right->flags;
>> + expr_flags_decay_consts(&expr->flags);
>> +
>
> What about expr->flags now if not TYPE_NUM?
> Are we sure it's always initialized to NONE by the alloctor?
>
>> diff --git a/validation/constexpr-binop.c b/validation/constexpr-binop.c
>> @@ -0,0 +1,33 @@
>> +static int a[] = {
>> + [0 + 0] = 0, // OK
>> + [0 + 0.] = 0, // KO
>> + [(void*)0 + 0] = 0, // KO
>> + [0 + __builtin_choose_expr(0, 0, 0)] = 0, // OK
>> + [0 + __builtin_choose_expr(0, 0., 0)] = 0, // OK
>> + [0 + __builtin_choose_expr(0, 0, 0.)] = 0, // KO
>> + [0 < 0] = 0, // OK
>> + [0 < 0.] = 0, // KO
>
> It's not clear to me what the standrad says about this case.
> What about the constness of 'usual artihmetic conversions' ?
> Also GCC don't complain on this one.
Within the square brackets, an integer constant expression is needed.
That's 6.6(6). "Floating constants that are immediate operands of casts"
are allowed. Implicitly promoted types are not, at least to my
interpretation.
>
>> + [0 && 0.] = 0, // KO
>
> Same here.
>
>
> Luc
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 03/13] expression: examine constness of binops and alike at evaluation only
2016-01-26 15:50 ` Nicolai Stange
@ 2016-01-26 17:24 ` Luc Van Oostenryck
2016-01-27 10:42 ` Nicolai Stange
0 siblings, 1 reply; 43+ messages in thread
From: Luc Van Oostenryck @ 2016-01-26 17:24 UTC (permalink / raw)
To: Nicolai Stange; +Cc: linux-sparse, Christopher Li, Josh Triplett
On Tue, Jan 26, 2016 at 04:50:07PM +0100, Nicolai Stange wrote:
> Luc Van Oostenryck <luc.vanoostenryck@gmail.com> writes:
>
> > On Mon, Jan 25, 2016 at 03:52:14PM +0100, Nicolai Stange wrote:
> >> + [0 < 0.] = 0, // KO
> >
> > It's not clear to me what the standrad says about this case.
> > What about the constness of 'usual artihmetic conversions' ?
> > Also GCC don't complain on this one.
>
> Within the square brackets, an integer constant expression is needed.
>
> That's 6.6(6). "Floating constants that are immediate operands of casts"
> are allowed. Implicitly promoted types are not, at least to my
> interpretation.
Yes, I saw that the standard isn't explicit about it.
The way I see things is:
- I don't see any reason why an explicit conversion would preserve
constness while an implicit one would not.
- intuitively, when I read the code I see that the result of this
expression is can be known at compile time.
But well ... I have the same issue with [(int) (0 + 0.0)] which
is clearly not allowed by the standard while [(int) 0.0] is.
Maybe those should be relaxed latter and we can invoke 6.6(10):
An implementation may accept other forms of constant expressions
OTOH, who cares about floats ;)
Reading a bit more about it ...
For the designator in the array initializer (but also probably elsewhere)
6.7.8(6) first uses
[ <i>constant-expression<\i> ]
and then
and the expression shall be an integer constant expression.
Can this last 'integer constant expression' be interpreted as 'constant
expression of integer type'?
This could be considered to be coherent with the footnote 99) in 6.6(6)
followed by 6.6(7).
I don't know, it's something for language lawyers.
Luc
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 03/13] expression: examine constness of binops and alike at evaluation only
2016-01-26 17:24 ` Luc Van Oostenryck
@ 2016-01-27 10:42 ` Nicolai Stange
2016-01-27 18:00 ` Luc Van Oostenryck
0 siblings, 1 reply; 43+ messages in thread
From: Nicolai Stange @ 2016-01-27 10:42 UTC (permalink / raw)
To: Luc Van Oostenryck
Cc: Nicolai Stange, linux-sparse, Christopher Li, Josh Triplett
Luc Van Oostenryck <luc.vanoostenryck@gmail.com> writes:
> On Tue, Jan 26, 2016 at 04:50:07PM +0100, Nicolai Stange wrote:
>> Luc Van Oostenryck <luc.vanoostenryck@gmail.com> writes:
>>
>> > On Mon, Jan 25, 2016 at 03:52:14PM +0100, Nicolai Stange wrote:
>> >> + [0 < 0.] = 0, // KO
>> >
>> > It's not clear to me what the standrad says about this case.
>> > What about the constness of 'usual artihmetic conversions' ?
>> > Also GCC don't complain on this one.
>>
>> Within the square brackets, an integer constant expression is needed.
>>
>> That's 6.6(6). "Floating constants that are immediate operands of casts"
>> are allowed. Implicitly promoted types are not, at least to my
>> interpretation.
>
> Yes, I saw that the standard isn't explicit about it.
> The way I see things is:
> - I don't see any reason why an explicit conversion would preserve
> constness while an implicit one would not.
Just to make it explicit here, we're not talking about "arithmetic
constant expressions", but "integer constant expressions".
I think the standard designers made this distinction in order to
differentiate between something so const that it can be used in static
initializers (arith. constexpr.) vs. sth. so const and _free of
surprises_ (integer constexpr.) that it can safely be used in various
sensitive places.
For example:
#define PI 3.14
switch(foo) {
case PI:
...
};
should certainly not be allowed, while a
#define PI 3.14
switch(foo) {
case (int)PI:
...
};
signals the compiler that the programmer knows (or pretends to know)
what he's doing, so it should be allowed.
OTOH,
0 < 0.
is clearly an arithmetic constant expression and can be used in static
initializers or wherever.
This is *my* interpretation of why the standard designers did it that
way. Of course I might be wrong though.
> - intuitively, when I read the code I see that the result of this
> expression is can be known at compile time.
Yes, the compiler knows that it's an _arithmetic_ constant expression.
>
> But well ... I have the same issue with [(int) (0 + 0.0)] which
Again, programmers writing code like this don't even pretend that they
know what they're doing. Why should a compiler or even sparse trust
them?
> is clearly not allowed by the standard while [(int) 0.0] is.
>
> Maybe those should be relaxed latter and we can invoke 6.6(10):
> An implementation may accept other forms of constant expressions
>
> OTOH, who cares about floats ;)
A true word. Thus, I suggest not to introduce any additional form of
constness at this moment. In the end, we wanted to be stricter than gcc.
If real world problems arise, we can easily return to that question.
But as you said, certainly nobody cares.
>
>
> Reading a bit more about it ...
>
> For the designator in the array initializer (but also probably elsewhere)
> 6.7.8(6) first uses
> [ <i>constant-expression<\i> ]
> and then
> and the expression shall be an integer constant expression.
>
> Can this last 'integer constant expression' be interpreted as 'constant
> expression of integer type'?
I'm sure that if the standard authors' real intents had been to allow
arithmetic constant expressions of integer types at this place, they
would have said so and not used the well defined term "integer constant
expression" at this point.
> This could be considered to be coherent with the footnote 99) in 6.6(6)
> followed by 6.6(7).
>
> I don't know, it's something for language lawyers.
If you don't agree with my interpretation, we could very well try to get
some language layer into our boat.
OTOH, if you agree that we could safely leave the semantics as they
currently are, I could just go on and prepare v3...
Nicolai
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 03/13] expression: examine constness of binops and alike at evaluation only
2016-01-27 10:42 ` Nicolai Stange
@ 2016-01-27 18:00 ` Luc Van Oostenryck
0 siblings, 0 replies; 43+ messages in thread
From: Luc Van Oostenryck @ 2016-01-27 18:00 UTC (permalink / raw)
To: Nicolai Stange; +Cc: linux-sparse, Christopher Li, Josh Triplett
On Wed, Jan 27, 2016 at 11:42:32AM +0100, Nicolai Stange wrote:
> Luc Van Oostenryck <luc.vanoostenryck@gmail.com> writes:
...
> > Maybe those should be relaxed latter and we can invoke 6.6(10):
> > An implementation may accept other forms of constant expressions
> >
> > OTOH, who cares about floats ;)
>
> A true word. Thus, I suggest not to introduce any additional form of
> constness at this moment. In the end, we wanted to be stricter than gcc.
>
> If real world problems arise, we can easily return to that question.
It's exactly what I meant.
...
> I'm sure that if the standard authors' real intents had been to allow
> arithmetic constant expressions of integer types at this place, they
> would have said so and not used the well defined term "integer constant
> expression" at this point.
I would definitively appreciate the standard being less ambiguous.
And I find the footnote 99) in 6.6 really confusing.
> > This could be considered to be coherent with the footnote 99) in 6.6(6)
> > followed by 6.6(7).
> >
> > I don't know, it's something for language lawyers.
>
> If you don't agree with my interpretation, we could very well try to get
> some language layer into our boat.
>
> OTOH, if you agree that we could safely leave the semantics as they
> currently are, I could just go on and prepare v3...
It's not that I really disagree with your interpretation, nor that I totally agree
with it. It's more me playing devil's advocate, insuring that the reasonable cases
are well covered.
It's the fact that GCC and your code differ on this point that made me stop at it
because it could make your changes less valuable which we don't want.
My remark about a language lawyer was not a serious one, it was more a way to say
"let's not spend more time on it for now".
Please go on.
And with what you have put in place for the expression constness it will be very easy
to relax some of the rules when there will be some needs for it.
Luc
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 03/13] expression: examine constness of binops and alike at evaluation only
2016-01-25 14:52 ` [PATCH v2 03/13] expression: examine constness of binops and alike " Nicolai Stange
2016-01-26 0:14 ` Luc Van Oostenryck
@ 2016-01-26 0:59 ` Luc Van Oostenryck
1 sibling, 0 replies; 43+ messages in thread
From: Luc Van Oostenryck @ 2016-01-26 0:59 UTC (permalink / raw)
To: Nicolai Stange; +Cc: linux-sparse, Christopher Li, Josh Triplett
On Mon, Jan 25, 2016 at 03:52:14PM +0100, Nicolai Stange wrote:
> @@ -1001,11 +996,9 @@ static struct symbol *evaluate_compare(struct expression *expr)
> struct symbol *ctype;
> const char *typediff;
>
> - if (expr->flags) {
> - if (!(expr->left->flags & expr->right->flags &
> - EXPR_FLAG_INT_CONST_EXPR))
> - expr->flags = EXPR_FLAG_NONE;
> - }
> + expr->flags = left->flags & right->flags;
> + expr_flags_decay_consts(&expr->flags);
> + expr_clear_flag(&expr->flags, EXPR_FLAG_ADDR_CONST_EXPR);
Related to the [0 < 0.0] test case, shouldn't this be (re) done after
the call to usual_conversions() ?
Luc
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 04/13] expression: examine constness of preops at evaluation only
2016-01-25 14:47 [PATCH v2 00/13] improve constexpr handling Nicolai Stange
` (2 preceding siblings ...)
2016-01-25 14:52 ` [PATCH v2 03/13] expression: examine constness of binops and alike " Nicolai Stange
@ 2016-01-25 14:53 ` Nicolai Stange
2016-01-26 1:10 ` Luc Van Oostenryck
2016-01-25 14:55 ` [PATCH v2 05/13] expression: examine constness of conditionals " Nicolai Stange
` (9 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Nicolai Stange @ 2016-01-25 14:53 UTC (permalink / raw)
To: linux-sparse
Cc: Nicolai Stange, Christopher Li, Josh Triplett, Luc Van Oostenryck
Currently, the propagation of expressions' constness flags through
prefix expressions is done in two steps:
- Several flags are speculatively set at expression parsing time
- and possibly cleared again at evaluation time.
Set aside this unfortunate split of code, the early propagation of
constness flags is not able to recognize constant expressions such as
-__builtin_choose_expr(0, 0, 0)
~__builtin_choose_expr(0, 0, 0)
!__builtin_choose_expr(0, 0, 0)
since the final expression to be thrown into the prefix expression is
known only after evaluation.
Move the whole calculation of prefix expressions' constness flags to
the evaluation phase.
Introduce support for tracking arithmetic constness propagation through
prefix expressions.
Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
evaluate.c | 17 ++++++++++++-----
expression.c | 4 ----
validation/constexpr-preop.c | 29 +++++++++++++++++++++++++++++
3 files changed, 41 insertions(+), 9 deletions(-)
create mode 100644 validation/constexpr-preop.c
diff --git a/evaluate.c b/evaluate.c
index 5e3e2ca..5138d40 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -1798,8 +1798,10 @@ static struct symbol *evaluate_sign(struct expression *expr)
{
struct symbol *ctype = expr->unop->ctype;
int class = classify_type(ctype, &ctype);
- if (expr->flags && !(expr->unop->flags & EXPR_FLAG_INT_CONST_EXPR))
- expr->flags = EXPR_FLAG_NONE;
+ unsigned char flags;
+
+ flags = expr->flags | expr->unop->flags;
+ expr_flags_decay_consts(&flags);
/* should be an arithmetic type */
if (!(class & TYPE_NUM))
return bad_expr_type(expr);
@@ -1816,6 +1818,7 @@ Normal:
}
if (expr->op == '+')
*expr = *expr->unop;
+ expr->flags = flags;
expr->ctype = ctype;
return ctype;
Restr:
@@ -1853,9 +1856,13 @@ static struct symbol *evaluate_preop(struct expression *expr)
return evaluate_postop(expr);
case '!':
- if (expr->flags && !(expr->unop->flags &
- EXPR_FLAG_INT_CONST_EXPR))
- expr->flags = EXPR_FLAG_NONE;
+ expr->flags = expr->unop->flags;
+ expr_flags_decay_consts(&expr->flags);
+ /*
+ * A logical negation never yields an address constant
+ * [6.6(9)].
+ */
+ expr_clear_flag(&expr->flags, EXPR_FLAG_ADDR_CONST_EXPR);
if (is_safe_type(ctype))
warning(expr->pos, "testing a 'safe expression'");
if (is_float_type(ctype)) {
diff --git a/expression.c b/expression.c
index e49a19d..377834e 100644
--- a/expression.c
+++ b/expression.c
@@ -452,8 +452,6 @@ struct token *primary_expression(struct token *token, struct expression **tree)
expr = alloc_expression(token->pos, EXPR_PREOP);
expr->op = '(';
token = parens_expression(token, &expr->unop, "in expression");
- if (expr->unop)
- expr->flags = expr->unop->flags;
break;
}
if (token->special == '[' && lookup_type(token->next)) {
@@ -665,8 +663,6 @@ static struct token *unary_expression(struct token *token, struct expression **t
unary = alloc_expression(token->pos, EXPR_PREOP);
unary->op = token->special;
unary->unop = unop;
- unary->flags = unop->flags;
- expr_flags_decay_consts(&unary->flags);
*tree = unary;
return next;
}
diff --git a/validation/constexpr-preop.c b/validation/constexpr-preop.c
new file mode 100644
index 0000000..5d869da
--- /dev/null
+++ b/validation/constexpr-preop.c
@@ -0,0 +1,29 @@
+static int a[] = {
+ [+0] = 0, // OK
+ [+__builtin_choose_expr(0, 0, 0)] = 0, // OK
+ [+0.] = 0, // KO
+ [+__builtin_choose_expr(0, 0, 0.)] = 0, // KO
+ [-0] = 0, // OK
+ [-__builtin_choose_expr(0, 0, 0)] = 0, // OK
+ [-0.] = 0, // KO
+ [-__builtin_choose_expr(0, 0, 0.)] = 0, // KO
+ [~0] = 0, // OK
+ [~__builtin_choose_expr(0, 0, 0)] = 0, // OK
+ [!0] = 0, // OK
+ [!__builtin_choose_expr(0, 0, 0)] = 0, // OK
+ [!0.] = 0, // KO
+ [!__builtin_choose_expr(0, 0, 0.)] = 0, // KO
+};
+
+/*
+ * check-name: Expression constness propagation in preops
+ *
+ * check-error-start
+constexpr-preop.c:4:5: error: bad constant expression
+constexpr-preop.c:5:33: error: bad constant expression
+constexpr-preop.c:8:4: error: bad constant expression
+constexpr-preop.c:9:4: error: bad constant expression
+constexpr-preop.c:14:4: error: bad integer constant expression
+constexpr-preop.c:15:4: error: bad integer constant expression
+ * check-error-end
+ */
--
2.7.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 04/13] expression: examine constness of preops at evaluation only
2016-01-25 14:53 ` [PATCH v2 04/13] expression: examine constness of preops " Nicolai Stange
@ 2016-01-26 1:10 ` Luc Van Oostenryck
0 siblings, 0 replies; 43+ messages in thread
From: Luc Van Oostenryck @ 2016-01-26 1:10 UTC (permalink / raw)
To: Nicolai Stange; +Cc: linux-sparse, Christopher Li, Josh Triplett
On Mon, Jan 25, 2016 at 03:53:38PM +0100, Nicolai Stange wrote:
> Currently, the propagation of expressions' constness flags through
> prefix expressions is done in two steps:
> - Several flags are speculatively set at expression parsing time
> - and possibly cleared again at evaluation time.
>
> Set aside this unfortunate split of code, the early propagation of
> constness flags is not able to recognize constant expressions such as
> -__builtin_choose_expr(0, 0, 0)
> ~__builtin_choose_expr(0, 0, 0)
> !__builtin_choose_expr(0, 0, 0)
> since the final expression to be thrown into the prefix expression is
> known only after evaluation.
>
> Move the whole calculation of prefix expressions' constness flags to
> the evaluation phase.
>
> Introduce support for tracking arithmetic constness propagation through
> prefix expressions.
What about replacing the whole description with something::
Move the whole calculation of prefix expressions' constness flags to
the evaluation phase so that we can now recognize constant expressions
such as
-__builtin_choose_expr(0, 0, 0)
~__builtin_choose_expr(0, 0, 0)
!__builtin_choose_expr(0, 0, 0)
and adding further explanation after this, if really needed?
Luc
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 05/13] expression: examine constness of conditionals at evaluation only
2016-01-25 14:47 [PATCH v2 00/13] improve constexpr handling Nicolai Stange
` (3 preceding siblings ...)
2016-01-25 14:53 ` [PATCH v2 04/13] expression: examine constness of preops " Nicolai Stange
@ 2016-01-25 14:55 ` Nicolai Stange
2016-01-26 1:16 ` Luc Van Oostenryck
2016-01-25 14:56 ` [PATCH v2 06/13] expression, evaluate: add support for recognizing address constants Nicolai Stange
` (8 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Nicolai Stange @ 2016-01-25 14:55 UTC (permalink / raw)
To: linux-sparse
Cc: Nicolai Stange, Christopher Li, Josh Triplett, Luc Van Oostenryck
Currently, the propagation of expressions' constness flags through
conditional expressions is done in two steps:
- Several flags are speculatively set at expression parsing time
- and possibly cleared again at evaluation time.
Set aside this unfortunate split of code, the early propagation of
constness flags is not able to recognize constant expressions such as
0 ? __builtin_choose_expr(0, 0, 0) : 0
0 ? 0 : __builtin_choose_expr(0, 0, 0)
since the final expression to be thrown into the conditional
expression is known only after evaluation.
Move the whole calculation of conditional expressions' constness flags
to the evaluation phase.
Introduce support for tracking arithmetic constness propagation through
conditional expressions.
Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
evaluate.c | 18 ++++++++----------
expression.c | 7 -------
validation/constexpr-conditional.c | 34 ++++++++++++++++++++++++++++++++++
3 files changed, 42 insertions(+), 17 deletions(-)
create mode 100644 validation/constexpr-conditional.c
diff --git a/evaluate.c b/evaluate.c
index 5138d40..97da51d 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -1114,16 +1114,14 @@ static struct symbol *evaluate_conditional_expression(struct expression *expr)
true = &expr->cond_true;
}
- if (expr->flags) {
- unsigned char flags_mask = 0;
- unsigned char flags;
-
- expr_set_flag(&flags_mask, EXPR_FLAG_INT_CONST_EXPR);
- flags = (expr->conditional->flags & flags_mask);
- flags &= (*true)->flags & expr->cond_false->flags;
- if (!flags)
- expr->flags = EXPR_FLAG_NONE;
- }
+ expr->flags = (expr->conditional->flags & (*true)->flags &
+ expr->cond_false->flags);
+ expr_flags_decay_consts(&expr->flags);
+ /*
+ * A conditional operator never yields an address constant
+ * [6.6(9)].
+ */
+ expr_clear_flag(&expr->flags, EXPR_FLAG_ADDR_CONST_EXPR);
lclass = classify_type(ltype, <ype);
rclass = classify_type(rtype, &rtype);
diff --git a/expression.c b/expression.c
index 377834e..792c2a5 100644
--- a/expression.c
+++ b/expression.c
@@ -858,13 +858,6 @@ struct token *conditional_expression(struct token *token, struct expression **tr
token = parse_expression(token->next, &expr->cond_true);
token = expect(token, ':', "in conditional expression");
token = conditional_expression(token, &expr->cond_false);
- if (expr->left && expr->cond_false) {
- expr->flags = expr->left->flags &
- expr->cond_false->flags;
- if (expr->cond_true)
- expr->flags &= expr->cond_true->flags;
- expr_flags_decay_consts(&expr->flags);
- }
}
return token;
}
diff --git a/validation/constexpr-conditional.c b/validation/constexpr-conditional.c
new file mode 100644
index 0000000..a3331b3
--- /dev/null
+++ b/validation/constexpr-conditional.c
@@ -0,0 +1,34 @@
+static int a[] = {
+ [0 ? : 0] = 0, // OK
+ [1 ? : 0] = 0, // OK
+ [0 ? 0 : 0] = 0, // OK
+ [1 ? 0 : 0] = 0, // OK
+ [0 ? 0 : __builtin_choose_expr(0, 0, 0)] = 0, // OK
+ [1 ? __builtin_choose_expr(0, 0, 0) : 0] = 0, // OK
+ [0 ? __builtin_choose_expr(0, 0, 0) : 0] = 0, // OK
+ [1 ? 1 : __builtin_choose_expr(0, 0, 0)] = 0, // OK
+ [__builtin_choose_expr(0, 0, 0) ? : 0] = 0, // OK
+ [__builtin_choose_expr(0, 0, 1) ? : 0] = 0, // OK
+ [0. ? : 0] = 0, // KO
+ [0 ? 0. : 0] = 0, // KO
+ [1 ? : 0.] = 0, // KO
+ [__builtin_choose_expr(0, 0., 0) ? : 0] = 0, // OK
+ [__builtin_choose_expr(0, 0, 0.) ? : 0] = 0, // KO
+ [0 ? __builtin_choose_expr(0, 0., 0) : 0] = 0, // OK
+ [0 ? __builtin_choose_expr(0, 0, 0.) : 0] = 0, // KO
+ [1 ? 0 : __builtin_choose_expr(0, 0., 0)] = 0, // OK
+ [1 ? 0 : __builtin_choose_expr(0, 0, 0.)] = 0, // KO
+};
+
+/*
+ * check-name: Expression constness propagation in conditional expressions
+ *
+ * check-error-start
+constexpr-conditional.c:12:13: error: bad constant expression
+constexpr-conditional.c:13:19: error: bad constant expression
+constexpr-conditional.c:14:12: error: bad constant expression
+constexpr-conditional.c:16:42: error: bad constant expression
+constexpr-conditional.c:18:48: error: bad constant expression
+constexpr-conditional.c:20:14: error: bad constant expression
+ * check-error-end
+ */
--
2.7.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 05/13] expression: examine constness of conditionals at evaluation only
2016-01-25 14:55 ` [PATCH v2 05/13] expression: examine constness of conditionals " Nicolai Stange
@ 2016-01-26 1:16 ` Luc Van Oostenryck
0 siblings, 0 replies; 43+ messages in thread
From: Luc Van Oostenryck @ 2016-01-26 1:16 UTC (permalink / raw)
To: Nicolai Stange; +Cc: linux-sparse, Christopher Li, Josh Triplett
On Mon, Jan 25, 2016 at 03:55:09PM +0100, Nicolai Stange wrote:
> Currently, the propagation of expressions' constness flags through
> conditional expressions is done in two steps:
> - Several flags are speculatively set at expression parsing time
> - and possibly cleared again at evaluation time.
>
> Set aside this unfortunate split of code, the early propagation of
> constness flags is not able to recognize constant expressions such as
> 0 ? __builtin_choose_expr(0, 0, 0) : 0
> 0 ? 0 : __builtin_choose_expr(0, 0, 0)
> since the final expression to be thrown into the conditional
> expression is known only after evaluation.
>
> Move the whole calculation of conditional expressions' constness flags
> to the evaluation phase.
>
> Introduce support for tracking arithmetic constness propagation through
> conditional expressions.
Same comments about the description as for the previous patches.
Good otherwise.
Luc
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 06/13] expression, evaluate: add support for recognizing address constants
2016-01-25 14:47 [PATCH v2 00/13] improve constexpr handling Nicolai Stange
` (4 preceding siblings ...)
2016-01-25 14:55 ` [PATCH v2 05/13] expression: examine constness of conditionals " Nicolai Stange
@ 2016-01-25 14:56 ` Nicolai Stange
2016-01-26 1:27 ` Luc Van Oostenryck
2016-01-26 3:10 ` Luc Van Oostenryck
2016-01-25 14:57 ` [PATCH v2 07/13] evaluate: check static storage duration objects' intializers' constness Nicolai Stange
` (7 subsequent siblings)
13 siblings, 2 replies; 43+ messages in thread
From: Nicolai Stange @ 2016-01-25 14:56 UTC (permalink / raw)
To: linux-sparse
Cc: Nicolai Stange, Christopher Li, Josh Triplett, Luc Van Oostenryck
Address constants [6.6(9)] constitute one of the types of constant
expressions allowed in initializers [6.6(7)] for static storage
duration objects [6.7.8(4)].
Introduce support for recognizing address constants created either
- explicitly by referencing a static storage duration object by means
of the unary & operator
- or implicitly by the use of an expression of array or function type.
Treat string literals as address constants.
Initially tag an expression as being an address constant at the
primary expression level, i.e. upon encountering a symbol designating
an object of static storage duration in primary_expression().
Carry these flags over to the *-preop wrapped expression created by
evaluate_symbol_expression().
For the special case of string literals, tag them as address constants
in evaluate_string().
Take care in evaluate_ptr_add() and evaluate_offset()
to properly propagate the address constness flags from
subexpressions to their parent expressions, namely the array ([])
or structure member dereference (->, .) expressions.
Finally, do not strip away an *-preop wrapped expression's constness
flags in evaluate_addressof().
Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
evaluate.c | 18 +++++++++++++++++-
expression.c | 8 ++++++++
2 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/evaluate.c b/evaluate.c
index 97da51d..70f419f 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -70,9 +70,11 @@ static struct symbol *evaluate_symbol_expression(struct expression *expr)
addr->symbol = sym;
addr->symbol_name = expr->symbol_name;
addr->ctype = &lazy_ptr_ctype; /* Lazy evaluation: we need to do a proper job if somebody does &sym */
+ addr->flags = expr->flags;
expr->type = EXPR_PREOP;
expr->op = '*';
expr->unop = addr;
+ expr->flags = EXPR_FLAG_NONE;
/* The type of a symbol is the symbol itself! */
expr->ctype = sym;
@@ -106,6 +108,7 @@ static struct symbol *evaluate_string(struct expression *expr)
addr->symbol = sym;
addr->ctype = &lazy_ptr_ctype;
+ expr_set_flag(&addr->flags, EXPR_FLAG_ADDR_CONST_EXPR);
expr->type = EXPR_PREOP;
expr->op = '*';
@@ -563,6 +566,14 @@ static struct symbol *evaluate_ptr_add(struct expression *expr, struct symbol *i
classify_type(degenerate(expr->left), &ctype);
base = examine_pointer_target(ctype);
+ /*
+ * An address constant +/- an integer constant expression
+ * yields an address constant again [6.6(7)].
+ */
+ if((expr->left->flags & EXPR_FLAG_ADDR_CONST_EXPR) &&
+ (expr->right->flags & EXPR_FLAG_INT_CONST_EXPR))
+ expr_set_flag(&expr->flags, EXPR_FLAG_ADDR_CONST_EXPR);
+
if (!base) {
expression_error(expr, "missing type information");
return NULL;
@@ -1678,7 +1689,6 @@ static struct symbol *evaluate_addressof(struct expression *expr)
}
ctype = op->ctype;
*expr = *op->unop;
- expr->flags = EXPR_FLAG_NONE;
if (expr->type == EXPR_SYMBOL) {
struct symbol *sym = expr->symbol;
@@ -1945,6 +1955,12 @@ static struct expression *evaluate_offset(struct expression *expr, unsigned long
* we ever take the address of this member dereference..
*/
add->ctype = &lazy_ptr_ctype;
+ /*
+ * An address constant +/- an integer constant expression
+ * yields an address constant again [6.6(7)].
+ */
+ add->flags |= expr->flags;
+
return add;
}
diff --git a/expression.c b/expression.c
index 792c2a5..afc4f39 100644
--- a/expression.c
+++ b/expression.c
@@ -437,6 +437,14 @@ struct token *primary_expression(struct token *token, struct expression **tree)
}
expr->symbol_name = token->ident;
expr->symbol = sym;
+
+ /*
+ * A pointer to an lvalue designating a static storage
+ * duration object is an address constant [6.6(9)].
+ */
+ if(sym && (sym->ctype.modifiers & (MOD_TOPLEVEL | MOD_STATIC)))
+ expr_set_flag(&expr->flags, EXPR_FLAG_ADDR_CONST_EXPR);
+
token = next;
break;
}
--
2.7.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 06/13] expression, evaluate: add support for recognizing address constants
2016-01-25 14:56 ` [PATCH v2 06/13] expression, evaluate: add support for recognizing address constants Nicolai Stange
@ 2016-01-26 1:27 ` Luc Van Oostenryck
2016-01-26 3:10 ` Luc Van Oostenryck
1 sibling, 0 replies; 43+ messages in thread
From: Luc Van Oostenryck @ 2016-01-26 1:27 UTC (permalink / raw)
To: Nicolai Stange; +Cc: linux-sparse, Christopher Li, Josh Triplett
On Mon, Jan 25, 2016 at 03:56:23PM +0100, Nicolai Stange wrote:
> Address constants [6.6(9)] constitute one of the types of constant
> expressions allowed in initializers [6.6(7)] for static storage
> duration objects [6.7.8(4)].
>
> Introduce support for recognizing address constants created either
> - explicitly by referencing a static storage duration object by means
> of the unary & operator
> - or implicitly by the use of an expression of array or function type.
>
> Treat string literals as address constants.
>
> Initially tag an expression as being an address constant at the
> primary expression level, i.e. upon encountering a symbol designating
> an object of static storage duration in primary_expression().
>
> Carry these flags over to the *-preop wrapped expression created by
> evaluate_symbol_expression().
>
> For the special case of string literals, tag them as address constants
> in evaluate_string().
>
> Take care in evaluate_ptr_add() and evaluate_offset()
> to properly propagate the address constness flags from
> subexpressions to their parent expressions, namely the array ([])
> or structure member dereference (->, .) expressions.
>
> Finally, do not strip away an *-preop wrapped expression's constness
> flags in evaluate_addressof().
There is two much thing in the description, this is a sure sign that the patch
should be splitted, like putting the part concerning strings in a separate one.
> @@ -1678,7 +1689,6 @@ static struct symbol *evaluate_addressof(struct expression *expr)
> }
> ctype = op->ctype;
> *expr = *op->unop;
> - expr->flags = EXPR_FLAG_NONE;
Is this because it's already initialized so by the allocator?
if so, it can be moved to the patch that add this initialization
(because it's unrelated to the current change).
Good otherwise.
Luc
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 06/13] expression, evaluate: add support for recognizing address constants
2016-01-25 14:56 ` [PATCH v2 06/13] expression, evaluate: add support for recognizing address constants Nicolai Stange
2016-01-26 1:27 ` Luc Van Oostenryck
@ 2016-01-26 3:10 ` Luc Van Oostenryck
1 sibling, 0 replies; 43+ messages in thread
From: Luc Van Oostenryck @ 2016-01-26 3:10 UTC (permalink / raw)
To: Nicolai Stange; +Cc: linux-sparse, Christopher Li, Josh Triplett
On Mon, Jan 25, 2016 at 03:56:23PM +0100, Nicolai Stange wrote:
> @@ -563,6 +566,14 @@ static struct symbol *evaluate_ptr_add(struct expression *expr, struct symbol *i
> classify_type(degenerate(expr->left), &ctype);
> base = examine_pointer_target(ctype);
>
> + /*
> + * An address constant +/- an integer constant expression
> + * yields an address constant again [6.6(7)].
> + */
> + if((expr->left->flags & EXPR_FLAG_ADDR_CONST_EXPR) &&
Missing ' ' in 'if('
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 07/13] evaluate: check static storage duration objects' intializers' constness
2016-01-25 14:47 [PATCH v2 00/13] improve constexpr handling Nicolai Stange
` (5 preceding siblings ...)
2016-01-25 14:56 ` [PATCH v2 06/13] expression, evaluate: add support for recognizing address constants Nicolai Stange
@ 2016-01-25 14:57 ` Nicolai Stange
2016-01-26 1:42 ` Luc Van Oostenryck
2016-01-25 14:59 ` [PATCH v2 08/13] expression: recognize references to labels as address constants Nicolai Stange
` (6 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Nicolai Stange @ 2016-01-25 14:57 UTC (permalink / raw)
To: linux-sparse
Cc: Nicolai Stange, Christopher Li, Josh Triplett, Luc Van Oostenryck
Initializers of static storage duration objects shall be constant
expressions [6.7.8(4)].
Warn if that requirement is not met and the -Wstatic-initializer-not-const
flag has been given on sparse's command line.
Identify static storage duration objects by having either of
MOD_TOPLEVEL or MOD_STATIC set.
Check an initializer's constness at the lowest possible subobject
level, i.e. at the level of the "assignment-expression" production
in [6.7.8].
For compound objects, make handle_list_initializer() pass the
surrounding object's storage duration modifiers down to
handle_simple_initializer() at subobject initializer evaluation.
Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
evaluate.c | 26 ++++++++++-
lib.c | 2 +
lib.h | 2 +-
sparse.1 | 7 +++
validation/constexpr-init.c | 110 ++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 145 insertions(+), 2 deletions(-)
create mode 100644 validation/constexpr-init.c
diff --git a/evaluate.c b/evaluate.c
index 70f419f..e3b08e4 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -2468,6 +2468,7 @@ static void handle_list_initializer(struct expression *expr,
{
struct expression *e, *last = NULL, *top = NULL, *next;
int jumped = 0;
+ unsigned long old_modifiers;
FOR_EACH_PTR(expr->expr_list, e) {
struct expression **v;
@@ -2522,8 +2523,21 @@ found:
else
v = &top->ident_expression;
- if (handle_simple_initializer(v, 1, lclass, top->ctype))
+ /*
+ * Temporarily copy storage modifiers down from
+ * surrounding type such that
+ * handle_simple_initializer() can check
+ * initializations of subobjects with static storage
+ * duration.
+ */
+ old_modifiers = top->ctype->ctype.modifiers;
+ top->ctype->ctype.modifiers =
+ old_modifiers | (ctype->ctype.modifiers & MOD_STORAGE);
+ if (handle_simple_initializer(v, 1, lclass, top->ctype)) {
+ top->ctype->ctype.modifiers = old_modifiers;
continue;
+ }
+ top->ctype->ctype.modifiers = old_modifiers;
if (!(lclass & TYPE_COMPOUND)) {
warning(e->pos, "bogus scalar initializer");
@@ -2633,6 +2647,16 @@ static int handle_simple_initializer(struct expression **ep, int nested,
if (!evaluate_expression(e))
return 1;
compatible_assignment_types(e, ctype, ep, "initializer");
+ /*
+ * Initializers for static storage duration objects
+ * shall be constant expressions or a string literal [6.7.8(4)].
+ */
+ if ((ctype->ctype.modifiers & (MOD_TOPLEVEL | MOD_STATIC)) &&
+ !(e->flags & (EXPR_FLAG_ARITH_CONST_EXPR
+ | EXPR_FLAG_ADDR_CONST_EXPR)) &&
+ Wstatic_initializer_not_const)
+ warning(e->pos, "initializer for static storage duration object is not a constant expression");
+
return 1;
}
diff --git a/lib.c b/lib.c
index 8dc5bcf..855fb3e 100644
--- a/lib.c
+++ b/lib.c
@@ -241,6 +241,7 @@ int Wtypesign = 0;
int Wundef = 0;
int Wuninitialized = 1;
int Wvla = 1;
+int Wstatic_initializer_not_const = 0;
int dbg_entry = 0;
int dbg_dead = 0;
@@ -464,6 +465,7 @@ static const struct warning {
{ "undef", &Wundef },
{ "uninitialized", &Wuninitialized },
{ "vla", &Wvla },
+ { "static-initializer-not-const", &Wstatic_initializer_not_const},
};
enum {
diff --git a/lib.h b/lib.h
index 15b69fa..1b38db2 100644
--- a/lib.h
+++ b/lib.h
@@ -127,7 +127,7 @@ extern int Wtypesign;
extern int Wundef;
extern int Wuninitialized;
extern int Wvla;
-
+extern int Wstatic_initializer_not_const;
extern int dbg_entry;
extern int dbg_dead;
diff --git a/sparse.1 b/sparse.1
index 4adaf6c..0df27f9 100644
--- a/sparse.1
+++ b/sparse.1
@@ -308,6 +308,13 @@ C99 does not specify the sizeof a _Bool. gcc uses 1.
Sparse does not issue these warnings by default.
.
.TP
+.B \-Wstatic-initializer-not-const
+Warn when initializing an object of static storage duration with an initializer
+which is not a constant expression.
+
+Sparse does not issue these warnings by default.
+.
+.TP
.B \-Wtransparent\-union
Warn about any declaration using the GCC extension
\fB__attribute__((transparent_union))\fR.
diff --git a/validation/constexpr-init.c b/validation/constexpr-init.c
new file mode 100644
index 0000000..b357de1
--- /dev/null
+++ b/validation/constexpr-init.c
@@ -0,0 +1,110 @@
+static int a = 1; // OK
+static int b[2] = {1, 1}; // OK
+static void c(void) {}
+
+static int *d = &a; // OK
+static int *e = &b[1]; // OK
+static int *f = b; // OK
+static void (*g)(void) = c; // OK
+static void (*h)(void) = &c; // OK
+static int *i = (int*)0; // OK
+static int *j = d; // KO
+static int *k = (int*)0 + 1; // OK
+
+static int *l = &a + 1; // OK
+static int *m = &b[1] + 1; // OK
+static int *n = b + 1; // OK
+static int *o = d + 1; // KO
+
+static int *p = &*&a; // OK
+static int *q = &*&b[1]; // OK
+static int *r = &*b; // OK
+static int *s = &*d; // KO
+
+static int *t = &*(&a + 1); // OK
+static int *u = &*(&b[1] + 1); // OK
+static int *v = &*(b + 1); // OK
+static int *w = &*(d + 1); // KO
+
+
+struct A {
+ int a;
+ int b[2];
+};
+
+struct B {
+ int c;
+ struct A d;
+};
+
+static struct B x= {1, {1, {1, 1}}}; // OK
+static struct B y= {a, {1, {1, 1}}}; // KO
+static struct B z= {1, {a, {1, 1}}}; // KO
+static struct B aa= {1, {1, {a, 1}}}; // KO
+static struct B ab= {1, {1, {1, a}}}; // KO
+static struct B ac= {.c = 1, .d = {.a = 1, .b = {1, 1}}}; // OK
+static struct B ad= {.c = a, .d = {.a = 1, .b = {1, 1}}}; // KO
+static struct B ae= {.c = 1, .d = {.a = a, .b = {1, 1}}}; // KO
+static struct B af= {.c = 1, .d = {.a = 1, .b = {a, 1}}}; // KO
+static struct B ag= {.c = 1, .d = {.a = 1, .b = {1, a}}}; // KO
+
+static int *ah = &x.d.a; // OK
+static int *ai = &(&x.d)->a; // OK
+static int *aj = x.d.b; // OK
+static int *ak = (&x.d)->b; // OK
+static int *al = &x.d.b[1]; // OK
+static int *am = &(&x.d)->b[1]; // OK
+
+static int an[] = {a, 1}; // KO
+static int ao[] = {1, a}; // KO
+static int ap[] = {[0] = a, [1] = 1}; // KO
+static int aq[] = {[0] = 1, [1] = a}; // KO
+
+static char *ar = "foobar"; // OK
+
+static void as(void) {
+ int a = 0;
+ int b = a; // OK
+}
+
+static void at(void) {
+ int a = 1;
+ static int b = a; // KO
+}
+
+static void au(void) {
+ int a = 1;
+ static int *b = &a; // KO
+}
+
+static void av(void) {
+ static int a = 1;
+ static int *b = &a; // OK
+}
+
+
+/*
+ * check-name: Static storage object initializer constness verification.
+ * check-command: sparse -Wstatic-initializer-not-const $file
+ *
+ * check-error-start
+constexpr-init.c:11:17: warning: initializer for static storage duration object is not a constant expression
+constexpr-init.c:17:19: warning: initializer for static storage duration object is not a constant expression
+constexpr-init.c:22:19: warning: initializer for static storage duration object is not a constant expression
+constexpr-init.c:27:22: warning: initializer for static storage duration object is not a constant expression
+constexpr-init.c:41:21: warning: initializer for static storage duration object is not a constant expression
+constexpr-init.c:42:25: warning: initializer for static storage duration object is not a constant expression
+constexpr-init.c:43:30: warning: initializer for static storage duration object is not a constant expression
+constexpr-init.c:44:33: warning: initializer for static storage duration object is not a constant expression
+constexpr-init.c:46:27: warning: initializer for static storage duration object is not a constant expression
+constexpr-init.c:47:41: warning: initializer for static storage duration object is not a constant expression
+constexpr-init.c:48:50: warning: initializer for static storage duration object is not a constant expression
+constexpr-init.c:49:53: warning: initializer for static storage duration object is not a constant expression
+constexpr-init.c:58:20: warning: initializer for static storage duration object is not a constant expression
+constexpr-init.c:59:23: warning: initializer for static storage duration object is not a constant expression
+constexpr-init.c:60:26: warning: initializer for static storage duration object is not a constant expression
+constexpr-init.c:61:35: warning: initializer for static storage duration object is not a constant expression
+constexpr-init.c:72:24: warning: initializer for static storage duration object is not a constant expression
+constexpr-init.c:77:26: warning: initializer for static storage duration object is not a constant expression
+ * check-error-end
+ */
--
2.7.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 07/13] evaluate: check static storage duration objects' intializers' constness
2016-01-25 14:57 ` [PATCH v2 07/13] evaluate: check static storage duration objects' intializers' constness Nicolai Stange
@ 2016-01-26 1:42 ` Luc Van Oostenryck
2016-01-26 16:08 ` Nicolai Stange
2016-02-01 3:00 ` Nicolai Stange
0 siblings, 2 replies; 43+ messages in thread
From: Luc Van Oostenryck @ 2016-01-26 1:42 UTC (permalink / raw)
To: Nicolai Stange; +Cc: linux-sparse, Christopher Li, Josh Triplett
On Mon, Jan 25, 2016 at 03:57:45PM +0100, Nicolai Stange wrote:
> Initializers of static storage duration objects shall be constant
> expressions [6.7.8(4)].
>
> Warn if that requirement is not met and the -Wstatic-initializer-not-const
> flag has been given on sparse's command line.
>
> Identify static storage duration objects by having either of
> MOD_TOPLEVEL or MOD_STATIC set.
>
> Check an initializer's constness at the lowest possible subobject
> level, i.e. at the level of the "assignment-expression" production
> in [6.7.8].
>
> For compound objects, make handle_list_initializer() pass the
> surrounding object's storage duration modifiers down to
> handle_simple_initializer() at subobject initializer evaluation.
Better here also to split the patch in two:
one add the -W flag flag and another one which will use it.
ch > Signed-off-by: Nicolai Stange <nicstange@gmail.com>
> ---
> evaluate.c | 26 ++++++++++-
> lib.c | 2 +
> lib.h | 2 +-
> sparse.1 | 7 +++
> validation/constexpr-init.c | 110 ++++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 145 insertions(+), 2 deletions(-)
> create mode 100644 validation/constexpr-init.c
>
> diff --git a/evaluate.c b/evaluate.c
> index 70f419f..e3b08e4 100644
> --- a/evaluate.c
> +++ b/evaluate.c
> @@ -2468,6 +2468,7 @@ static void handle_list_initializer(struct expression *expr,
> {
> struct expression *e, *last = NULL, *top = NULL, *next;
> int jumped = 0;
> + unsigned long old_modifiers;
>
> FOR_EACH_PTR(expr->expr_list, e) {
> struct expression **v;
> @@ -2522,8 +2523,21 @@ found:
> else
> v = &top->ident_expression;
>
> - if (handle_simple_initializer(v, 1, lclass, top->ctype))
> + /*
> + * Temporarily copy storage modifiers down from
> + * surrounding type such that
> + * handle_simple_initializer() can check
> + * initializations of subobjects with static storage
> + * duration.
> + */
> + old_modifiers = top->ctype->ctype.modifiers;
> + top->ctype->ctype.modifiers =
> + old_modifiers | (ctype->ctype.modifiers & MOD_STORAGE);
> + if (handle_simple_initializer(v, 1, lclass, top->ctype)) {
> + top->ctype->ctype.modifiers = old_modifiers;
> continue;
> + }
> + top->ctype->ctype.modifiers = old_modifiers;
I don't understand why saving the mods is needed.
It feels hackish to me. Isn't it because something is done wrongly at another
level or maybe handle_simple_initializer() need an additional arg or so?
> @@ -2633,6 +2647,16 @@ static int handle_simple_initializer(struct expression **ep, int nested,
...
> + warning(e->pos, "initializer for static storage duration object is not a constant expression");
This is quite longish message.
What about something like "non-constant initializer"?
> diff --git a/lib.c b/lib.c
> --- a/lib.c
> +++ b/lib.c
> @@ -241,6 +241,7 @@ int Wtypesign = 0;
> int Wundef = 0;
> int Wuninitialized = 1;
> int Wvla = 1;
> +int Wstatic_initializer_not_const = 0;
Here also it's quite longish. Yes I'm a lazy typer :)
What about simply -Wconst-initializer ?
One thing that could be added (later and in another patch) is to
set it by default when the C99 variant is selected.
> enum {
> diff --git a/lib.h b/lib.h
> index 15b69fa..1b38db2 100644
> --- a/lib.h
> +++ b/lib.h
> @@ -127,7 +127,7 @@ extern int Wtypesign;
> extern int Wundef;
> extern int Wuninitialized;
> extern int Wvla;
> -
> +extern int Wstatic_initializer_not_const;
Better to leave the blank line where it was.
Luc
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 07/13] evaluate: check static storage duration objects' intializers' constness
2016-01-26 1:42 ` Luc Van Oostenryck
@ 2016-01-26 16:08 ` Nicolai Stange
2016-01-26 17:56 ` Luc Van Oostenryck
2016-02-01 3:00 ` Nicolai Stange
1 sibling, 1 reply; 43+ messages in thread
From: Nicolai Stange @ 2016-01-26 16:08 UTC (permalink / raw)
To: Luc Van Oostenryck
Cc: Nicolai Stange, linux-sparse, Christopher Li, Josh Triplett
Luc Van Oostenryck <luc.vanoostenryck@gmail.com> writes:
> On Mon, Jan 25, 2016 at 03:57:45PM +0100, Nicolai Stange wrote:
>> Initializers of static storage duration objects shall be constant
>> expressions [6.7.8(4)].
>>
>> Warn if that requirement is not met and the -Wstatic-initializer-not-const
>> flag has been given on sparse's command line.
>>
>> Identify static storage duration objects by having either of
>> MOD_TOPLEVEL or MOD_STATIC set.
>>
>> Check an initializer's constness at the lowest possible subobject
>> level, i.e. at the level of the "assignment-expression" production
>> in [6.7.8].
>>
>> For compound objects, make handle_list_initializer() pass the
>> surrounding object's storage duration modifiers down to
>> handle_simple_initializer() at subobject initializer evaluation.
>
> Better here also to split the patch in two:
> one add the -W flag flag and another one which will use it.
Introducing a flag without any functionality attached to it feels wrong
for me. For example, where to update the manpage? Before or after actual
functionality is introduced?
>
> ch > Signed-off-by: Nicolai Stange <nicstange@gmail.com>
>> ---
>> evaluate.c | 26 ++++++++++-
>> lib.c | 2 +
>> lib.h | 2 +-
>> sparse.1 | 7 +++
>> validation/constexpr-init.c | 110 ++++++++++++++++++++++++++++++++++++++++++++
>> 5 files changed, 145 insertions(+), 2 deletions(-)
>> create mode 100644 validation/constexpr-init.c
>>
>> diff --git a/evaluate.c b/evaluate.c
>> index 70f419f..e3b08e4 100644
>> --- a/evaluate.c
>> +++ b/evaluate.c
>> @@ -2468,6 +2468,7 @@ static void handle_list_initializer(struct expression *expr,
>> {
>> struct expression *e, *last = NULL, *top = NULL, *next;
>> int jumped = 0;
>> + unsigned long old_modifiers;
>>
>> FOR_EACH_PTR(expr->expr_list, e) {
>> struct expression **v;
>> @@ -2522,8 +2523,21 @@ found:
>> else
>> v = &top->ident_expression;
>>
>> - if (handle_simple_initializer(v, 1, lclass, top->ctype))
>> + /*
>> + * Temporarily copy storage modifiers down from
>> + * surrounding type such that
>> + * handle_simple_initializer() can check
>> + * initializations of subobjects with static storage
>> + * duration.
>> + */
>> + old_modifiers = top->ctype->ctype.modifiers;
>> + top->ctype->ctype.modifiers =
>> + old_modifiers | (ctype->ctype.modifiers & MOD_STORAGE);
>> + if (handle_simple_initializer(v, 1, lclass, top->ctype)) {
>> + top->ctype->ctype.modifiers = old_modifiers;
>> continue;
>> + }
>> + top->ctype->ctype.modifiers = old_modifiers;
>
> I don't understand why saving the mods is needed.
> It feels hackish to me. Isn't it because something is done wrongly at another
> level or maybe handle_simple_initializer() need an additional arg or so?
It _is_ hackish and I will change to add an additional argument to
handle_simple_initializer().
>> @@ -2633,6 +2647,16 @@ static int handle_simple_initializer(struct expression **ep, int nested,
> ...
>> + warning(e->pos, "initializer for static storage duration object is not a constant expression");
>
> This is quite longish message.
> What about something like "non-constant initializer"?
That could be misleading:
static const int a = 1;
static const int b = a;
is forbidden, but obiously, 'a' is constant.
I'd like to keep the C99 term "constant expression", as well as
"initializer" and "static".
I could s/storage duration//.
>> diff --git a/lib.c b/lib.c
>> --- a/lib.c
>> +++ b/lib.c
>> @@ -241,6 +241,7 @@ int Wtypesign = 0;
>> int Wundef = 0;
>> int Wuninitialized = 1;
>> int Wvla = 1;
>> +int Wstatic_initializer_not_const = 0;
>
> Here also it's quite longish. Yes I'm a lazy typer :)
> What about simply -Wconst-initializer ?
Josh Triplett wrote in his replies to my RFC series:
Shouldn't it be something like -Wnon-constant-initializer,
since that's what it checks for?
I conclude that we generally want to have -Wwhat-is-checked.
Now, it is the *non*-constant initializers that are being checked for.
Unfortunately, -WnoXXXXXXX seems to get misinterpreted as "switch
XXXXXXX" off by sparse's command line parsing.
In this case "switch n-constant-initializer off".
(I did not verify that by reading code, just by trying it out and
failing, so just a guess).
The -Wstatic-initializer-not-const choice made in the current series is
simply a workaround, any better suggestions welcome!
I'm also fine with -Wstatic-initializer.
Comments?
>
> One thing that could be added (later and in another patch) is to
> set it by default when the C99 variant is selected.
>
>> enum {
>> diff --git a/lib.h b/lib.h
>> index 15b69fa..1b38db2 100644
>> --- a/lib.h
>> +++ b/lib.h
>> @@ -127,7 +127,7 @@ extern int Wtypesign;
>> extern int Wundef;
>> extern int Wuninitialized;
>> extern int Wvla;
>> -
>> +extern int Wstatic_initializer_not_const;
>
> Better to leave the blank line where it was.
Yes, indeed :P.
>
> Luc
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 07/13] evaluate: check static storage duration objects' intializers' constness
2016-01-26 16:08 ` Nicolai Stange
@ 2016-01-26 17:56 ` Luc Van Oostenryck
2016-01-26 20:18 ` Luc Van Oostenryck
0 siblings, 1 reply; 43+ messages in thread
From: Luc Van Oostenryck @ 2016-01-26 17:56 UTC (permalink / raw)
To: Nicolai Stange; +Cc: linux-sparse, Christopher Li, Josh Triplett
On Tue, Jan 26, 2016 at 05:08:16PM +0100, Nicolai Stange wrote:
> Luc Van Oostenryck <luc.vanoostenryck@gmail.com> writes:
> > Better here also to split the patch in two:
> > one add the -W flag flag and another one which will use it.
>
> Introducing a flag without any functionality attached to it feels wrong
> for me. For example, where to update the manpage? Before or after actual
> functionality is introduced?
For me it's fine when it's all part of the same serie.
The update to the manpage should be done when adding the flag,
before using it.
> >> @@ -2633,6 +2647,16 @@ static int handle_simple_initializer(struct expression **ep, int nested,
> > ...
> >> + warning(e->pos, "initializer for static storage duration object is not a constant expression");
> >
> > This is quite longish message.
> > What about something like "non-constant initializer"?
>
> That could be misleading:
>
> static const int a = 1;
> static const int b = a;
>
> is forbidden, but obiously, 'a' is constant.
>
> I'd like to keep the C99 term "constant expression", as well as
> "initializer" and "static".
>
> I could s/storage duration//.
Or "non-constant initializer for static object"?
> >> +int Wstatic_initializer_not_const = 0;
> >
> > Here also it's quite longish. Yes I'm a lazy typer :)
> > What about simply -Wconst-initializer ?
>
> Josh Triplett wrote in his replies to my RFC series:
> Shouldn't it be something like -Wnon-constant-initializer,
> since that's what it checks for?
>
> I conclude that we generally want to have -Wwhat-is-checked.
> Now, it is the *non*-constant initializers that are being checked for.
>
> Unfortunately, -WnoXXXXXXX seems to get misinterpreted as "switch
> XXXXXXX" off by sparse's command line parsing.
> In this case "switch n-constant-initializer off".
> (I did not verify that by reading code, just by trying it out and
> failing, so just a guess).
Yes the code disable warning flags that begin by 'no' or 'no-' .
I see things a bit more loosely: -Wfoobar could means that we will check
and warn something related to 'foobar' but yes it's certainly better
to keep the logic in the direction "warn if 'foobar' is encountered".
> The -Wstatic-initializer-not-const choice made in the current series is
> simply a workaround, any better suggestions welcome!
>
> I'm also fine with -Wstatic-initializer.
>
> Comments?
I dunno.
Better to leave it so for now, I think.
Luc
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 07/13] evaluate: check static storage duration objects' intializers' constness
2016-01-26 17:56 ` Luc Van Oostenryck
@ 2016-01-26 20:18 ` Luc Van Oostenryck
0 siblings, 0 replies; 43+ messages in thread
From: Luc Van Oostenryck @ 2016-01-26 20:18 UTC (permalink / raw)
To: Nicolai Stange; +Cc: linux-sparse, Christopher Li, Josh Triplett
On Tue, Jan 26, 2016 at 06:56:39PM +0100, Luc Van Oostenryck wrote:
> On Tue, Jan 26, 2016 at 05:08:16PM +0100, Nicolai Stange wrote:
>
> > The -Wstatic-initializer-not-const choice made in the current series is
> > simply a workaround, any better suggestions welcome!
> >
> > I'm also fine with -Wstatic-initializer.
> >
> > Comments?
>
> I dunno.
> Better to leave it so for now, I think.
I see that clang has -Wconstexpr-not-const, which is not too long
and still quite explicit and can be used for others things than
initializers.
Luc
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 07/13] evaluate: check static storage duration objects' intializers' constness
2016-01-26 1:42 ` Luc Van Oostenryck
2016-01-26 16:08 ` Nicolai Stange
@ 2016-02-01 3:00 ` Nicolai Stange
1 sibling, 0 replies; 43+ messages in thread
From: Nicolai Stange @ 2016-02-01 3:00 UTC (permalink / raw)
To: Luc Van Oostenryck
Cc: Nicolai Stange, linux-sparse, Christopher Li, Josh Triplett
Luc Van Oostenryck <luc.vanoostenryck@gmail.com> writes:
> On Mon, Jan 25, 2016 at 03:57:45PM +0100, Nicolai Stange wrote:
>> Initializers of static storage duration objects shall be constant
>> expressions [6.7.8(4)].
>>
>> Warn if that requirement is not met and the -Wstatic-initializer-not-const
>> flag has been given on sparse's command line.
>>
>> Identify static storage duration objects by having either of
>> MOD_TOPLEVEL or MOD_STATIC set.
>>
>> Check an initializer's constness at the lowest possible subobject
>> level, i.e. at the level of the "assignment-expression" production
>> in [6.7.8].
>>
>> For compound objects, make handle_list_initializer() pass the
>> surrounding object's storage duration modifiers down to
>> handle_simple_initializer() at subobject initializer evaluation.
>
> Better here also to split the patch in two:
> one add the -W flag flag and another one which will use it.
>
> ch > Signed-off-by: Nicolai Stange <nicstange@gmail.com>
>> ---
>> evaluate.c | 26 ++++++++++-
>> lib.c | 2 +
>> lib.h | 2 +-
>> sparse.1 | 7 +++
>> validation/constexpr-init.c | 110 ++++++++++++++++++++++++++++++++++++++++++++
>> 5 files changed, 145 insertions(+), 2 deletions(-)
>> create mode 100644 validation/constexpr-init.c
>>
>> diff --git a/evaluate.c b/evaluate.c
>> index 70f419f..e3b08e4 100644
>> --- a/evaluate.c
>> +++ b/evaluate.c
>> @@ -2468,6 +2468,7 @@ static void handle_list_initializer(struct expression *expr,
>> {
>> struct expression *e, *last = NULL, *top = NULL, *next;
>> int jumped = 0;
>> + unsigned long old_modifiers;
>>
>> FOR_EACH_PTR(expr->expr_list, e) {
>> struct expression **v;
>> @@ -2522,8 +2523,21 @@ found:
>> else
>> v = &top->ident_expression;
>>
>> - if (handle_simple_initializer(v, 1, lclass, top->ctype))
>> + /*
>> + * Temporarily copy storage modifiers down from
>> + * surrounding type such that
>> + * handle_simple_initializer() can check
>> + * initializations of subobjects with static storage
>> + * duration.
>> + */
>> + old_modifiers = top->ctype->ctype.modifiers;
>> + top->ctype->ctype.modifiers =
>> + old_modifiers | (ctype->ctype.modifiers & MOD_STORAGE);
>> + if (handle_simple_initializer(v, 1, lclass, top->ctype)) {
>> + top->ctype->ctype.modifiers = old_modifiers;
>> continue;
>> + }
>> + top->ctype->ctype.modifiers = old_modifiers;
>
> I don't understand why saving the mods is needed.
> It feels hackish to me. Isn't it because something is done wrongly at another
> level or maybe handle_simple_initializer() need an additional arg or so?
I haven't addressed this one yet, so: giving handle_simple_initializer()
an additional flag won't suffice since it can recursively call into
handle_list_initializer() again. Thus, handle_simple_initializer() would
need an additional argument, too. Plus we'd need a enw entry point
examining the top level modifiers for staticness.
I thought it would be much easier and straight forward to temporarily
flag the elements of an initializer list as static if the parent is in
global context.
The "temporarily" in there is of course argueable...
>> @@ -2633,6 +2647,16 @@ static int handle_simple_initializer(struct expression **ep, int nested,
> ...
>> + warning(e->pos, "initializer for static storage duration object is not a constant expression");
>
> This is quite longish message.
> What about something like "non-constant initializer"?
>
>> diff --git a/lib.c b/lib.c
>> --- a/lib.c
>> +++ b/lib.c
>> @@ -241,6 +241,7 @@ int Wtypesign = 0;
>> int Wundef = 0;
>> int Wuninitialized = 1;
>> int Wvla = 1;
>> +int Wstatic_initializer_not_const = 0;
>
> Here also it's quite longish. Yes I'm a lazy typer :)
> What about simply -Wconst-initializer ?
>
> One thing that could be added (later and in another patch) is to
> set it by default when the C99 variant is selected.
>
>> enum {
>> diff --git a/lib.h b/lib.h
>> index 15b69fa..1b38db2 100644
>> --- a/lib.h
>> +++ b/lib.h
>> @@ -127,7 +127,7 @@ extern int Wtypesign;
>> extern int Wundef;
>> extern int Wuninitialized;
>> extern int Wvla;
>> -
>> +extern int Wstatic_initializer_not_const;
>
> Better to leave the blank line where it was.
>
>
>
> Luc
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 08/13] expression: recognize references to labels as address constants
2016-01-25 14:47 [PATCH v2 00/13] improve constexpr handling Nicolai Stange
` (6 preceding siblings ...)
2016-01-25 14:57 ` [PATCH v2 07/13] evaluate: check static storage duration objects' intializers' constness Nicolai Stange
@ 2016-01-25 14:59 ` Nicolai Stange
2016-01-26 1:45 ` Luc Van Oostenryck
2016-01-25 15:00 ` [PATCH v2 09/13] expression: examine constness of __builtin_offsetof at evaluation only Nicolai Stange
` (5 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Nicolai Stange @ 2016-01-25 14:59 UTC (permalink / raw)
To: linux-sparse
Cc: Nicolai Stange, Christopher Li, Josh Triplett, Luc Van Oostenryck
As an extension, GCC allows labels to be referenced a la
label1:
...
void *ptr = &&label1;
Tag these label references as being address constants allowing them
to be used as initializers for objects of static storage duration.
Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
expression.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/expression.c b/expression.c
index afc4f39..b82a036 100644
--- a/expression.c
+++ b/expression.c
@@ -683,6 +683,7 @@ static struct token *unary_expression(struct token *token, struct expression **t
sym->ctype.modifiers |= MOD_ADDRESSABLE;
add_symbol(&function_computed_target_list, sym);
}
+ expr_set_flag(&label->flags, EXPR_FLAG_ADDR_CONST_EXPR);
label->label_symbol = sym;
*tree = label;
return token->next->next;
--
2.7.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* [PATCH v2 09/13] expression: examine constness of __builtin_offsetof at evaluation only
2016-01-25 14:47 [PATCH v2 00/13] improve constexpr handling Nicolai Stange
` (7 preceding siblings ...)
2016-01-25 14:59 ` [PATCH v2 08/13] expression: recognize references to labels as address constants Nicolai Stange
@ 2016-01-25 15:00 ` Nicolai Stange
2016-01-26 1:57 ` Luc Van Oostenryck
2016-01-25 15:02 ` [PATCH v2 10/13] symbol: flag builtins constant_p, safe_p and warning as constexprs Nicolai Stange
` (4 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Nicolai Stange @ 2016-01-25 15:00 UTC (permalink / raw)
To: linux-sparse
Cc: Nicolai Stange, Christopher Li, Josh Triplett, Luc Van Oostenryck
Currently, the determination of a __builtin_offsetof() expressions'
constness flags is done in two steps:
- Several flags are speculatively set at expression parsing time
- and possibly cleared again at evaluation if the member expression
includes a non-const array index like in
__builtin_offsetof(struct A, a.b[non_const_foo])
For consistency with other expression types' evaluation, defer the
determination of a __builtin_offsetof() expression's constness to
evaluation time, too.
Furthermore, carry an array index expression's constness flags
through the implicit cast to size_t type.
Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
evaluate.c | 13 ++++++++-----
expression.c | 3 ---
validation/constexpr-offsetof.c | 21 +++++++++++++++++++++
3 files changed, 29 insertions(+), 8 deletions(-)
create mode 100644 validation/constexpr-offsetof.c
diff --git a/evaluate.c b/evaluate.c
index e3b08e4..d32f5a4 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -3001,7 +3001,6 @@ static struct symbol *evaluate_offsetof(struct expression *expr)
}
ctype = field;
expr->type = EXPR_VALUE;
- expr->flags = EXPR_FLAG_NONE;
expr_set_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
expr->value = offset;
expr->taint = 0;
@@ -3020,7 +3019,6 @@ static struct symbol *evaluate_offsetof(struct expression *expr)
ctype = ctype->ctype.base_type;
if (!expr->index) {
expr->type = EXPR_VALUE;
- expr->flags = EXPR_FLAG_NONE;
expr_set_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
expr->value = 0;
expr->taint = 0;
@@ -3028,13 +3026,18 @@ static struct symbol *evaluate_offsetof(struct expression *expr)
} else {
struct expression *idx = expr->index, *m;
struct symbol *i_type = evaluate_expression(idx);
+ unsigned old_idx_flags;
int i_class = classify_type(i_type, &i_type);
+
if (!is_int(i_class)) {
expression_error(expr, "non-integer index");
return NULL;
}
unrestrict(idx, i_class, &i_type);
+ old_idx_flags = idx->flags;
idx = cast_to(idx, size_t_ctype);
+ idx->flags |= old_idx_flags;
+ expr_flags_decay_consts(&idx->flags);
m = alloc_const_expression(expr->pos,
bits_to_bytes(ctype->bit_size));
m->ctype = size_t_ctype;
@@ -3045,19 +3048,19 @@ static struct symbol *evaluate_offsetof(struct expression *expr)
expr->op = '*';
expr->ctype = size_t_ctype;
expr->flags = m->flags & idx->flags;
+ expr_flags_decay_consts(&expr->flags);
}
}
if (e) {
struct expression *copy = __alloc_expression(0);
- unsigned char flags_mask = EXPR_FLAG_NONE;
*copy = *expr;
if (e->type == EXPR_OFFSETOF)
e->in = ctype;
if (!evaluate_expression(e))
return NULL;
expr->type = EXPR_BINOP;
- expr_set_flag(&flags_mask, EXPR_FLAG_INT_CONST_EXPR);
- expr->flags = e->flags & copy->flags & flags_mask;
+ expr->flags = e->flags & copy->flags;
+ expr_flags_decay_consts(&expr->flags);
expr->op = '+';
expr->ctype = size_t_ctype;
expr->left = copy;
diff --git a/expression.c b/expression.c
index b82a036..4ecc865 100644
--- a/expression.c
+++ b/expression.c
@@ -199,7 +199,6 @@ static struct token *builtin_offsetof_expr(struct token *token,
return expect(token, ')', "at end of __builtin_offset");
case SPECIAL_DEREFERENCE:
e = alloc_expression(token->pos, EXPR_OFFSETOF);
- expr_set_flag(&e->flags, EXPR_FLAG_INT_CONST_EXPR);
e->op = '[';
*p = e;
p = &e->down;
@@ -207,7 +206,6 @@ static struct token *builtin_offsetof_expr(struct token *token,
case '.':
token = token->next;
e = alloc_expression(token->pos, EXPR_OFFSETOF);
- expr_set_flag(&e->flags, EXPR_FLAG_INT_CONST_EXPR);
e->op = '.';
if (token_type(token) != TOKEN_IDENT) {
sparse_error(token->pos, "Expected member name");
@@ -219,7 +217,6 @@ static struct token *builtin_offsetof_expr(struct token *token,
case '[':
token = token->next;
e = alloc_expression(token->pos, EXPR_OFFSETOF);
- expr_set_flag(&e->flags, EXPR_FLAG_INT_CONST_EXPR);
e->op = '[';
token = parse_expression(token, &e->index);
token = expect(token, ']',
diff --git a/validation/constexpr-offsetof.c b/validation/constexpr-offsetof.c
new file mode 100644
index 0000000..d1697b0
--- /dev/null
+++ b/validation/constexpr-offsetof.c
@@ -0,0 +1,21 @@
+struct A {
+ int a[1];
+ int b;
+};
+
+extern int c;
+
+static int o[] = {
+ [__builtin_offsetof(struct A, b)] = 0, // OK
+ [__builtin_offsetof(struct A, a[0])] = 0, // OK
+ [__builtin_offsetof(struct A, a[0*0])] = 0, // OK
+ [__builtin_offsetof(struct A, a[c])] = 0 // KO
+};
+
+/*
+ * check-name: __builtin_offsetof() constness verification.
+ *
+ * check-error-start
+constexpr-offsetof.c:12:39: error: bad constant expression
+ * check-error-end
+ */
--
2.7.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 09/13] expression: examine constness of __builtin_offsetof at evaluation only
2016-01-25 15:00 ` [PATCH v2 09/13] expression: examine constness of __builtin_offsetof at evaluation only Nicolai Stange
@ 2016-01-26 1:57 ` Luc Van Oostenryck
2016-02-01 3:06 ` Nicolai Stange
0 siblings, 1 reply; 43+ messages in thread
From: Luc Van Oostenryck @ 2016-01-26 1:57 UTC (permalink / raw)
To: Nicolai Stange; +Cc: linux-sparse, Christopher Li, Josh Triplett
On Mon, Jan 25, 2016 at 04:00:48PM +0100, Nicolai Stange wrote:
> Currently, the determination of a __builtin_offsetof() expressions'
> constness flags is done in two steps:
> - Several flags are speculatively set at expression parsing time
> - and possibly cleared again at evaluation if the member expression
> includes a non-const array index like in
> __builtin_offsetof(struct A, a.b[non_const_foo])
>
> For consistency with other expression types' evaluation, defer the
> determination of a __builtin_offsetof() expression's constness to
> evaluation time, too.
>
> Furthermore, carry an array index expression's constness flags
> through the implicit cast to size_t type.
Better to split this into two patches.
> @@ -3028,13 +3026,18 @@ static struct symbol *evaluate_offsetof(struct expression *expr)
> } else {
> struct expression *idx = expr->index, *m;
> struct symbol *i_type = evaluate_expression(idx);
> + unsigned old_idx_flags;
> int i_class = classify_type(i_type, &i_type);
> +
> if (!is_int(i_class)) {
> expression_error(expr, "non-integer index");
> return NULL;
> }
> unrestrict(idx, i_class, &i_type);
> + old_idx_flags = idx->flags;
> idx = cast_to(idx, size_t_ctype);
> + idx->flags |= old_idx_flags;
> + expr_flags_decay_consts(&idx->flags);
> m = alloc_const_expression(expr->pos,
> bits_to_bytes(ctype->bit_size));
> m->ctype = size_t_ctype;
It's not clear at all to me why this is needed.
Why cast_to() can't set itself the right value for idx->flags?
Luc
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 09/13] expression: examine constness of __builtin_offsetof at evaluation only
2016-01-26 1:57 ` Luc Van Oostenryck
@ 2016-02-01 3:06 ` Nicolai Stange
0 siblings, 0 replies; 43+ messages in thread
From: Nicolai Stange @ 2016-02-01 3:06 UTC (permalink / raw)
To: Luc Van Oostenryck
Cc: Nicolai Stange, linux-sparse, Christopher Li, Josh Triplett
Luc Van Oostenryck <luc.vanoostenryck@gmail.com> writes:
> On Mon, Jan 25, 2016 at 04:00:48PM +0100, Nicolai Stange wrote:
>> Currently, the determination of a __builtin_offsetof() expressions'
>> constness flags is done in two steps:
>> - Several flags are speculatively set at expression parsing time
>> - and possibly cleared again at evaluation if the member expression
>> includes a non-const array index like in
>> __builtin_offsetof(struct A, a.b[non_const_foo])
>>
>> For consistency with other expression types' evaluation, defer the
>> determination of a __builtin_offsetof() expression's constness to
>> evaluation time, too.
>>
>> Furthermore, carry an array index expression's constness flags
>> through the implicit cast to size_t type.
>
> Better to split this into two patches.
>
>> @@ -3028,13 +3026,18 @@ static struct symbol *evaluate_offsetof(struct expression *expr)
>> } else {
>> struct expression *idx = expr->index, *m;
>> struct symbol *i_type = evaluate_expression(idx);
>> + unsigned old_idx_flags;
>> int i_class = classify_type(i_type, &i_type);
>> +
>> if (!is_int(i_class)) {
>> expression_error(expr, "non-integer index");
>> return NULL;
>> }
>> unrestrict(idx, i_class, &i_type);
>> + old_idx_flags = idx->flags;
>> idx = cast_to(idx, size_t_ctype);
>> + idx->flags |= old_idx_flags;
>> + expr_flags_decay_consts(&idx->flags);
>> m = alloc_const_expression(expr->pos,
>> bits_to_bytes(ctype->bit_size));
>> m->ctype = size_t_ctype;
>
> It's not clear at all to me why this is needed.
> Why cast_to() can't set itself the right value for idx->flags?
cast_to() is for implicit casts and to be honest, I don't know what the
->flags should be set to.
Also, this is the only place in the whole code where preserving the
flags over a cast_to() call is needed.
Furthermore, we could get rid of the flags' save and restore by moving
the calculation of expr->flags in front of the cast_to() invocation --
all other cast_to() actually do it this way.
However, I think, it's better to have the assignment to expr->flags
grouped with the rest of the assignments to expr's members.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 10/13] symbol: flag builtins constant_p, safe_p and warning as constexprs
2016-01-25 14:47 [PATCH v2 00/13] improve constexpr handling Nicolai Stange
` (8 preceding siblings ...)
2016-01-25 15:00 ` [PATCH v2 09/13] expression: examine constness of __builtin_offsetof at evaluation only Nicolai Stange
@ 2016-01-25 15:02 ` Nicolai Stange
2016-01-26 2:00 ` Luc Van Oostenryck
2016-01-25 15:03 ` [PATCH v2 11/13] evaluate: relax some constant expression rules for pointer expressions Nicolai Stange
` (3 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Nicolai Stange @ 2016-01-25 15:02 UTC (permalink / raw)
To: linux-sparse
Cc: Nicolai Stange, Christopher Li, Josh Triplett, Luc Van Oostenryck
Unconditionally flag the expressions
__builtin_constant_p(),
__builtin_safe_p(),
__builtin_warning()
as being integer constant expressions.
Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
symbol.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/symbol.c b/symbol.c
index 0ceff62..0438dc4 100644
--- a/symbol.c
+++ b/symbol.c
@@ -642,9 +642,10 @@ struct symbol *create_symbol(int stream, const char *name, int type, int namespa
return sym;
}
-static int evaluate_to_integer(struct expression *expr)
+static int evaluate_to_int_const_expr(struct expression *expr)
{
expr->ctype = &int_ctype;
+ expr_set_flag(&expr->flags, EXPR_FLAG_INT_CONST_EXPR);
return 1;
}
@@ -749,17 +750,17 @@ out:
}
static struct symbol_op constant_p_op = {
- .evaluate = evaluate_to_integer,
+ .evaluate = evaluate_to_int_const_expr,
.expand = expand_constant_p
};
static struct symbol_op safe_p_op = {
- .evaluate = evaluate_to_integer,
+ .evaluate = evaluate_to_int_const_expr,
.expand = expand_safe_p
};
static struct symbol_op warning_op = {
- .evaluate = evaluate_to_integer,
+ .evaluate = evaluate_to_int_const_expr,
.expand = expand_warning
};
--
2.7.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 10/13] symbol: flag builtins constant_p, safe_p and warning as constexprs
2016-01-25 15:02 ` [PATCH v2 10/13] symbol: flag builtins constant_p, safe_p and warning as constexprs Nicolai Stange
@ 2016-01-26 2:00 ` Luc Van Oostenryck
0 siblings, 0 replies; 43+ messages in thread
From: Luc Van Oostenryck @ 2016-01-26 2:00 UTC (permalink / raw)
To: Nicolai Stange; +Cc: linux-sparse, Christopher Li, Josh Triplett
On Mon, Jan 25, 2016 at 04:02:06PM +0100, Nicolai Stange wrote:
> Unconditionally flag the expressions
> __builtin_constant_p(),
> __builtin_safe_p(),
> __builtin_warning()
> as being integer constant expressions.
Good!
Luc
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 11/13] evaluate: relax some constant expression rules for pointer expressions
2016-01-25 14:47 [PATCH v2 00/13] improve constexpr handling Nicolai Stange
` (9 preceding siblings ...)
2016-01-25 15:02 ` [PATCH v2 10/13] symbol: flag builtins constant_p, safe_p and warning as constexprs Nicolai Stange
@ 2016-01-25 15:03 ` Nicolai Stange
2016-01-26 2:05 ` Luc Van Oostenryck
2016-01-25 15:04 ` [PATCH v2 12/13] expression, evaluate: support compound literals as address constants Nicolai Stange
` (2 subsequent siblings)
13 siblings, 1 reply; 43+ messages in thread
From: Nicolai Stange @ 2016-01-25 15:03 UTC (permalink / raw)
To: linux-sparse
Cc: Nicolai Stange, Christopher Li, Josh Triplett, Luc Van Oostenryck
The official constraints on constant expressions [6.6] are insanely
strict in that they do not allow some constructs commonly used in the
wild.
Relax them by treating
- address constants cast to different pointer type as address constants
again,
- address constants cast to integer type as integer constant
expressions
- conditional expressions whose true and false branches both yield
address constants as address constants,
- and conditional expressions whose condition is an address constant
as an constant expression to the extent their true and false branches
allow.
Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
evaluate.c | 35 ++++++++++++++++++++++++++++++-----
1 file changed, 30 insertions(+), 5 deletions(-)
diff --git a/evaluate.c b/evaluate.c
index d32f5a4..2b60294 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -1125,14 +1125,23 @@ static struct symbol *evaluate_conditional_expression(struct expression *expr)
true = &expr->cond_true;
}
- expr->flags = (expr->conditional->flags & (*true)->flags &
- expr->cond_false->flags);
- expr_flags_decay_consts(&expr->flags);
/*
+ * A conditional operator yields a particular constant
+ * expression type only if all of its three subexpressions are
+ * of that type [6.6(6), 6.6(8)].
+ * As an extension, relax this restriction by allowing any
+ * constant expression type for the condition expression.
+ *
* A conditional operator never yields an address constant
* [6.6(9)].
+ * However, as an extension, if the condition is any constant
+ * expression, and the true and false expressions are both
+ * address constants, mark the result as an address constant.
*/
- expr_clear_flag(&expr->flags, EXPR_FLAG_ADDR_CONST_EXPR);
+ if (expr->conditional->flags & (EXPR_FLAG_ARITH_CONST_EXPR |
+ EXPR_FLAG_ADDR_CONST_EXPR))
+ expr->flags = (*true)->flags &expr->cond_false->flags;
+ expr_flags_decay_consts(&expr->flags);
lclass = classify_type(ltype, <ype);
rclass = classify_type(rtype, &rtype);
@@ -2783,14 +2792,30 @@ static struct symbol *evaluate_cast(struct expression *expr)
/*
* Casts of integer literals to pointer type yield
* address constants [6.6(9)].
+ *
+ * As an extension, treat address constants cast to a
+ * different pointer type as adress constants again.
+ *
+ * As another extension, treat integer constant
+ * expressions (in contrast to literals) cast to
+ * pointer type as address constants.
*/
if (class1 & TYPE_PTR &&
- (target->flags & EXPR_FLAG_INT_CONST)) {
+ ((target->flags & EXPR_FLAG_INT_CONST_EXPR) ||
+ (target->flags & EXPR_FLAG_ADDR_CONST_EXPR))) {
expr_set_flag(&expr->flags, EXPR_FLAG_ADDR_CONST_EXPR);
+
}
} else {
expr->flags = target->flags;
expr_flags_decay_consts(&expr->flags);
+
+ /*
+ * As an extension, treat address constants cast to
+ * integer type as an arithmetic constant.
+ */
+ if (expr->flags & EXPR_FLAG_ADDR_CONST_EXPR)
+ expr_set_flag(&expr->flags, EXPR_FLAG_ARITH_CONST_EXPR);
/*
* Casts to numeric types never result in address
* constants [6.6(9)].
--
2.7.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 11/13] evaluate: relax some constant expression rules for pointer expressions
2016-01-25 15:03 ` [PATCH v2 11/13] evaluate: relax some constant expression rules for pointer expressions Nicolai Stange
@ 2016-01-26 2:05 ` Luc Van Oostenryck
0 siblings, 0 replies; 43+ messages in thread
From: Luc Van Oostenryck @ 2016-01-26 2:05 UTC (permalink / raw)
To: Nicolai Stange; +Cc: linux-sparse, Christopher Li, Josh Triplett
On Mon, Jan 25, 2016 at 04:03:26PM +0100, Nicolai Stange wrote:
> The official constraints on constant expressions [6.6] are insanely
> strict in that they do not allow some constructs commonly used in the
> wild.
>
> Relax them by treating
> - address constants cast to different pointer type as address constants
> again,
> - address constants cast to integer type as integer constant
> expressions
> - conditional expressions whose true and false branches both yield
> address constants as address constants,
> - and conditional expressions whose condition is an address constant
> as an constant expression to the extent their true and false branches
> allow.
> @@ -2783,14 +2792,30 @@ static struct symbol *evaluate_cast(struct expression *expr)
> /*
> * Casts of integer literals to pointer type yield
> * address constants [6.6(9)].
> + *
> + * As an extension, treat address constants cast to a
> + * different pointer type as adress constants again.
Typo: s/adress/address/
Good otherwise.
Luc
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 12/13] expression, evaluate: support compound literals as address constants
2016-01-25 14:47 [PATCH v2 00/13] improve constexpr handling Nicolai Stange
` (10 preceding siblings ...)
2016-01-25 15:03 ` [PATCH v2 11/13] evaluate: relax some constant expression rules for pointer expressions Nicolai Stange
@ 2016-01-25 15:04 ` Nicolai Stange
2016-01-26 2:07 ` Luc Van Oostenryck
2016-01-25 15:05 ` [PATCH v2 13/13] symbol: do not inherit storage modifiers from base types at examination Nicolai Stange
2016-01-25 21:01 ` [PATCH v2 00/13] improve constexpr handling Luc Van Oostenryck
13 siblings, 1 reply; 43+ messages in thread
From: Nicolai Stange @ 2016-01-25 15:04 UTC (permalink / raw)
To: linux-sparse
Cc: Nicolai Stange, Christopher Li, Josh Triplett, Luc Van Oostenryck
Toplevel compound literals have got static storage duration
[6.5.2.5(6)].
This implies that
1. their addresses are address constants [6.6(9)] and
2. their initializers must contain constant expressions only
[6.5.2.5(3), 6.7.8(4)] .
Flag the anonymous symbol created at expression parsing time as having
static storage duration if the compound literal occurs at top level
scope.
Flag the whole expression as being an address constant at evaluation
time if its corresponding anonymous symbol had been previously marked
as having static storage duration.
Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
evaluate.c | 2 ++
expression.c | 2 ++
validation/constexpr-compound-literal.c | 19 +++++++++++++++++++
3 files changed, 23 insertions(+)
create mode 100644 validation/constexpr-compound-literal.c
diff --git a/evaluate.c b/evaluate.c
index 2b60294..ec19fe4 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -2770,6 +2770,8 @@ static struct symbol *evaluate_cast(struct expression *expr)
addr->ctype = &lazy_ptr_ctype; /* Lazy eval */
addr->symbol = sym;
+ if (sym->ctype.modifiers & MOD_TOPLEVEL)
+ expr_set_flag(&addr->flags, EXPR_FLAG_ADDR_CONST_EXPR);
expr->type = EXPR_PREOP;
expr->op = '*';
diff --git a/expression.c b/expression.c
index 4ecc865..789353a 100644
--- a/expression.c
+++ b/expression.c
@@ -713,6 +713,8 @@ static struct token *cast_expression(struct token *token, struct expression **tr
cast->cast_type = sym;
token = expect(token, ')', "at end of cast operator");
if (match_op(token, '{')) {
+ if (toplevel(block_scope))
+ sym->ctype.modifiers |= MOD_TOPLEVEL;
if (is_force)
warning(sym->pos,
"[force] in compound literal");
diff --git a/validation/constexpr-compound-literal.c b/validation/constexpr-compound-literal.c
new file mode 100644
index 0000000..0892183
--- /dev/null
+++ b/validation/constexpr-compound-literal.c
@@ -0,0 +1,19 @@
+static int *a = &(int){ 1 }; // OK
+static int *b = &(int){ *a }; // KO
+
+static void foo(void)
+{
+ int *b = &(int){ 1 }; // OK
+ int *c = &(int){ *a }; // OK
+ static int *d = &(int){ 1 }; // KO
+}
+
+/*
+ * check-name: compound literal address constness verification
+ * check-command: sparse -Wstatic-initializer-not-const $file
+ *
+ * check-error-start
+constexpr-compound-literal.c:2:25: warning: initializer for static storage duration object is not a constant expression
+constexpr-compound-literal.c:8:27: warning: initializer for static storage duration object is not a constant expression
+ * check-error-end
+ */
--
2.7.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 12/13] expression, evaluate: support compound literals as address constants
2016-01-25 15:04 ` [PATCH v2 12/13] expression, evaluate: support compound literals as address constants Nicolai Stange
@ 2016-01-26 2:07 ` Luc Van Oostenryck
0 siblings, 0 replies; 43+ messages in thread
From: Luc Van Oostenryck @ 2016-01-26 2:07 UTC (permalink / raw)
To: Nicolai Stange; +Cc: linux-sparse, Christopher Li, Josh Triplett
On Mon, Jan 25, 2016 at 04:04:28PM +0100, Nicolai Stange wrote:
> Toplevel compound literals have got static storage duration
> [6.5.2.5(6)].
>
> This implies that
> 1. their addresses are address constants [6.6(9)] and
> 2. their initializers must contain constant expressions only
> [6.5.2.5(3), 6.7.8(4)] .
>
> Flag the anonymous symbol created at expression parsing time as having
> static storage duration if the compound literal occurs at top level
> scope.
>
> Flag the whole expression as being an address constant at evaluation
> time if its corresponding anonymous symbol had been previously marked
> as having static storage duration.
Good.
Luc
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 13/13] symbol: do not inherit storage modifiers from base types at examination
2016-01-25 14:47 [PATCH v2 00/13] improve constexpr handling Nicolai Stange
` (11 preceding siblings ...)
2016-01-25 15:04 ` [PATCH v2 12/13] expression, evaluate: support compound literals as address constants Nicolai Stange
@ 2016-01-25 15:05 ` Nicolai Stange
2016-01-26 2:54 ` Luc Van Oostenryck
2016-01-25 21:01 ` [PATCH v2 00/13] improve constexpr handling Luc Van Oostenryck
13 siblings, 1 reply; 43+ messages in thread
From: Nicolai Stange @ 2016-01-25 15:05 UTC (permalink / raw)
To: linux-sparse
Cc: Nicolai Stange, Christopher Li, Josh Triplett, Luc Van Oostenryck
Consider the following code snippet:
static inline foo(int dummy, ...) {}
static int a = 0;
static void bar(void)
{
foo(0, a);
}
Sparse moans:
test.c:5:9: warning: initializer for static storage duration object
is not a constant expression
The cause can be tracked down as follows:
The anonymous node created by inline_function() for the variadic
argument will get assigned to its base_type whatever the passed
expression's ctype is. For the special case of a primary expression
referencing a symbol, this ctype is the referenced symbol itself.
Furthermore, inline_function() sets that symbol node's initializer
to this expression.
Now, when the anonymous symbol node is evaluated, its base_type is
handled in examine_base_type(). This applies the base_type's modifiers,
i.e. the referenced symbol's MOD_STATIC in this case, to the inheriting
ctype, that of the anonymous node, itself.
This in turn instructs the evaluation of the symbol's initializer to
allow constant expressions only.
Do not inherit a base_type's storage related modifiers in
examine_base_type().
Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
symbol.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/symbol.c b/symbol.c
index 0438dc4..5c94dc2 100644
--- a/symbol.c
+++ b/symbol.c
@@ -214,7 +214,8 @@ static struct symbol *examine_base_type(struct symbol *sym)
if (!base_type || base_type->type == SYM_PTR)
return base_type;
sym->ctype.as |= base_type->ctype.as;
- sym->ctype.modifiers |= base_type->ctype.modifiers & MOD_PTRINHERIT;
+ sym->ctype.modifiers |= base_type->ctype.modifiers & MOD_PTRINHERIT &
+ ~MOD_STORAGE;
concat_ptr_list((struct ptr_list *)base_type->ctype.contexts,
(struct ptr_list **)&sym->ctype.contexts);
if (base_type->type == SYM_NODE) {
--
2.7.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH v2 13/13] symbol: do not inherit storage modifiers from base types at examination
2016-01-25 15:05 ` [PATCH v2 13/13] symbol: do not inherit storage modifiers from base types at examination Nicolai Stange
@ 2016-01-26 2:54 ` Luc Van Oostenryck
0 siblings, 0 replies; 43+ messages in thread
From: Luc Van Oostenryck @ 2016-01-26 2:54 UTC (permalink / raw)
To: Nicolai Stange
Cc: linux-sparse, Christopher Li, Josh Triplett, Linus Torvalds
On Mon, Jan 25, 2016 at 04:05:41PM +0100, Nicolai Stange wrote:
> Consider the following code snippet:
> static inline foo(int dummy, ...) {}
> static int a = 0;
> static void bar(void)
> {
> foo(0, a);
> }
>
> Sparse moans:
> test.c:5:9: warning: initializer for static storage duration object
> is not a constant expression
>
> The cause can be tracked down as follows:
> The anonymous node created by inline_function() for the variadic
> argument will get assigned to its base_type whatever the passed
> expression's ctype is. For the special case of a primary expression
> referencing a symbol, this ctype is the referenced symbol itself.
> Furthermore, inline_function() sets that symbol node's initializer
> to this expression.
>
> Now, when the anonymous symbol node is evaluated, its base_type is
> handled in examine_base_type(). This applies the base_type's modifiers,
> i.e. the referenced symbol's MOD_STATIC in this case, to the inheriting
> ctype, that of the anonymous node, itself.
> This in turn instructs the evaluation of the symbol's initializer to
> allow constant expressions only.
>
> Do not inherit a base_type's storage related modifiers in
> examine_base_type().
For this one ...
there is a lot to say about, not about your patch but about what should be done.
It should be noted that the warning only occurs because:
1) the function is inlined.
2) the function is variadic and the symbol is given in the variable part.
3) the symbol doesn't need integer promotion.
4) MOD_STORAGE is part of MOD_PTRINHERIT.
It would be good to show these conditions in the test case.
What I see in the code about calls is that when conditions 1, 2 & 3 are met
the argument to this variadic inline function is sorta used as-it-is while,
I think, a sort of l-value to r-value conversion should be done.
This conversion would essentially just adapting some of the modifiers:
clearing MOD_STORAGE (and maybe MOD_ADDRESSABLE).
For the condition 4), while digging in the code history (commit 822d3b1a222b),
it can be seen that MOD_STORAGE was added to MOD_PTRINHERIT without a real need;
quoting Linus:
This still keeps other attributes, like "pointer to 'static'". Useful?
Probably not. I'll have to think about it.
Normally it's not a problem to have it because unneeded modifiers are generally
filtered out or simply ignored.
So there is several ways to "fix" (or at least hide) this problem but it's not
clear to me what really should be done.
I added Linus in CC, his opinion on the subject would be precious.
NB. While looking at this one I've noted several other problems with modifier
inheritance. I'll send an RFC in the coming hours or days.
Cheers,
Luc
> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
> ---
> symbol.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/symbol.c b/symbol.c
> index 0438dc4..5c94dc2 100644
> --- a/symbol.c
> +++ b/symbol.c
> @@ -214,7 +214,8 @@ static struct symbol *examine_base_type(struct symbol *sym)
> if (!base_type || base_type->type == SYM_PTR)
> return base_type;
> sym->ctype.as |= base_type->ctype.as;
> - sym->ctype.modifiers |= base_type->ctype.modifiers & MOD_PTRINHERIT;
> + sym->ctype.modifiers |= base_type->ctype.modifiers & MOD_PTRINHERIT &
> + ~MOD_STORAGE;
> concat_ptr_list((struct ptr_list *)base_type->ctype.contexts,
> (struct ptr_list **)&sym->ctype.contexts);
> if (base_type->type == SYM_NODE) {
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 00/13] improve constexpr handling
2016-01-25 14:47 [PATCH v2 00/13] improve constexpr handling Nicolai Stange
` (12 preceding siblings ...)
2016-01-25 15:05 ` [PATCH v2 13/13] symbol: do not inherit storage modifiers from base types at examination Nicolai Stange
@ 2016-01-25 21:01 ` Luc Van Oostenryck
2016-01-25 21:26 ` Nicolai Stange
13 siblings, 1 reply; 43+ messages in thread
From: Luc Van Oostenryck @ 2016-01-25 21:01 UTC (permalink / raw)
To: Nicolai Stange; +Cc: linux-sparse, Christopher Li, Josh Triplett
On Mon, Jan 25, 2016 at 03:47:51PM +0100, Nicolai Stange wrote:
> This is the second attempt to clean up and improve sparse's handling
> of constant expressions. After I got some helpful reviews from
> Josh Triplett and Luc Van Oostenryck on my initial RFC series, I feel
> quite comfortable with this now and dropped the 'RFC' tag in favour of
> 'v2'.
Great.
> Quote from my initial 'RFC' cover letter regarding the structure of
> this series:
>
> This patch series is split into four parts:
> - The first part deals with the refactorization of the current integer
> constant expression handling and introduces some support for
> recognizing arithmetic expressions. [1-5/13]
> - The second part introduces support for recognizing address constants.
> [6/13]
> - The third part introduces a check for the constness of static storage
> duration objects' initializers. [7/13]
> - The last part stems from my tests with the kernel. It contains things
> I missed in the first [9-10/13] and second [8,12/13] parts and
> relaxes some of the constraints on constant expressions [11/13].
> For the last patch [13/13], please see below.
> [...]
> Although the patches of the fourth part, the fixup part, fit very well
> into the first two categories, their associated testcases, if any,
> depend on [7/13]. Thus, I dediced to keep the order as is.
Yes, it's fine. certainly so since you now added the -W flag.
I really consider your [13/13] as a totally separate patch
but it needs this series to see its effect.
> Quote end.
>
>
> The question from the initial 'RFC' series whether or not to relax the
> constexpr constraints, meaning that
>
> a difference of address constants may yield an integer constant
>
> in order to make the kernel's ACPI_OFFSET macro happy, is still
> unaddressed. However, if it turns out that we actually want to do so,
> this single issue can be easily handled by some follow up patch.
Yes, indeed.
Luc
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH v2 00/13] improve constexpr handling
2016-01-25 21:01 ` [PATCH v2 00/13] improve constexpr handling Luc Van Oostenryck
@ 2016-01-25 21:26 ` Nicolai Stange
0 siblings, 0 replies; 43+ messages in thread
From: Nicolai Stange @ 2016-01-25 21:26 UTC (permalink / raw)
To: Luc Van Oostenryck
Cc: Nicolai Stange, linux-sparse, Christopher Li, Josh Triplett
Hi Luc,
thank you once again!
Luc Van Oostenryck <luc.vanoostenryck@gmail.com> writes:
> On Mon, Jan 25, 2016 at 03:47:51PM +0100, Nicolai Stange wrote:
>> This is the second attempt to clean up and improve sparse's handling
>> of constant expressions. After I got some helpful reviews from
>> Josh Triplett and Luc Van Oostenryck on my initial RFC series, I feel
>> quite comfortable with this now and dropped the 'RFC' tag in favour of
>> 'v2'.
>
> Great.
>
>> Quote from my initial 'RFC' cover letter regarding the structure of
>> this series:
>>
>> This patch series is split into four parts:
>> - The first part deals with the refactorization of the current integer
>> constant expression handling and introduces some support for
>> recognizing arithmetic expressions. [1-5/13]
>> - The second part introduces support for recognizing address constants.
>> [6/13]
>> - The third part introduces a check for the constness of static storage
>> duration objects' initializers. [7/13]
>> - The last part stems from my tests with the kernel. It contains things
>> I missed in the first [9-10/13] and second [8,12/13] parts and
>> relaxes some of the constraints on constant expressions [11/13].
>> For the last patch [13/13], please see below.
>> [...]
>> Although the patches of the fourth part, the fixup part, fit very well
>> into the first two categories, their associated testcases, if any,
>> depend on [7/13]. Thus, I dediced to keep the order as is.
>
> Yes, it's fine. certainly so since you now added the -W flag.
> I really consider your [13/13] as a totally separate patch
> but it needs this series to see its effect.
You're right, it somehow feels like it could go separately. However, it
is strictly needed in this form or another in order to avoid false
positives with -Wstatic-initializer-not-const (see the commit message of
[13/13] for an example). Because of that, I'd prefer to keep it with
this series.
>> Quote end.
>>
>>
>> The question from the initial 'RFC' series whether or not to relax the
>> constexpr constraints, meaning that
>>
>> a difference of address constants may yield an integer constant
>>
>> in order to make the kernel's ACPI_OFFSET macro happy, is still
>> unaddressed. However, if it turns out that we actually want to do so,
>> this single issue can be easily handled by some follow up patch.
>
> Yes, indeed.
>
>
> Luc
^ permalink raw reply [flat|nested] 43+ messages in thread