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.