Re: [PATCH 1/2] x86: kvm: avoid -Wsometimes-uninitized warning

[Arnd Bergmann]

On Fri, Jul 12, 2019 at 2:03 PM Roman Kagan <rkagan@xxxxxxxxxxxxx> wrote:
>
> On Fri, Jul 12, 2019 at 11:12:29AM +0200, Arnd Bergmann wrote:
> > clang points out that running a 64-bit guest on a 32-bit host
> > would lead to uninitialized variables:
> >
> > arch/x86/kvm/hyperv.c:1610:6: error: variable 'ingpa' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
> >         if (!longmode) {
> >             ^~~~~~~~~
> > arch/x86/kvm/hyperv.c:1632:55: note: uninitialized use occurs here
> >         trace_kvm_hv_hypercall(code, fast, rep_cnt, rep_idx, ingpa, outgpa);
> >                                                              ^~~~~
> > arch/x86/kvm/hyperv.c:1610:2: note: remove the 'if' if its condition is always true
> >         if (!longmode) {
> >         ^~~~~~~~~~~~~~~
> > arch/x86/kvm/hyperv.c:1595:18: note: initialize the variable 'ingpa' to silence this warning
> >         u64 param, ingpa, outgpa, ret = HV_STATUS_SUCCESS;
> >                         ^
> >                          = 0
> > arch/x86/kvm/hyperv.c:1610:6: error: variable 'outgpa' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
> > arch/x86/kvm/hyperv.c:1610:6: error: variable 'param' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
> >
> > Since that combination is not supported anyway, change the condition
> > to tell the compiler how the code is actually executed.
>
> Hmm, the compiler *is* told all it needs:
>
>
> arch/x86/kvm/x86.h:
> ...
> static inline int is_long_mode(struct kvm_vcpu *vcpu)
> {
> #ifdef CONFIG_X86_64
>         return vcpu->arch.efer & EFER_LMA;
> #else
>         return 0;
> #endif
> }
>
> static inline bool is_64_bit_mode(struct kvm_vcpu *vcpu)
> {
>         int cs_db, cs_l;
>
>         if (!is_long_mode(vcpu))
>                 return false;
>         kvm_x86_ops->get_cs_db_l_bits(vcpu, &cs_db, &cs_l);
>         return cs_l;
> }
> ...
>
> so in !CONFIG_X86_64 case is_64_bit_mode() unconditionally returns
> false, and the branch setting the values of the variables is always
> taken.

I think what happens here is that clang does not treat the return
code of track the return code of is_64_bit_mode() as a constant
expression, and therefore assumes that the if() condition
may or may not be true, for the purpose of determining whether
the variable is used without an inialization. This would hold even
if it later eliminates the code leading up to the if() in an optimization
stage. IS_ENABLED(CONFIG_X86_64) however is a constant
expression, so with the patch, it understands this.

In contrast, gcc seems to perform all the inlining first, and
then see if some variable is used uninitialized in the final code.
This gives additional information to the compiler, but makes the
outcome less predictable since it depends on optimization flags
and architecture specific behavior.

Both approaches have their own sets of false positive and false
negative warnings.

       Arnd