Re: [PATCH] Change the default for Mixed declarations.

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Feb 14, 2023 at 05:07:38PM +0100, Juan Quintela wrote:
>> Hi
>> 
>> I want to enter a discussion about changing the default of the style
>> guide.
>> 
>> There are several reasons for that:
>> - they exist since C99 (i.e. all supported compilers support them)
>> - they eliminate the posibility of an unitialized variable.
>
> Actually they don't do that reliably. In fact, when combined
> with usage of 'goto', they introduce uninitialized variables,
> despite the declaration having an initialization present, and
> thus actively mislead reviewers into thinking their code is
> safe.
>
> Consider this example:

[...]

> What happens is that when you 'goto $LABEL' across a variable
> declaration, the variable is in scope at your target label, but
> its declared initializers never get run :-(
>
> Luckily you can protect against that with gcc:
>
> $ gcc -Wjump-misses-init -Wall -o mixed mixed.c
> mixed.c: In function ‘foo’:
> mixed.c:7:12: warning: jump skips variable initialization [-Wjump-misses-init]
>     7 |            goto cleanup;
>       |            ^~~~
> mixed.c:15:5: note: label ‘cleanup’ defined here
>    15 |     cleanup:
>       |     ^~~~~~~
> mixed.c:11:13: note: ‘items’ declared here
>    11 |        int *items = malloc(sizeof(int) *nitems);
>       |             ^~~~~
> mixed.c:7:12: warning: jump skips variable initialization [-Wjump-misses-init]
>     7 |            goto cleanup;
>       |            ^~~~
> mixed.c:15:5: note: label ‘cleanup’ defined here
>    15 |     cleanup:
>       |     ^~~~~~~
> mixed.c:10:12: note: ‘nitems’ declared here
>    10 |        int nitems = 3;
>       |            ^~~~~~
>
>
> however that will warn about *all* cases where we jump over a
> declared variable, even if the variable we're jumping over is
> not used at the target label location. IOW, it has significant
> false positive rates. There are quite a few triggers for this
> in the QEMU code already if we turn on this warning.
>
> It also doesn't alter that the code initialization is misleading
> to read.

Yup.  Strong dislike.

>> - (at least for me), declaring the index inside the for make clear
>>   that index is not used outside the for.
>
> I'll admit that declaring loop indexes in the for() is a nice
> bit, but I'm not a fan in general of mixing the declarations
> in the middle of code for projects that use the 'goto cleanup'
> pattern.

A declaration in a for statement's first operand is effectively at the
beginning of a block.  Therefore, use of this feature is already
sanctioned by the QEMU Coding Style.  The proposed patch at most
clarifies this.

>> - Current documentation already declares that they are allowed in some
>>   cases.
>> - Lots of places already use them.
>> 
>> We can change the text to whatever you want, just wondering if it is
>> valib to change the standard.
>> 
>> Doing a trivial grep through my local qemu messages (around 100k) it
>> shows that some people are complaining that they are not allowed, and
>> other saying that they are used all over the place.
>
> IMHO the status quo is bad because it is actively dangerous when
> combined with goto and we aren't using any compiler warnings to
> help us.
>
> Either we allow it, but use -Wjump-misses-init to prevent mixing
> delayed declarations with gotos, and just avoid this when it triggers
> a false positive.
>
> Or we forbid it, rewrite current cases that use it, and then add
> -Wdeclaration-after-statement to enforce it.

I'm in favour of -Wdeclaration-after-statement.

> IMHO if we are concerned about uninitialized variables then I think
> a better approach is to add -ftrivial-auto-var-init=zero, which will
> make the compiler initialize all variables to 0 if they lack an
> explicit initializer. 

How often do we get bitten by uninitialized variables despite
-Wmaybe-uninitialized?  Honest question!

>> Discuss.