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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

I'm honestly surprised that it hasn't come up yet in the MISRA work.

>  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).

There are exactly two uses of this antipattern in the entirety of Xen. 
They are both in x86 init code.

Do you honestly think trying to block a patch this clear and obvious is
going to be a good use of anyone's time.

>
>>  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.

The fact this is a crap API design doesn't make it ok to use undefined
behaviour.

Getting ~0 back is strictly less bad than getting stack rubble because
at least it's obviously wrong.

~Andrew