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

[Daniel P . Berrangé]

On Fri, Mar 24, 2023 at 06:39:34PM +0100, Juan Quintela wrote:
> Daniel P. Berrangé <berrange@redhat.com> wrote:
> > 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.
> 
> Wait a minute.
> If you use goto, you are already in special rules.
> 
> And don't get confused, I fully agree when using goto for two reasons:
> - performance
>   if you show that the code is x% faster when using goto, it is
>   justified.  It is even better if you send a bug report to gcc/clang,
>   but I will not opose that use.
> - code clearity
>   Some code (basically error paths) are clearer with goto that without
>   them.
> 
> But that don't mind that mixed declarations are bad.  It means that goto
> is complicated.

Yes, goto is complicated and we shouldn't make that worse by using a
code pattern that encourages mistakes IMHO.

> >> - 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.
> >
> >
> > 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. 
> 
> I think this is a bad idea.
> If we want to "catch" unitialized variables, using something like:
> 
> -ftrivial-auto-var-init=pattern sounds much saner.

It depends on what you are aiming to achieve.

In almost all cases where we forgot to initialize something, all-zeros
is the value that we would have wanted to be present. IOW, init=zero
will (almost) always make the code do what we wanted it to do, and thus
is the safe option to make QEMU robust.

Using a non-zero value will be potentially dangerous in a number of
possible ways. It will lead to loops iterating when they should not,
potentially even infinite loops. It will lead to reads/writes off
the end of arrays. It will lead to attempts to free() invalid pointers.
Essentially all the ways in which an uninitialized value can make the
code go wrong wil still potentially happen if we initialized to a
non-zero value. The only benefit is that it will go horribly wrong
in the same way each time.

IOW...

* If you want the application to be robust and generally "do the
  right thing", even in the face of missing initializers, then
  using -ftrivial-auto-var-init=zero is the right answer.

* If you want the application to go horribly wrong, but in a
  repeatable manner, then -ftrivial-auto-var-init=pattern is the
  right answer

Personally I prefer QEMU to be robust and thus believe we should
initialize to zero in real world deployments.

Potentially there's a case for CI jobs to use a non-zero pattern
though to try to expose edge cases.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|