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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.01.2024 21:43, 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.

I don't think so - aiui this is another of the many gcc extensions to the
language (no code is generated for this type of initialization, iirc).

Jan