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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

Okay, let's go into the details then. For initialization to be skipped,
two successive rdtsc_ordered() (taking read_pt_and_tsc() as reference)
need to return values 2^^64-1 apart. How do you see that happening, when
(iirc) we've been in agreement more than once that TSC rollover isn't
possible with current or foreseeable hardware?

Things are naturally less unlikely in read_pt_and_tmcct(), for it being
32-bit quantities there. Otoh iirc the APIC timer ticks at rate quite a
bit lower than the TSC. So 2^^32-1 ticks are still a lot of time.

> The compiler really has emitted logic paths where stack rubble is returned.

Of course, since it can't make assumptions on realistic value ranges.

>> 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 fully aware of this. The construct was used wrongly in too many cases.
Still I recall times where I was actively asked to use the construct.

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

These two instances aren't all that old. If you deem them antipatterns (I
don't, albeit I see fair room for abuse), why did you not object (suggesting
whatever better alternative)?

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

Well, you're dong two things at a time, both of which may be clear and
obvious to you. I agree for one half, but I have reservations with the
other. Hence asking that you at least involve all REST maintainers by
Cc-ing them on the patch submission isn't a waste of time, I don't think.
And note, I'm not saying "no" to that second part of the change, but I
do see downsides alongside the upsides you (and I) see.

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

Thank you for wording it that way.

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

But then why not change things so there's no issue anymore? Plus I'm not
sure how / whether "obviously wrong" would manifest. I expect it would
be an entirely unobvious boot hang, or other misbehavior.

Jan