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 <[email protected]>
---
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) |
--
2.20.0
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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]> 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