include/sysemu/kvm.h | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
Building QEMU on fedora26 with the latest gcc package fails:
CC ppc64-softmmu/target/ppc/kvm.o
In file included from include/sysemu/hw_accel.h:16:0,
from target/ppc/kvm.c:31:
target/ppc/kvm.c: In function ‘kvmppc_booke_watchdog_enable’:
include/sysemu/kvm.h:449:35: error: ‘args_tmp[i]’ may be used uninitialized
in this function [-Werror=maybe-uninitialized]
cap.args[i] = args_tmp[i]; \
^
target/ppc/kvm.c: In function ‘kvmppc_set_papr’:
include/sysemu/kvm.h:449:35: error: ‘args_tmp[i]’ may be used uninitialized
in this function [-Werror=maybe-uninitialized]
cc1: all warnings being treated as errors
$ rpm -q gcc
gcc-7.1.1-3.fc26.ppc64le
Testing the size of args_tmp seems to be enough to prevent the warning
to pop up.
Signed-off-by: Greg Kurz <groug@kaod.org>
---
include/sysemu/kvm.h | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 91fc07ee9afe..41fc1005a9ea 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -429,9 +429,11 @@ int kvm_vm_check_extension(KVMState *s, unsigned int extension);
}; \
uint64_t args_tmp[] = { __VA_ARGS__ }; \
int i; \
- for (i = 0; i < (int)ARRAY_SIZE(args_tmp) && \
+ if (sizeof(args_tmp)) { \
+ for (i = 0; i < (int)ARRAY_SIZE(args_tmp) && \
i < ARRAY_SIZE(cap.args); i++) { \
- cap.args[i] = args_tmp[i]; \
+ cap.args[i] = args_tmp[i]; \
+ } \
} \
kvm_vm_ioctl(s, KVM_ENABLE_CAP, &cap); \
})
@@ -444,9 +446,11 @@ int kvm_vm_check_extension(KVMState *s, unsigned int extension);
}; \
uint64_t args_tmp[] = { __VA_ARGS__ }; \
int i; \
- for (i = 0; i < (int)ARRAY_SIZE(args_tmp) && \
+ if (sizeof(args_tmp)) { \
+ for (i = 0; i < (int)ARRAY_SIZE(args_tmp) && \
i < ARRAY_SIZE(cap.args); i++) { \
- cap.args[i] = args_tmp[i]; \
+ cap.args[i] = args_tmp[i]; \
+ } \
} \
kvm_vcpu_ioctl(cpu, KVM_ENABLE_CAP, &cap); \
})
Hi Greg,
On 08/02/2017 11:47 AM, Greg Kurz wrote:
> Building QEMU on fedora26 with the latest gcc package fails:
>
> CC ppc64-softmmu/target/ppc/kvm.o
> In file included from include/sysemu/hw_accel.h:16:0,
> from target/ppc/kvm.c:31:
> target/ppc/kvm.c: In function ‘kvmppc_booke_watchdog_enable’:
> include/sysemu/kvm.h:449:35: error: ‘args_tmp[i]’ may be used uninitialized
> in this function [-Werror=maybe-uninitialized]
> cap.args[i] = args_tmp[i]; \
> ^
> target/ppc/kvm.c: In function ‘kvmppc_set_papr’:
> include/sysemu/kvm.h:449:35: error: ‘args_tmp[i]’ may be used uninitialized
> in this function [-Werror=maybe-uninitialized]
> cc1: all warnings being treated as errors
>
> $ rpm -q gcc
> gcc-7.1.1-3.fc26.ppc64le
>
> Testing the size of args_tmp seems to be enough to prevent the warning
> to pop up.
This sizeof() use looks unnatural to me. I wonder why not use size_t,
since this is about sizeof()/ARRAY_SIZE(). The problem seems to come
from the commit this cast was introduced (61c7bbd236):
target-ppc/kvm.c:1302:21: error: comparison of unsigned expression < 0
is always false [-Werror=type-limits]
So I'd rather suggest this code, which looks more natural to read to me:
if (ARRAY_SIZE(args_tmp)) {
for (i = 0; i < ARRAY_SIZE(args_tmp) && ...
I Cc'ed Eric :)
Regards,
Phil.
>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> include/sysemu/kvm.h | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 91fc07ee9afe..41fc1005a9ea 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -429,9 +429,11 @@ int kvm_vm_check_extension(KVMState *s, unsigned int extension);
> }; \
> uint64_t args_tmp[] = { __VA_ARGS__ }; \
> int i; \
> - for (i = 0; i < (int)ARRAY_SIZE(args_tmp) && \
> + if (sizeof(args_tmp)) { \
> + for (i = 0; i < (int)ARRAY_SIZE(args_tmp) && \
> i < ARRAY_SIZE(cap.args); i++) { \
> - cap.args[i] = args_tmp[i]; \
> + cap.args[i] = args_tmp[i]; \
> + } \
> } \
> kvm_vm_ioctl(s, KVM_ENABLE_CAP, &cap); \
> })
> @@ -444,9 +446,11 @@ int kvm_vm_check_extension(KVMState *s, unsigned int extension);
> }; \
> uint64_t args_tmp[] = { __VA_ARGS__ }; \
> int i; \
> - for (i = 0; i < (int)ARRAY_SIZE(args_tmp) && \
> + if (sizeof(args_tmp)) { \
> + for (i = 0; i < (int)ARRAY_SIZE(args_tmp) && \
> i < ARRAY_SIZE(cap.args); i++) { \
> - cap.args[i] = args_tmp[i]; \
> + cap.args[i] = args_tmp[i]; \
> + } \
> } \
> kvm_vcpu_ioctl(cpu, KVM_ENABLE_CAP, &cap); \
> })
>
>
On 08/03/2017 08:34 AM, Philippe Mathieu-Daudé wrote:
> Hi Greg,
>
> On 08/02/2017 11:47 AM, Greg Kurz wrote:
>> Building QEMU on fedora26 with the latest gcc package fails:
>>
>> CC ppc64-softmmu/target/ppc/kvm.o
>> In file included from include/sysemu/hw_accel.h:16:0,
>> from target/ppc/kvm.c:31:
>> target/ppc/kvm.c: In function ‘kvmppc_booke_watchdog_enable’:
>> include/sysemu/kvm.h:449:35: error: ‘args_tmp[i]’ may be used
>> uninitialized
>
> This sizeof() use looks unnatural to me. I wonder why not use size_t,
> since this is about sizeof()/ARRAY_SIZE(). The problem seems to come
> from the commit this cast was introduced (61c7bbd236):
>
> target-ppc/kvm.c:1302:21: error: comparison of unsigned expression < 0
> is always false [-Werror=type-limits]
>
> So I'd rather suggest this code, which looks more natural to read to me:
>
> if (ARRAY_SIZE(args_tmp)) {
> for (i = 0; i < ARRAY_SIZE(args_tmp) && ...
For that matter, the existing code is doing:
int i;
i < (int)ARRAY_SIZE(args_tmp)
but wouldn't that be better as:
size_t i;
i < ARRAY_SIZE(args_temp)
I guess we have both the old compilers (per commit 61c7bbd2) and the new
to worry about; although I was unable to reproduce it on Fedora 26 on
x86_64 (is this an architecture-dependent compiler bug?)
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
On 08/03/2017 10:55 AM, Eric Blake wrote:
> On 08/03/2017 08:34 AM, Philippe Mathieu-Daudé wrote:
>> Hi Greg,
>>
>> On 08/02/2017 11:47 AM, Greg Kurz wrote:
>>> Building QEMU on fedora26 with the latest gcc package fails:
>>>
>>> CC ppc64-softmmu/target/ppc/kvm.o
>>> In file included from include/sysemu/hw_accel.h:16:0,
>>> from target/ppc/kvm.c:31:
>>> target/ppc/kvm.c: In function ‘kvmppc_booke_watchdog_enable’:
>>> include/sysemu/kvm.h:449:35: error: ‘args_tmp[i]’ may be used
>>> uninitialized
>
>>
>> This sizeof() use looks unnatural to me. I wonder why not use size_t,
>> since this is about sizeof()/ARRAY_SIZE(). The problem seems to come
>> from the commit this cast was introduced (61c7bbd236):
>>
>> target-ppc/kvm.c:1302:21: error: comparison of unsigned expression < 0
>> is always false [-Werror=type-limits]
>>
>> So I'd rather suggest this code, which looks more natural to read to me:
>>
>> if (ARRAY_SIZE(args_tmp)) {
>> for (i = 0; i < ARRAY_SIZE(args_tmp) && ...
Oops I forgot to suggest size_t i.
>
> For that matter, the existing code is doing:
>
> int i;
> i < (int)ARRAY_SIZE(args_tmp)
>
> but wouldn't that be better as:
>
> size_t i;
> i < ARRAY_SIZE(args_temp)
>
> I guess we have both the old compilers (per commit 61c7bbd2) and the new
> to worry about; although I was unable to reproduce it on Fedora 26 on
> x86_64 (is this an architecture-dependent compiler bug?)
>
Ok so let's stop losing time about compiler incoherent warnings, using
-Wno-type-limits for GCC < 5...
So we can keep a sane/understandable codebase, using size_t and no (int)
cast.
Phil.
On Thu, 3 Aug 2017 11:07:13 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 08/03/2017 10:55 AM, Eric Blake wrote:
> > On 08/03/2017 08:34 AM, Philippe Mathieu-Daudé wrote:
> >> Hi Greg,
> >>
> >> On 08/02/2017 11:47 AM, Greg Kurz wrote:
> >>> Building QEMU on fedora26 with the latest gcc package fails:
> >>>
> >>> CC ppc64-softmmu/target/ppc/kvm.o
> >>> In file included from include/sysemu/hw_accel.h:16:0,
> >>> from target/ppc/kvm.c:31:
> >>> target/ppc/kvm.c: In function ‘kvmppc_booke_watchdog_enable’:
> >>> include/sysemu/kvm.h:449:35: error: ‘args_tmp[i]’ may be used
> >>> uninitialized
> >
> >>
> >> This sizeof() use looks unnatural to me. I wonder why not use size_t,
> >> since this is about sizeof()/ARRAY_SIZE(). The problem seems to come
> >> from the commit this cast was introduced (61c7bbd236):
> >>
> >> target-ppc/kvm.c:1302:21: error: comparison of unsigned expression < 0
> >> is always false [-Werror=type-limits]
> >>
> >> So I'd rather suggest this code, which looks more natural to read to me:
> >>
> >> if (ARRAY_SIZE(args_tmp)) {
> >> for (i = 0; i < ARRAY_SIZE(args_tmp) && ...
>
> Oops I forgot to suggest size_t i.
>
> >
> > For that matter, the existing code is doing:
> >
> > int i;
> > i < (int)ARRAY_SIZE(args_tmp)
> >
> > but wouldn't that be better as:
> >
> > size_t i;
> > i < ARRAY_SIZE(args_temp)
> >
> > I guess we have both the old compilers (per commit 61c7bbd2) and the new
> > to worry about; although I was unable to reproduce it on Fedora 26 on
> > x86_64 (is this an architecture-dependent compiler bug?)
> >
>
The following code snippet spits a warning with gcc-7.1.1-3.fc26 and -Wall on
ppc64le AND x86_64:
int foo(void)
{
char empty_array[] = { };
int i, ret = 0;
for (i = 0; i < (int) (sizeof(empty_array) / sizeof(empty_array[0])); i++) {
ret = empty_array[i];
}
return ret;
}
If I drop the (int), the warning goes away... and so does the build break
of qemu-system-ppc64 on my ppc64le host.
I don't have 4.7.1 or 4.6 compilers around but I could check 4.8.5
is okay with that change FWIW.
> Ok so let's stop losing time about compiler incoherent warnings, using
> -Wno-type-limits for GCC < 5...
> So we can keep a sane/understandable codebase, using size_t and no (int)
> cast.
>
If I also convert 'int i' to 'size_t i' then I get the same error as
in commit 61c7bbd2:
error: comparison of unsigned expression < 0 is always
false [-Werror=type-limits]
> Phil.
On 08/03/2017 01:38 PM, Greg Kurz wrote:
> The following code snippet spits a warning with gcc-7.1.1-3.fc26 and -Wall on
> ppc64le AND x86_64:
>
> int foo(void)
> {
> char empty_array[] = { };
> int i, ret = 0;
>
> for (i = 0; i < (int) (sizeof(empty_array) / sizeof(empty_array[0])); i++) {
> ret = empty_array[i];
> }
>
> return ret;
> }
Confirmed. No idea why the cast makes gcc think i is uninitialized.
>
> If I drop the (int), the warning goes away... and so does the build break
> of qemu-system-ppc64 on my ppc64le host.
>
> I don't have 4.7.1 or 4.6 compilers around but I could check 4.8.5
> is okay with that change FWIW.
I tested with gcc 4.4.7 on RHEL 6, but -Wtype-limits there didn't warn
whether or not the (int) was present; it's possible that we still need
to test with 4.6 or 4.7.1 to see whether it makes a difference.
>
>> Ok so let's stop losing time about compiler incoherent warnings, using
>> -Wno-type-limits for GCC < 5...
>> So we can keep a sane/understandable codebase, using size_t and no (int)
>> cast.
That's also a possibility, if we still hit old compilers that require
the (int).
>>
>
> If I also convert 'int i' to 'size_t i' then I get the same error as
> in commit 61c7bbd2:
>
> error: comparison of unsigned expression < 0 is always
> false [-Werror=type-limits]
Confirmed with newer gcc, whether or not the (int) cast is present.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
On Thu, 3 Aug 2017 13:56:45 -0500
Eric Blake <eblake@redhat.com> wrote:
> On 08/03/2017 01:38 PM, Greg Kurz wrote:
> > The following code snippet spits a warning with gcc-7.1.1-3.fc26 and -Wall on
> > ppc64le AND x86_64:
> >
> > int foo(void)
> > {
> > char empty_array[] = { };
> > int i, ret = 0;
> >
> > for (i = 0; i < (int) (sizeof(empty_array) / sizeof(empty_array[0])); i++) {
> > ret = empty_array[i];
> > }
> >
> > return ret;
> > }
>
> Confirmed. No idea why the cast makes gcc think i is uninitialized.
>
FYI, I've created a BZ to track this issue in gcc:
https://bugzilla.redhat.com/show_bug.cgi?id=1478864
> >
> > If I drop the (int), the warning goes away... and so does the build break
> > of qemu-system-ppc64 on my ppc64le host.
> >
> > I don't have 4.7.1 or 4.6 compilers around but I could check 4.8.5
> > is okay with that change FWIW.
>
> I tested with gcc 4.4.7 on RHEL 6, but -Wtype-limits there didn't warn
> whether or not the (int) was present; it's possible that we still need
> to test with 4.6 or 4.7.1 to see whether it makes a difference.
>
> >
> >> Ok so let's stop losing time about compiler incoherent warnings, using
> >> -Wno-type-limits for GCC < 5...
> >> So we can keep a sane/understandable codebase, using size_t and no (int)
> >> cast.
>
> That's also a possibility, if we still hit old compilers that require
> the (int).
>
> >>
> >
> > If I also convert 'int i' to 'size_t i' then I get the same error as
> > in commit 61c7bbd2:
> >
> > error: comparison of unsigned expression < 0 is always
> > false [-Werror=type-limits]
>
> Confirmed with newer gcc, whether or not the (int) cast is present.
>
On Thu, 3 Aug 2017 10:34:45 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Greg,
>
> On 08/02/2017 11:47 AM, Greg Kurz wrote:
> > Building QEMU on fedora26 with the latest gcc package fails:
> >
> > CC ppc64-softmmu/target/ppc/kvm.o
> > In file included from include/sysemu/hw_accel.h:16:0,
> > from target/ppc/kvm.c:31:
> > target/ppc/kvm.c: In function ‘kvmppc_booke_watchdog_enable’:
> > include/sysemu/kvm.h:449:35: error: ‘args_tmp[i]’ may be used uninitialized
> > in this function [-Werror=maybe-uninitialized]
> > cap.args[i] = args_tmp[i]; \
> > ^
> > target/ppc/kvm.c: In function ‘kvmppc_set_papr’:
> > include/sysemu/kvm.h:449:35: error: ‘args_tmp[i]’ may be used uninitialized
> > in this function [-Werror=maybe-uninitialized]
> > cc1: all warnings being treated as errors
> >
> > $ rpm -q gcc
> > gcc-7.1.1-3.fc26.ppc64le
> >
> > Testing the size of args_tmp seems to be enough to prevent the warning
> > to pop up.
>
> This sizeof() use looks unnatural to me. I wonder why not use size_t,
> since this is about sizeof()/ARRAY_SIZE(). The problem seems to come
> from the commit this cast was introduced (61c7bbd236):
>
> target-ppc/kvm.c:1302:21: error: comparison of unsigned expression < 0
> is always false [-Werror=type-limits]
>
Switching from int to size_t causes this error ^^ to show up again.
> So I'd rather suggest this code, which looks more natural to read to me:
>
> if (ARRAY_SIZE(args_tmp)) {
Yeah, it's maybe more in phase with the context but anyway, the truly
unnatural thing is to be forced to do:
if (foo) {
for (i = 0; i < foo; ...
> for (i = 0; i < ARRAY_SIZE(args_tmp) && ...
>
> I Cc'ed Eric :)
>
> Regards,
>
> Phil.
>
> >
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > include/sysemu/kvm.h | 12 ++++++++----
> > 1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > index 91fc07ee9afe..41fc1005a9ea 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -429,9 +429,11 @@ int kvm_vm_check_extension(KVMState *s, unsigned int extension);
> > }; \
> > uint64_t args_tmp[] = { __VA_ARGS__ }; \
> > int i; \
> > - for (i = 0; i < (int)ARRAY_SIZE(args_tmp) && \
> > + if (sizeof(args_tmp)) { \
> > + for (i = 0; i < (int)ARRAY_SIZE(args_tmp) && \
> > i < ARRAY_SIZE(cap.args); i++) { \
> > - cap.args[i] = args_tmp[i]; \
> > + cap.args[i] = args_tmp[i]; \
> > + } \
> > } \
> > kvm_vm_ioctl(s, KVM_ENABLE_CAP, &cap); \
> > })
> > @@ -444,9 +446,11 @@ int kvm_vm_check_extension(KVMState *s, unsigned int extension);
> > }; \
> > uint64_t args_tmp[] = { __VA_ARGS__ }; \
> > int i; \
> > - for (i = 0; i < (int)ARRAY_SIZE(args_tmp) && \
> > + if (sizeof(args_tmp)) { \
> > + for (i = 0; i < (int)ARRAY_SIZE(args_tmp) && \
> > i < ARRAY_SIZE(cap.args); i++) { \
> > - cap.args[i] = args_tmp[i]; \
> > + cap.args[i] = args_tmp[i]; \
> > + } \
> > } \
> > kvm_vcpu_ioctl(cpu, KVM_ENABLE_CAP, &cap); \
> > })
> >
> >
Hi Greg, On 08/02/2017 11:47 AM, Greg Kurz wrote: > Building QEMU on fedora26 with the latest gcc package fails: > > CC ppc64-softmmu/target/ppc/kvm.o > In file included from include/sysemu/hw_accel.h:16:0, > from target/ppc/kvm.c:31: > target/ppc/kvm.c: In function ‘kvmppc_booke_watchdog_enable’: > include/sysemu/kvm.h:449:35: error: ‘args_tmp[i]’ may be used uninitialized > in this function [-Werror=maybe-uninitialized] > cap.args[i] = args_tmp[i]; \ > ^ > target/ppc/kvm.c: In function ‘kvmppc_set_papr’: > include/sysemu/kvm.h:449:35: error: ‘args_tmp[i]’ may be used uninitialized > in this function [-Werror=maybe-uninitialized] > cc1: all warnings being treated as errors I'm trying to reproduce this in our docker images (all x86_64 based) but can't reproduce. ./configure shows: KVM support yes but in "sysemu/kvm.h" CONFIG_KVM_IS_POSSIBLE is not defined I can see CONFIG_KVM defined, but no NEED_CPU_H. > > $ rpm -q gcc > gcc-7.1.1-3.fc26.ppc64le I don't have native ppc64le to build, do you know if it is possible to cross-compile enabling kvm? It seems I have the correct Linux headers, I wonder if this isn't a ./configure test which disable the kvm cross-build. Regards, Phil.
On 08/03/2017 08:46 AM, Philippe Mathieu-Daudé wrote:
> Hi Greg,
>
> On 08/02/2017 11:47 AM, Greg Kurz wrote:
>> Building QEMU on fedora26 with the latest gcc package fails:
>>
>> CC ppc64-softmmu/target/ppc/kvm.o
>> In file included from include/sysemu/hw_accel.h:16:0,
>> from target/ppc/kvm.c:31:
>> target/ppc/kvm.c: In function ‘kvmppc_booke_watchdog_enable’:
>> include/sysemu/kvm.h:449:35: error: ‘args_tmp[i]’ may be used
>> uninitialized
>> in this function [-Werror=maybe-uninitialized]
>> cap.args[i] = args_tmp[i]; \
>> ^
>> target/ppc/kvm.c: In function ‘kvmppc_set_papr’:
>> include/sysemu/kvm.h:449:35: error: ‘args_tmp[i]’ may be used
>> uninitialized
>> in this function [-Werror=maybe-uninitialized]
>> cc1: all warnings being treated as errors
>
> I'm trying to reproduce this in our docker images (all x86_64 based) but
> can't reproduce.
That's because x86_64 hosts only call kvm_vm_enable_cap() with non-empty
varargs. But we have:
accel/kvm/kvm-all.c: ret = kvm_vm_enable_cap(s, KVM_CAP_S390_IRQCHIP, 0);
which is only compiled on s390 hosts (or, at least that's my guess,
based on the cap name) - and THAT code is passing empty varargs, which
explains args_tmp[] being a 0-length array, and getting the compiler to
complain about i < 0 always being false.
So my question on IRC was whether we can stack the decks, and force a
non-empty args_tmp = { 0, __VA_ARGS__} coupled by skipping the first
iteration in the for loop. Or, since cap.args[] is already being
zero-initialized, args_tmp = { __VA_ARGS__, 0 } means the last iteration
of the for loop is a no-op (assigning 0 to something that is already 0)
- although that may be harder to correctly account for both empty and
non-empty __VA_ARGS__.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
On Thu, 3 Aug 2017 09:10:29 -0500
Eric Blake <eblake@redhat.com> wrote:
> On 08/03/2017 08:46 AM, Philippe Mathieu-Daudé wrote:
> > Hi Greg,
> >
> > On 08/02/2017 11:47 AM, Greg Kurz wrote:
> >> Building QEMU on fedora26 with the latest gcc package fails:
> >>
> >> CC ppc64-softmmu/target/ppc/kvm.o
> >> In file included from include/sysemu/hw_accel.h:16:0,
> >> from target/ppc/kvm.c:31:
> >> target/ppc/kvm.c: In function ‘kvmppc_booke_watchdog_enable’:
> >> include/sysemu/kvm.h:449:35: error: ‘args_tmp[i]’ may be used
> >> uninitialized
> >> in this function [-Werror=maybe-uninitialized]
> >> cap.args[i] = args_tmp[i]; \
> >> ^
> >> target/ppc/kvm.c: In function ‘kvmppc_set_papr’:
> >> include/sysemu/kvm.h:449:35: error: ‘args_tmp[i]’ may be used
> >> uninitialized
> >> in this function [-Werror=maybe-uninitialized]
> >> cc1: all warnings being treated as errors
> >
> > I'm trying to reproduce this in our docker images (all x86_64 based) but
> > can't reproduce.
>
> That's because x86_64 hosts only call kvm_vm_enable_cap() with non-empty
> varargs.
There's
target/i386/kvm.c: kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_SYNIC, 0)) {
> But we have:
>
> accel/kvm/kvm-all.c: ret = kvm_vm_enable_cap(s, KVM_CAP_S390_IRQCHIP, 0);
>
> which is only compiled on s390 hosts (or, at least that's my guess,
> based on the cap name)
I don't see how the compiler can optimize this away, as the check for
this cap is an ioctl...
> - and THAT code is passing empty varargs, which
> explains args_tmp[] being a 0-length array, and getting the compiler to
> complain about i < 0 always being false.
[I don't have any s390x system with gcc7 yet, or I'd test this.]
>
> So my question on IRC was whether we can stack the decks, and force a
> non-empty args_tmp = { 0, __VA_ARGS__} coupled by skipping the first
> iteration in the for loop. Or, since cap.args[] is already being
> zero-initialized, args_tmp = { __VA_ARGS__, 0 } means the last iteration
> of the for loop is a no-op (assigning 0 to something that is already 0)
> - although that may be harder to correctly account for both empty and
> non-empty __VA_ARGS__.
This seems a bit ugly. And I still don't understand why this only seems
to hit on ppc...
On 03/08/2017 16:24, Cornelia Huck wrote:
> On Thu, 3 Aug 2017 09:10:29 -0500
> Eric Blake <eblake@redhat.com> wrote:
>
>> On 08/03/2017 08:46 AM, Philippe Mathieu-Daudé wrote:
>>> Hi Greg,
>>>
>>> On 08/02/2017 11:47 AM, Greg Kurz wrote:
>>>> Building QEMU on fedora26 with the latest gcc package fails:
>>>>
>>>> CC ppc64-softmmu/target/ppc/kvm.o
>>>> In file included from include/sysemu/hw_accel.h:16:0,
>>>> from target/ppc/kvm.c:31:
>>>> target/ppc/kvm.c: In function ‘kvmppc_booke_watchdog_enable’:
>>>> include/sysemu/kvm.h:449:35: error: ‘args_tmp[i]’ may be used
>>>> uninitialized
>>>> in this function [-Werror=maybe-uninitialized]
>>>> cap.args[i] = args_tmp[i]; \
>>>> ^
>>>> target/ppc/kvm.c: In function ‘kvmppc_set_papr’:
>>>> include/sysemu/kvm.h:449:35: error: ‘args_tmp[i]’ may be used
>>>> uninitialized
>>>> in this function [-Werror=maybe-uninitialized]
>>>> cc1: all warnings being treated as errors
>>>
>>> I'm trying to reproduce this in our docker images (all x86_64 based) but
>>> can't reproduce.
>>
>> That's because x86_64 hosts only call kvm_vm_enable_cap() with non-empty
>> varargs.
>
> There's
>
> target/i386/kvm.c: kvm_vcpu_enable_cap(cs, KVM_CAP_HYPERV_SYNIC, 0)) {
>
>> But we have:
>>
>> accel/kvm/kvm-all.c: ret = kvm_vm_enable_cap(s, KVM_CAP_S390_IRQCHIP, 0);
>>
>> which is only compiled on s390 hosts (or, at least that's my guess,
>> based on the cap name)
>
> I don't see how the compiler can optimize this away, as the check for
> this cap is an ioctl...
Indeed. This is a compiler bug and it only provides a warning (meaning
--disable-werror silences it). I don't think it's a good idea to uglify
the code for something that the compiler should easily optimize away...
Paolo
> This seems a bit ugly. And I still don't understand why this only seems
> to hit on ppc...
On Thu, 3 Aug 2017 10:46:47 -0300 Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > Hi Greg, > > On 08/02/2017 11:47 AM, Greg Kurz wrote: > > Building QEMU on fedora26 with the latest gcc package fails: > > > > CC ppc64-softmmu/target/ppc/kvm.o > > In file included from include/sysemu/hw_accel.h:16:0, > > from target/ppc/kvm.c:31: > > target/ppc/kvm.c: In function ‘kvmppc_booke_watchdog_enable’: > > include/sysemu/kvm.h:449:35: error: ‘args_tmp[i]’ may be used uninitialized > > in this function [-Werror=maybe-uninitialized] > > cap.args[i] = args_tmp[i]; \ > > ^ > > target/ppc/kvm.c: In function ‘kvmppc_set_papr’: > > include/sysemu/kvm.h:449:35: error: ‘args_tmp[i]’ may be used uninitialized > > in this function [-Werror=maybe-uninitialized] > > cc1: all warnings being treated as errors > > I'm trying to reproduce this in our docker images (all x86_64 based) but > can't reproduce. > > ./configure shows: > > KVM support yes > > but in "sysemu/kvm.h" CONFIG_KVM_IS_POSSIBLE is not defined > > I can see CONFIG_KVM defined, but no NEED_CPU_H. > > > > > $ rpm -q gcc > > gcc-7.1.1-3.fc26.ppc64le > > I don't have native ppc64le to build, do you know if it is possible to > cross-compile enabling kvm? It seems I have the correct Linux headers, I > wonder if this isn't a ./configure test which disable the kvm cross-build. > I don't cross-compile personally but I see no reasons why it wouldn't be possible. I'd say the biggest churn is to setup the cross-compile environment :) > Regards, > > Phil.
© 2016 - 2024 Red Hat, Inc.