Re: [PATCH] docs/style: allow C99 mixed declarations

[Markus Armbruster]

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

> On Mon, Feb 05, 2024 at 12:18:19PM -0500, Stefan Hajnoczi wrote:
>> C99 mixed declarations support interleaving of local variable
>> declarations and code.
>> 
>> The coding style "generally" forbids C99 mixed declarations with some
>> exceptions to the rule. This rule is not checked by checkpatch.pl and
>> naturally there are violations in the source tree.
>> 
>> While contemplating adding another exception, I came to the conclusion
>> that the best location for declarations depends on context. Let the
>> programmer declare variables where it is best for legibility. Don't try
>> to define all possible scenarios/exceptions.
>
> IIRC, we had a discussion on this topic sometime last year, but can't
> remember what the $SUBJECT was, so I'll just repeat what I said then.

From: Juan Quintela <quintela@redhat.com>
Subject: [PATCH] Change the default for Mixed declarations.
Date: Tue, 14 Feb 2023 17:07:38 +0100
Message-Id: <20230214160738.88614-1-quintela@redhat.com>
https://lore.kernel.org/qemu-devel/20230214160738.88614-1-quintela@redhat.com/

> Combining C99 mixed declarations with 'goto' is dangerous.
>
> Consider this program:
>
> $ cat jump.c
> #include <stdlib.h>
>
> int main(int argc, char**argv) {
>
>   if (getenv("SKIP"))
>     goto target;
>
>   char *foo = malloc(30);
>
>  target:
>   free(foo);
> }
>
> $ gcc -Wall -Wuninitialized -o jump jump.c
>
> $ SKIP=1 ./jump 
> free(): invalid pointer
> Aborted (core dumped)
>
>
>  -> The programmer thinks they have initialized 'foo'
>  -> GCC thinks the programmer has initialized 'foo'
>  -> Yet 'foo' is not guaranteed to be initialized at 'target:'
>
> Given that QEMU makes heavy use of 'goto', allowing C99 mixed
> declarations exposes us to significant danger.
>
> Full disclosure, GCC fails to diagnmose this mistake, even
> with a decl at start of 'main', but at least the mistake is
> now more visible to the programmer.
>
> Fortunately with -fanalyzer GCC can diagnose this:
>
> $ gcc -fanalyzer -Wall -o jump jump.c
> jump.c: In function ‘main’:
> jump.c:12:3: warning: use of uninitialized value ‘foo’ [CWE-457] 
> [-Wanalyzer-use-of-uninitialized-value]
>    12 |   free(foo);
>       |   ^~~~~~~~~
>   ‘main’: events 1-5
>     |
>     |    6 |   if (getenv("SKIP"))
>     |      |      ~  
>     |      |      |
>     |      |      (3) following ‘true’ branch...
>     |    7 |     goto target;
>     |      |     ~~~~
>     |      |     |
>     |      |     (4) ...to here
>     |    8 | 
>     |    9 |  char *foo = malloc(30);
>     |      |        ^~~
>     |      |        |
>     |      |        (1) region created on stack here
>     |      |        (2) capacity: 8 bytes
>     |......
>     |   12 |   free(foo);
>     |      |   ~~~~~~~~~
>     |      |   |
>     |      |   (5) use of uninitialized value ‘foo’ here
>
>
> ...but -fanalyzer isn't something we have enabled by default, it
> is opt-in. I'm also not sure how comprehensive the flow control
> analysis of -fanalyzer is ?  Can we be sure it'll catch these
> mistakes in large complex functions with many code paths ?
>
> Even if the compiler does reliably warn, I think the code pattern
> remains misleading to contributors, as the flow control flaw is
> very non-obvious.

Yup.  Strong dislike.

> Rather than accept the status quo and remove the coding guideline,
> I think we should strengthen the guidelines, such that it is
> explicitly forbidden in any method that uses 'goto'. Personally
> I'd go all the way to -Werror=declaration-after-statement, as

I support this.

> while C99 mixed decl is appealing,

Not to me.

I much prefer declarations and statements to be visually distinct.
Putting declarations first and separating from statements them with a
blank line accomplishes that.  Less necessary in languages where
declarations are syntactically obvious.

>                                    it isn't exactly a game
> changer in improving code maintainability.


[...]