| Message ID | 1399824664-3499-1-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
On Sun, 11 May 2014 18:11:04 +0200 Alexander Graf <agraf@suse.de> wrote: > Commit 40f1ee27aa1 introduced handy helpers for enable_cap calls on > vcpu and vm level. Unfortunately some older gcc versions (4.7.1, 4.6) > seem to choke on signedness detection in inline created variables: > > target-ppc/kvm.c: In function 'kvmppc_booke_watchdog_enable': > target-ppc/kvm.c:1302:21: error: comparison of unsigned expression < 0 is always false [-Werror=type-limits] > target-ppc/kvm.c: In function 'kvmppc_set_papr': > target-ppc/kvm.c:1504:21: error: comparison of unsigned expression < 0 is always false [-Werror=type-limits] > > We can easily work around this by not doing a compare with 0, but instead > shift all values up by one. The resulting binary should look the same, as > all those ARRAY_SIZE values are compile time statically optimized. > > This patch fixes compile errors on some of my systems. > > Signed-off-by: Alexander Graf <agraf@suse.de> > --- > include/sysemu/kvm.h | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h > index 192fe89..989c261 100644 > --- a/include/sysemu/kvm.h > +++ b/include/sysemu/kvm.h > @@ -302,9 +302,9 @@ int kvm_check_extension(KVMState *s, unsigned int extension); > }; \ > uint64_t args_tmp[] = { __VA_ARGS__ }; \ > int i; \ > - for (i = 0; i < ARRAY_SIZE(args_tmp) && \ > - i < ARRAY_SIZE(cap.args); i++) { \ > - cap.args[i] = args_tmp[i]; \ > + for (i = 1; i < (MIN(ARRAY_SIZE(args_tmp), \ > + ARRAY_SIZE(cap.args)) + 1); i++) { \ > + cap.args[i - 1] = args_tmp[i - 1]; \ > } \ > kvm_vm_ioctl(s, KVM_ENABLE_CAP, &cap); \ > }) > @@ -317,9 +317,9 @@ int kvm_check_extension(KVMState *s, unsigned int extension); > }; \ > uint64_t args_tmp[] = { __VA_ARGS__ }; \ > int i; \ > - for (i = 0; i < ARRAY_SIZE(args_tmp) && \ > - i < ARRAY_SIZE(cap.args); i++) { \ > - cap.args[i] = args_tmp[i]; \ > + for (i = 1; i < (MIN(ARRAY_SIZE(args_tmp), \ > + ARRAY_SIZE(cap.args)) + 1); i++) { \ > + cap.args[i - 1] = args_tmp[i - 1]; \ > } \ > kvm_vcpu_ioctl(cpu, KVM_ENABLE_CAP, &cap); \ > }) That looks ... somewhat ugly. I think you either should add a proper comment here (or somebody will fix it back in the future when reading the code), ... or would it also work to type-cast the result of ARRAY_SIZE to "int"? Something like this: for (i = 0; i < (int)ARRAY_SIZE(args_tmp) && \ i < (int)ARRAY_SIZE(cap.args); i++) Thomas
On Mon, 12 May 2014 09:33:39 +0200 Thomas Huth <thuth@linux.vnet.ibm.com> wrote: > On Sun, 11 May 2014 18:11:04 +0200 > Alexander Graf <agraf@suse.de> wrote: > > > Commit 40f1ee27aa1 introduced handy helpers for enable_cap calls on > > vcpu and vm level. Unfortunately some older gcc versions (4.7.1, 4.6) > > seem to choke on signedness detection in inline created variables: > > > > target-ppc/kvm.c: In function 'kvmppc_booke_watchdog_enable': > > target-ppc/kvm.c:1302:21: error: comparison of unsigned expression < 0 is always false [-Werror=type-limits] > > target-ppc/kvm.c: In function 'kvmppc_set_papr': > > target-ppc/kvm.c:1504:21: error: comparison of unsigned expression < 0 is always false [-Werror=type-limits] > > > > We can easily work around this by not doing a compare with 0, but instead > > shift all values up by one. The resulting binary should look the same, as > > all those ARRAY_SIZE values are compile time statically optimized. > > > > This patch fixes compile errors on some of my systems. > > > > Signed-off-by: Alexander Graf <agraf@suse.de> > > --- > > include/sysemu/kvm.h | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h > > index 192fe89..989c261 100644 > > --- a/include/sysemu/kvm.h > > +++ b/include/sysemu/kvm.h > > @@ -302,9 +302,9 @@ int kvm_check_extension(KVMState *s, unsigned int extension); > > }; \ > > uint64_t args_tmp[] = { __VA_ARGS__ }; \ > > int i; \ > > - for (i = 0; i < ARRAY_SIZE(args_tmp) && \ > > - i < ARRAY_SIZE(cap.args); i++) { \ > > - cap.args[i] = args_tmp[i]; \ > > + for (i = 1; i < (MIN(ARRAY_SIZE(args_tmp), \ > > + ARRAY_SIZE(cap.args)) + 1); i++) { \ > > + cap.args[i - 1] = args_tmp[i - 1]; \ > > } \ > > kvm_vm_ioctl(s, KVM_ENABLE_CAP, &cap); \ > > }) > > @@ -317,9 +317,9 @@ int kvm_check_extension(KVMState *s, unsigned int extension); > > }; \ > > uint64_t args_tmp[] = { __VA_ARGS__ }; \ > > int i; \ > > - for (i = 0; i < ARRAY_SIZE(args_tmp) && \ > > - i < ARRAY_SIZE(cap.args); i++) { \ > > - cap.args[i] = args_tmp[i]; \ > > + for (i = 1; i < (MIN(ARRAY_SIZE(args_tmp), \ > > + ARRAY_SIZE(cap.args)) + 1); i++) { \ > > + cap.args[i - 1] = args_tmp[i - 1]; \ > > } \ > > kvm_vcpu_ioctl(cpu, KVM_ENABLE_CAP, &cap); \ > > }) > > That looks ... somewhat ugly. I think you either should add a proper > comment here (or somebody will fix it back in the future when reading > the code), ... or would it also work to type-cast the result of > ARRAY_SIZE to "int"? Something like this: > > for (i = 0; i < (int)ARRAY_SIZE(args_tmp) && \ > i < (int)ARRAY_SIZE(cap.args); i++) If that one works, I'd prefer that. The other version hurts my eyes :( If it doesn't, I agree it should be properly commented.
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h index 192fe89..989c261 100644 --- a/include/sysemu/kvm.h +++ b/include/sysemu/kvm.h @@ -302,9 +302,9 @@ int kvm_check_extension(KVMState *s, unsigned int extension); }; \ uint64_t args_tmp[] = { __VA_ARGS__ }; \ int i; \ - for (i = 0; i < ARRAY_SIZE(args_tmp) && \ - i < ARRAY_SIZE(cap.args); i++) { \ - cap.args[i] = args_tmp[i]; \ + for (i = 1; i < (MIN(ARRAY_SIZE(args_tmp), \ + ARRAY_SIZE(cap.args)) + 1); i++) { \ + cap.args[i - 1] = args_tmp[i - 1]; \ } \ kvm_vm_ioctl(s, KVM_ENABLE_CAP, &cap); \ }) @@ -317,9 +317,9 @@ int kvm_check_extension(KVMState *s, unsigned int extension); }; \ uint64_t args_tmp[] = { __VA_ARGS__ }; \ int i; \ - for (i = 0; i < ARRAY_SIZE(args_tmp) && \ - i < ARRAY_SIZE(cap.args); i++) { \ - cap.args[i] = args_tmp[i]; \ + for (i = 1; i < (MIN(ARRAY_SIZE(args_tmp), \ + ARRAY_SIZE(cap.args)) + 1); i++) { \ + cap.args[i - 1] = args_tmp[i - 1]; \ } \ kvm_vcpu_ioctl(cpu, KVM_ENABLE_CAP, &cap); \ })
Commit 40f1ee27aa1 introduced handy helpers for enable_cap calls on vcpu and vm level. Unfortunately some older gcc versions (4.7.1, 4.6) seem to choke on signedness detection in inline created variables: target-ppc/kvm.c: In function 'kvmppc_booke_watchdog_enable': target-ppc/kvm.c:1302:21: error: comparison of unsigned expression < 0 is always false [-Werror=type-limits] target-ppc/kvm.c: In function 'kvmppc_set_papr': target-ppc/kvm.c:1504:21: error: comparison of unsigned expression < 0 is always false [-Werror=type-limits] We can easily work around this by not doing a compare with 0, but instead shift all values up by one. The resulting binary should look the same, as all those ARRAY_SIZE values are compile time statically optimized. This patch fixes compile errors on some of my systems. Signed-off-by: Alexander Graf <agraf@suse.de> --- include/sysemu/kvm.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)