| Message ID | 20190712091239.716978-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] x86: kvm: avoid -Wsometimes-uninitized warning | expand |
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. > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > arch/x86/kvm/hyperv.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c > index a39e38f13029..950436c502ba 100644 > --- a/arch/x86/kvm/hyperv.c > +++ b/arch/x86/kvm/hyperv.c > @@ -1607,7 +1607,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu) > > longmode = is_64_bit_mode(vcpu); > > - if (!longmode) { > + if (!IS_ENABLED(CONFIG_X86_64) || !longmode) { > param = ((u64)kvm_rdx_read(vcpu) << 32) | > (kvm_rax_read(vcpu) & 0xffffffff); > ingpa = ((u64)kvm_rbx_read(vcpu) << 32) | So this is rather a workaround for the compiler giving false positive. I suggest to at least rephrase the log message to inidcate this. Roman.
On Fri, Jul 12, 2019 at 2:03 PM Roman Kagan <rkagan@virtuozzo.com> 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
On 12/07/19 15:02, Arnd Bergmann wrote: > 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. True, on the other hand constant returns are not really rocket science. :) Maybe change is_long_mode to a macro if !CONFIG_X86_64? That would be better if clang likes it. Paolo
On Fri, Jul 12, 2019 at 3:14 PM Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 12/07/19 15:02, Arnd Bergmann wrote: > > 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. > > True, on the other hand constant returns are not really rocket science. :) > > Maybe change is_long_mode to a macro if !CONFIG_X86_64? That would be > better if clang likes it. I had to also get rid of the temporary variable to make it work. Sending v2 now. Arnd
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c index a39e38f13029..950436c502ba 100644 --- a/arch/x86/kvm/hyperv.c +++ b/arch/x86/kvm/hyperv.c @@ -1607,7 +1607,7 @@ int kvm_hv_hypercall(struct kvm_vcpu *vcpu) longmode = is_64_bit_mode(vcpu); - if (!longmode) { + if (!IS_ENABLED(CONFIG_X86_64) || !longmode) { param = ((u64)kvm_rdx_read(vcpu) << 32) | (kvm_rax_read(vcpu) & 0xffffffff); ingpa = ((u64)kvm_rbx_read(vcpu) << 32) |
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. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- arch/x86/kvm/hyperv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.20.0