Luc Van Oostenryck <luc.vanoostenryck@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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
--
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
