improve constexpr handling

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.

expression.c +69 -0

+
+/*
+ * 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;
+}
+
+/*

Quoted 2 lines · Nicolai Stange

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

Quoted 7 lines

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.

Quoted 6 lines

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 */

Quoted 8 lines

'(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

Quoted 7 lines

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

expression.c static struct token *parse_type(struct token *token, struct expression **tree) · line 134

Quoted 9 lines

expression.c +1 -1

@@ -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?

expression.c struct token *primary_expression(struct token *token, struct expression **tree) · line 463

Quoted 7 lines

expression.c +2 -1

@@ -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'

Quoted 69 lines

expression.c +69 -0

+
+/*
+ * 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;
+}
+
+/*

expression.c struct token *primary_expression(struct token *token, struct expression **tree)

Quoted 10 lines

expression.c +10 -0

+ * 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 '&'.

Quoted 4 lines

enum {

expression.c +0 -0

 	Taint_comma = 1,

@@ -77,7 +188,7 @@ enum {
 

expression.c enum { · line 191

Quoted 4 lines

expression.c +1 -1

 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

Quoted 5 lines · Luc Van Oostenryck

Quoted 2 lines · Nicolai Stange

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.

Quoted 10 lines

Quoted 7 lines

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.

Quoted 15 lines

Quoted 6 lines

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 */

Quoted 8 lines

'(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.

Quoted 1 line

This could be moved to the top of the patch description

Yes.

Quoted 8 lines

Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 12 lines

expression.c static struct token *parse_type(struct token *token, struct expression **tree) · line 134

Quoted 9 lines

expression.c +1 -1

@@ -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?

Quoted 9 lines

expression.c struct token *primary_expression(struct token *token, struct expression **tree) · line 463

Quoted 7 lines

expression.c +2 -1

@@ -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.

Quoted 88 lines

Quoted 69 lines

expression.c +69 -0

+
+/*
+ * 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;
+}
+
+/*

expression.c struct token *primary_expression(struct token *token, struct expression **tree)

Quoted 10 lines

expression.c +10 -0

+ * 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;

Quoted 8 lines

- 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) ?

Quoted 5 lines

Quoted 5 lines

enum {

expression.c +0 -0

 	Taint_comma = 1,

@@ -77,7 +188,7 @@ enum {
 

Quoted 9 lines

expression.c enum { · line 191

Quoted 4 lines

expression.c +1 -1

 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].

Quoted 3 lines

Luc

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 <[email protected]>

---
 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

evaluate.c +30 -12

index 0cf85ae..11917ee 100644
@@ -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

expression.c +0 -19

index eccdb5a..33f4581 100644
@@ -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;
 		}
 	}

validation/constexpr-cast.c new file +25 -0

new file mode 100644
index 0000000..2706961
@@ -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
+ */

Signature · 6 lines

2.7.0

--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 17 lines · Nicolai Stange

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.

Quoted 2 lines

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

Quoted 136 lines

Signed-off-by Nicolai Stange <[email protected]>

---
 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

evaluate.c +30 -12

index 0cf85ae..11917ee 100644
@@ -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

expression.c +0 -19

index eccdb5a..33f4581 100644
@@ -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;
 		}
 	}

validation/constexpr-cast.c new file +25 -0

new file mode 100644
index 0000000..2706961
@@ -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
+ */

Signature · 6 lines

2.7.0

--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.

evaluate.c +0 -1

