[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] xen: Use -Wuninitialized and -Winit-self
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.
Kind regards,
Roberto
|
