Mailing List Archive: [PATCH] xen: Use -Wuninitialized and -Winit-self

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

Dec 28, 2023, 11:39 AM

Post #1 of 11 (97 views)

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. In
both cases, initialise to ~0 like the associated *_min variable which also
gates updating best.

Fixes: 23658e823238 ("x86/time: further improve TSC / CPU freq calibration accuracy")
Fixes: 3f3906b462d5 ("x86/APIC: calibrate against platform timer when possible")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

Full Gitlab:
https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/1121715339
---
xen/Makefile | 3 ++-
xen/arch/x86/time.c | 4 ++--
2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/Makefile b/xen/Makefile
index 21832d640225..81861bc41867 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -393,8 +393,9 @@ CFLAGS-$(CONFIG_CC_SPLIT_SECTIONS) += -ffunction-sections -fdata-sections

CFLAGS += -nostdinc -fno-builtin -fno-common
CFLAGS += -Werror -Wredundant-decls -Wwrite-strings -Wno-pointer-arith
-CFLAGS += -Wdeclaration-after-statement
+CFLAGS += -Wdeclaration-after-statement -Wuninitialized
$(call cc-option-add,CFLAGS,CC,-Wvla)
+$(call cc-option-add,CFLAGS,CC,-Winit-self)
CFLAGS += -pipe -D__XEN__ -include $(srctree)/include/xen/config.h
CFLAGS-$(CONFIG_DEBUG_INFO) += -g

diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 6d33edd0addc..3fd90e9b78bc 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -294,7 +294,7 @@ static uint32_t __init read_pt_and_tsc(uint64_t *tsc,
const struct platform_timesource *pts)
{
uint64_t tsc_prev = *tsc = rdtsc_ordered(), tsc_min = ~0;
- uint32_t best = best;
+ uint32_t best = ~0;
unsigned int i;

for ( i = 0; ; ++i )
@@ -1019,7 +1019,7 @@ static u64 __init init_platform_timer(void)
static uint64_t __init read_pt_and_tmcct(uint32_t *tmcct)
{
uint32_t tmcct_prev = *tmcct = apic_tmcct_read(), tmcct_min = ~0;
- uint64_t best = best;
+ uint64_t best = ~0;
unsigned int i;

for ( i = 0; ; ++i )
--
2.30.2

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

jbeulich at suse

Jan 4, 2024, 5:41 AM

Post #2 of 11 (88 views)

Permalink

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

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

andrew.cooper3 at citrix

Jan 4, 2024, 6:33 AM

Post #3 of 11 (88 views)

Permalink

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

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

jbeulich at suse

Jan 4, 2024, 7:33 AM

Post #4 of 11 (88 views)

Permalink

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

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

roberto.bagnara at bugseng

Jan 4, 2024, 12:43 PM

Post #5 of 11 (87 views)

Permalink

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

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

tamas.k.lengyel at gmail

Jan 4, 2024, 1:27 PM

Post #6 of 11 (87 views)

Permalink

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

+1 for changing these APIs to make it clear when an error happened
instead of returning magic value. Otherwise yea clearly should not use
init-to-self anywhere just to silence other warnings..

Tamas

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

sstabellini at kernel

Jan 4, 2024, 4:02 PM

Post #7 of 11 (87 views)

Permalink

On Thu, 4 Jan 2024, 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.

Thanks for the insight, Roberto. I think best = best is the worst option
because it tries to suppress an uninitialized warning by introducing an
undefined behavior.

I think anything else is better, including:
- best = ~0;
- best = 0;
- some sort of uninitialized_var implementation without init-to-self

I am in favor of adding -Winit-self to the build. That's a good idea. I
don't have an opinion on whether it should be done as part of this patch
or separately. I also don't have an opinion on whether the Fixes tags
are appropriate. I would be happy with or without them.

So, I would ack this patch.

I see that updating the function to return a proper error would be good
but I wouldn't scope-creep an otherwise simple improvement, so I
wouldn't ask the contributor to do it necessarily as part of this patch.

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

jbeulich at suse

Jan 4, 2024, 10:56 PM

Post #8 of 11 (78 views)

Permalink

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

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

jbeulich at suse

Jan 4, 2024, 11:00 PM

Post #9 of 11 (78 views)

Permalink

On 04.01.2024 16:33, Jan Beulich wrote:
> On 04.01.2024 15:33, Andrew Cooper wrote:
>> On 04/01/2024 1:41 pm, Jan Beulich wrote:
>>> 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.

And, btw: The construct was (meant to be) used in particular for suppressing
false-positive -Wuninitialized warnings on, in particular, older gcc. Which
means that by enabling this option we may also subsequently need to deal with
fallout. I'm not convinced we want to uniformly do so by adding "normal"
initializers wherever (not really) needed.

Jan

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

roberto.bagnara at bugseng

Jan 5, 2024, 4:25 AM

Post #10 of 11 (78 views)

Permalink

On 2024-01-05 07:56, Jan Beulich wrote:
> 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).

Well, it is undefined behavior for the C Standard: "undefined behavior"
is a technical term (see, e.g., C18 3.4.3p1) that is not dependent on
any compiler or any implementation technique for the C language.
Any implementation can deal with undefined behavior with absolute
freedom, which includes, among infinite other possibilities, doing what
you believe it does (iyrc ;-)

In addition, the compiler is typically not the only element of the toolchain
that needs to reliably interpret and process C code: so, even if we could reach
absolute certainty about what a specific version of one compiler does in response
to one instance of undefined behavior (and I think we cannot), then we would
need to obtain similar guarantees for other tools.

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

sstabellini at kernel

Apr 25, 2024, 4:01 PM

Post #11 of 11 (6 views)

Permalink

On Thu, 28 Dec 2023, 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. In
> both cases, initialise to ~0 like the associated *_min variable which also
> gates updating best.
>
> Fixes: 23658e823238 ("x86/time: further improve TSC / CPU freq calibration accuracy")
> Fixes: 3f3906b462d5 ("x86/APIC: calibrate against platform timer when possible")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>