Nicolai Stange <nicstange@xxxxxxxxx> writes: > Luc Van Oostenryck <luc.vanoostenryck@xxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html