Re: [PATCH] xen: Use -Wuninitialized and -Winit-self

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 4 Jan 2024, Roberto Bagnara wrote:
> On 2024-01-04 15:33, Andrew Cooper wrote:
> > On 04/01/2024 1:41 pm, Jan Beulich wrote:
> > > On 28.12.2023 20:39, Andrew Cooper wrote:
> > > > The use of uninitialised data is undefined behaviour.  At -O2 with
> > > > trivial
> > > > examples, both Clang and GCC delete the variable, and in the case of a
> > > > function return, the caller gets whatever was stale in %rax prior to the
> > > > call.
> > > > 
> > > > Clang includes -Wuninitialized within -Wall, but GCC only includes it in
> > > > -Wextra, which is not used by Xen at this time.
> > > > 
> > > > Furthermore, the specific pattern of assigning a variable to itself in
> > > > its
> > > > declaration is only diagnosed by GCC with -Winit-self.  Clang does
> > > > diagnoise
> > > > simple forms of this pattern with a plain -Wuninitialized, but it fails
> > > > to
> > > > diagnose the instances in Xen that GCC manages to find.
> > > > 
> > > > GCC, with -Wuninitialized and -Winit-self notices:
> > > > 
> > > >    arch/x86/time.c: In function ‘read_pt_and_tsc’:
> > > >    arch/x86/time.c:297:14: error: ‘best’ is used uninitialized in this
> > > > function [-Werror=uninitialized]
> > > >      297 |     uint32_t best = best;
> > > >          |              ^~~~
> > > >    arch/x86/time.c: In function ‘read_pt_and_tmcct’:
> > > >    arch/x86/time.c:1022:14: error: ‘best’ is used uninitialized in this
> > > > function [-Werror=uninitialized]
> > > >     1022 |     uint64_t best = best;
> > > >          |              ^~~~
> > > > 
> > > > and both have logic paths where best can be returned while uninitalised.
> > > I disagree. In both cases the variables are reliably set during the first
> > > loop iteration.
> > 
> > I suggest you pay attention to the precision of the integers.
> > 
> > It is hard (likely prohibitively hard) to avoid entering the if(), but
> > it is not impossible.
> > 
> > The compiler really has emitted logic paths where stack rubble is returned.
> > 
> > > Furthermore this initialize-to-self is a well known pattern to suppress
> > > the
> > > -Wuninitialized induced warnings, originally used by Linux'es
> > > uninitialized_var().
> > 
> > I'm glad you cited this, because it proves my point.
> > 
> > Notice how it was purged from Linux slowly over the course of 8 years
> > because it had been shown to create real bugs, by hiding real uses of
> > uninitialised variables.
> 
> There is a worse problem for initialize-to-self: it is undefined behavior
> per se.  If this is done to suppress a warning, then what happens is
> paradoxical: in order to suppress a warning about a potential undefined
> behavior (the variable might indeed be always written before being read)
> one introduces a definite undefined behavior.

Thanks for the insight, Roberto. I think best = best is the worst option
because it tries to suppress an uninitialized warning by introducing an
undefined behavior.

I think anything else is better, including:
- best = ~0;
- best = 0;
- some sort of uninitialized_var implementation without init-to-self

I am in favor of adding -Winit-self to the build. That's a good idea. I
don't have an opinion on whether it should be done as part of this patch
or separately. I also don't have an opinion on whether the Fixes tags
are appropriate. I would be happy with or without them.

So, I would ack this patch.

I see that updating the function to return a proper error would be good
but I wouldn't scope-creep an otherwise simple improvement, so I
wouldn't ask the contributor to do it necessarily as part of this patch.