[Qemu-devel] [PATCH v2] kvm: workaround build break on gcc-7.1.1 / fedora26

Greg Kurz posted 1 patch 6 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/150210580404.1343.7325713896658799315.stgit@bahia.lan
Test FreeBSD passed
Test checkpatch passed
Test docker passed
Test s390x passed
include/sysemu/kvm.h |   14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
[Qemu-devel] [PATCH v2] kvm: workaround build break on gcc-7.1.1 / fedora26
Posted by Greg Kurz 6 years, 7 months ago
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

The compiler should obviously optimize this code away when no extra
agument is passed to kvm_vm_enable_cap() and kvm_vcpu_enable_cap(),
but it doesn't. This bug should be fixed one day in gcc, but we can
also change our code pattern so that we don't hit the issue anymore.
We workaround this, by using memcpy() instead of open-coding the copy.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
v2: - use memcpy()
---
 include/sysemu/kvm.h |   14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 91fc07ee9afe..3a458f50e9f4 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -428,11 +428,8 @@ int kvm_vm_check_extension(KVMState *s, unsigned int extension);
             .flags = cap_flags,                                      \
         };                                                           \
         uint64_t args_tmp[] = { __VA_ARGS__ };                       \
-        int i;                                                       \
-        for (i = 0; i < (int)ARRAY_SIZE(args_tmp) &&                 \
-                     i < ARRAY_SIZE(cap.args); i++) {                \
-            cap.args[i] = args_tmp[i];                               \
-        }                                                            \
+        size_t n = MIN(ARRAY_SIZE(args_tmp), ARRAY_SIZE(cap.args));  \
+        memcpy(cap.args, args_tmp, n * sizeof(cap.args[0]));         \
         kvm_vm_ioctl(s, KVM_ENABLE_CAP, &cap);                       \
     })
 
@@ -443,11 +440,8 @@ int kvm_vm_check_extension(KVMState *s, unsigned int extension);
             .flags = cap_flags,                                      \
         };                                                           \
         uint64_t args_tmp[] = { __VA_ARGS__ };                       \
-        int i;                                                       \
-        for (i = 0; i < (int)ARRAY_SIZE(args_tmp) &&                 \
-                     i < ARRAY_SIZE(cap.args); i++) {                \
-            cap.args[i] = args_tmp[i];                               \
-        }                                                            \
+        size_t n = MIN(ARRAY_SIZE(args_tmp), ARRAY_SIZE(cap.args));  \
+        memcpy(cap.args, args_tmp, n * sizeof(cap.args[0]));         \
         kvm_vcpu_ioctl(cpu, KVM_ENABLE_CAP, &cap);                   \
     })
 


Re: [Qemu-devel] [PATCH v2] kvm: workaround build break on gcc-7.1.1 / fedora26
Posted by Cornelia Huck 6 years, 7 months ago
On Mon, 07 Aug 2017 13:36:44 +0200
Greg Kurz <groug@kaod.org> 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
> 
> The compiler should obviously optimize this code away when no extra
> agument is passed to kvm_vm_enable_cap() and kvm_vcpu_enable_cap(),
> but it doesn't. This bug should be fixed one day in gcc, but we can
> also change our code pattern so that we don't hit the issue anymore.
> We workaround this, by using memcpy() instead of open-coding the copy.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> v2: - use memcpy()
> ---
>  include/sysemu/kvm.h |   14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)

Acked-by: Cornelia Huck <cohuck@redhat.com>

Re: [Qemu-devel] [PATCH v2] kvm: workaround build break on gcc-7.1.1 / fedora26
Posted by Paolo Bonzini 6 years, 7 months ago
On 07/08/2017 13:36, 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
> 
> The compiler should obviously optimize this code away when no extra
> agument is passed to kvm_vm_enable_cap() and kvm_vcpu_enable_cap(),
> but it doesn't. This bug should be fixed one day in gcc, but we can
> also change our code pattern so that we don't hit the issue anymore.
> We workaround this, by using memcpy() instead of open-coding the copy.

Nice way to do it, thanks.  I'll queue it for 2.10.

Paolo

> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> v2: - use memcpy()
> ---
>  include/sysemu/kvm.h |   14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 91fc07ee9afe..3a458f50e9f4 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -428,11 +428,8 @@ int kvm_vm_check_extension(KVMState *s, unsigned int extension);
>              .flags = cap_flags,                                      \
>          };                                                           \
>          uint64_t args_tmp[] = { __VA_ARGS__ };                       \
> -        int i;                                                       \
> -        for (i = 0; i < (int)ARRAY_SIZE(args_tmp) &&                 \
> -                     i < ARRAY_SIZE(cap.args); i++) {                \
> -            cap.args[i] = args_tmp[i];                               \
> -        }                                                            \
> +        size_t n = MIN(ARRAY_SIZE(args_tmp), ARRAY_SIZE(cap.args));  \
> +        memcpy(cap.args, args_tmp, n * sizeof(cap.args[0]));         \
>          kvm_vm_ioctl(s, KVM_ENABLE_CAP, &cap);                       \
>      })
>  
> @@ -443,11 +440,8 @@ int kvm_vm_check_extension(KVMState *s, unsigned int extension);
>              .flags = cap_flags,                                      \
>          };                                                           \
>          uint64_t args_tmp[] = { __VA_ARGS__ };                       \
> -        int i;                                                       \
> -        for (i = 0; i < (int)ARRAY_SIZE(args_tmp) &&                 \
> -                     i < ARRAY_SIZE(cap.args); i++) {                \
> -            cap.args[i] = args_tmp[i];                               \
> -        }                                                            \
> +        size_t n = MIN(ARRAY_SIZE(args_tmp), ARRAY_SIZE(cap.args));  \
> +        memcpy(cap.args, args_tmp, n * sizeof(cap.args[0]));         \
>          kvm_vcpu_ioctl(cpu, KVM_ENABLE_CAP, &cap);                   \
>      })
>  
> 


Re: [Qemu-devel] [PATCH v2] kvm: workaround build break on gcc-7.1.1 / fedora26
Posted by Philippe Mathieu-Daudé 6 years, 7 months ago
On Mon, Aug 7, 2017 at 8:55 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 07/08/2017 13:36, 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
>>
>> The compiler should obviously optimize this code away when no extra
>> agument is passed to kvm_vm_enable_cap() and kvm_vcpu_enable_cap(),
>> but it doesn't. This bug should be fixed one day in gcc, but we can
>> also change our code pattern so that we don't hit the issue anymore.
>> We workaround this, by using memcpy() instead of open-coding the copy.
>
> Nice way to do it, thanks.  I'll queue it for 2.10.

Indeed :)

>
> Paolo
>
>> Signed-off-by: Greg Kurz <groug@kaod.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>> ---
>> v2: - use memcpy()
>> ---
>>  include/sysemu/kvm.h |   14 ++++----------
>>  1 file changed, 4 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
>> index 91fc07ee9afe..3a458f50e9f4 100644
>> --- a/include/sysemu/kvm.h
>> +++ b/include/sysemu/kvm.h
>> @@ -428,11 +428,8 @@ int kvm_vm_check_extension(KVMState *s, unsigned int extension);
>>              .flags = cap_flags,                                      \
>>          };                                                           \
>>          uint64_t args_tmp[] = { __VA_ARGS__ };                       \
>> -        int i;                                                       \
>> -        for (i = 0; i < (int)ARRAY_SIZE(args_tmp) &&                 \
>> -                     i < ARRAY_SIZE(cap.args); i++) {                \
>> -            cap.args[i] = args_tmp[i];                               \
>> -        }                                                            \
>> +        size_t n = MIN(ARRAY_SIZE(args_tmp), ARRAY_SIZE(cap.args));  \
>> +        memcpy(cap.args, args_tmp, n * sizeof(cap.args[0]));         \
>>          kvm_vm_ioctl(s, KVM_ENABLE_CAP, &cap);                       \
>>      })
>>
>> @@ -443,11 +440,8 @@ int kvm_vm_check_extension(KVMState *s, unsigned int extension);
>>              .flags = cap_flags,                                      \
>>          };                                                           \
>>          uint64_t args_tmp[] = { __VA_ARGS__ };                       \
>> -        int i;                                                       \
>> -        for (i = 0; i < (int)ARRAY_SIZE(args_tmp) &&                 \
>> -                     i < ARRAY_SIZE(cap.args); i++) {                \
>> -            cap.args[i] = args_tmp[i];                               \
>> -        }                                                            \
>> +        size_t n = MIN(ARRAY_SIZE(args_tmp), ARRAY_SIZE(cap.args));  \
>> +        memcpy(cap.args, args_tmp, n * sizeof(cap.args[0]));         \
>>          kvm_vcpu_ioctl(cpu, KVM_ENABLE_CAP, &cap);                   \
>>      })
>>
>>
>