Daniel P. Berrangé <berra...@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 <quint...@redhat.com> Subject: [PATCH] Change the default for Mixed declarations. Date: Tue, 14 Feb 2023 17:07:38 +0100 Message-Id: <20230214160738.88614-1-quint...@redhat.com> https://lore.kernel.org/qemu-devel/20230214160738.88614-1-quint...@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. [...]