@@ -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) {

Quoted 14 lines · Nicolai Stange

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.

Quoted 5 lines

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.

Quoted 6 lines

evaluate.c +0 -1

@@ -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) {

evaluate.c static struct symbol *evaluate_binop(struct expression *expr) · line 898

Quoted 10 lines

evaluate.c +3 -5

-		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?

validation/constexpr-binop.c line 9

Quoted 11 lines

validation/constexpr-binop.c +9 -0

@@ -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.

validation/constexpr-binop.c line 14

Quoted 2 lines

validation/constexpr-binop.c +1 -0

+	[0 && 0.] = 0,						// KO

Same here.

Luc

Quoted 6 lines · Luc Van Oostenryck

validation/constexpr-binop.c line 9

Quoted 1 line · Nicolai Stange

validation/constexpr-binop.c new file +1 -0

+	[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.

Quoted 11 lines · Nicolai Stange

Quoted 6 lines · Luc Van Oostenryck

validation/constexpr-binop.c line 9

Quoted 1 line · Nicolai Stange

validation/constexpr-binop.c new file +1 -0

+	[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.

Quoted 3 lines

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 Van Oostenryck <[email protected]> writes:

Quoted 4 lines

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.

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.

Quoted 4 lines

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...

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.

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.

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().

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.

---
 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

evaluate.c +6 -1

index 70f419f..e3b08e4 100644
@@ -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

evaluate.c +0 -0

@@ -2633,6 +2647,16 @@ static int handle_simple_initializer(struct expression **ep, int nested,

Quoted 13 lines · Nicolai Stange

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].

Quoted 3 lines

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 <[email protected]>

Quoted 37 lines

---
 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

evaluate.c +12 -1

index 70f419f..e3b08e4 100644
@@ -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)) {

evaluate.c found: · line 2540

Quoted 4 lines

evaluate.c +3 -0

+			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?

Quoted 1 line

evaluate.c +0 -0

@@ -2633,6 +2647,16 @@ static int handle_simple_initializer(struct expression **ep, int nested,

...

evaluate.c static int handle_simple_initializer(struct expression **ep, int nested, · line 2658

Quoted 1 line

evaluate.c +1 -0

+			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"?

lib.c int Wtypesign = 0; · line 244

Quoted 8 lines

lib.c +1 -0

@@ -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.

lib.h extern int Wtypesign; · line 130

Quoted 11 lines

lib.c +0 -0

 enum {

lib.h +1 -1

index 15b69fa..1b38db2 100644
@@ -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

Quoted 2 lines · Luc Van Oostenryck

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?

Quoted 6 lines

Quoted 1 line

(quoted diff) +0 -0

@@ -2633,6 +2647,16 @@ static int handle_simple_initializer(struct expression **ep, int nested,

...

evaluate.c static int handle_simple_initializer(struct expression **ep, int nested, · line 2658

Quoted 1 line

evaluate.c +1 -0

+			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//.

Quoted 4 lines

Quoted 1 line

+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?

Quoted 6 lines · Nicolai Stange

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.

Quoted 9 lines · Nicolai Stange

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.

Quoted 7 lines

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 <[email protected]>

Quoted 37 lines

---
 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

evaluate.c +12 -1

index 70f419f..e3b08e4 100644
@@ -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)) {

evaluate.c found: · line 2540

Quoted 4 lines

evaluate.c +3 -0

+			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?

Quoted 1 line

evaluate.c +0 -0

@@ -2633,6 +2647,16 @@ static int handle_simple_initializer(struct expression **ep, int nested,

...

evaluate.c static int handle_simple_initializer(struct expression **ep, int nested, · line 2658

Quoted 1 line

evaluate.c +1 -0

+			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"?

lib.c int Wtypesign = 0; · line 244

Quoted 8 lines

lib.c +1 -0

@@ -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.

lib.h extern int Wtypesign; · line 130

Quoted 11 lines

lib.c +0 -0

 enum {

lib.h +1 -1

index 15b69fa..1b38db2 100644
@@ -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

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.

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.

evaluate.c +2 -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");

Quoted 7 lines · Nicolai Stange

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])

Quoted 6 lines

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.

Quoted 14 lines

evaluate.c +3 -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);

evaluate.c static struct symbol *evaluate_offsetof(struct expression *expr) · line 3043

Quoted 5 lines

evaluate.c +2 -0

+			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?

Unconditionally flag the expressions
__builtin_constant_p(),
__builtin_safe_p(),
__builtin_warning()
as being integer constant expressions.

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.

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.

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 <[email protected]>

---
 symbol.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

symbol.c +2 -1

index 0438dc4..5c94dc2 100644
@@ -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) {

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'.

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.

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.

Quoted 5 lines · Nicolai Stange

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.

Quoted 9 lines

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]

Quoted 10 lines

- 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.

Quoted 11 lines

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