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

[Jan Beulich <jbeulich@xxxxxxxx>]

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. Therefore I also disagree that there want to be Fixes: tags
here. There's one case where initialization could be bypassed, but that's a
purely theoretical case afaict.

Furthermore this initialize-to-self is a well known pattern to suppress the
-Wuninitialized induced warnings, originally used by Linux'es
uninitialized_var(). If we really want to use -Winit-self (and hence disallow
use of this pattern even in cases like the ones here, where they're used to
suppress false positive warnings), this should imo be done separately from
adding -Wuninitialized, and only after proper weighing of the pros and cons
(a wider Cc list would be required anyway for the xen/Makefile change).

>  In
> both cases, initialise to ~0 like the associated *_min variable which also
> gates updating best.

Considering the affected functions are both __init, this change isn't a big
problem. But if you were truly concerned of the one theoretical case, you
can't get away with this either: If the variables really remained unwritten,
by returning ~0 you'd end up confusing the caller.

Jan