[Qemu-devel] [PATCH v2] xics_kvm: fix a build break

[Qemu-devel] [PATCH v2] xics_kvm: fix a build break

[Cédric Le Goater]

On CentOS 7.5, gcc-4.8.5-28.el7_5.1.ppc64le fails to build QEMU due to :

  hw/intc/xics_kvm.c: In function ‘ics_set_kvm_state’:
  hw/intc/xics_kvm.c:281:13: error: ‘ret’ may be used uninitialized in this
    function [-Werror=maybe-uninitialized]
             return ret;

Fix the breakage and also remove the extra error reporting as
kvm_device_access() already provides a substantial error message.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---

 Greg, can you please test ? RHEL7.5 does not catch the issue with the
 same compiler level :/ I don't understand why.

 hw/intc/xics_kvm.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index 8bdf6afe82a0..8dba2f84e71e 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -186,8 +186,7 @@ static void ics_get_kvm_state(ICSState *ics)
         kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
                           i + ics->offset, &state, false, &local_err);
         if (local_err) {
-            error_report("Unable to retrieve KVM interrupt controller state"
-                    " for IRQ %d: %s", i + ics->offset, strerror(errno));
+            error_report_err(local_err);
             exit(1);
         }
 
@@ -273,11 +272,10 @@ static int ics_set_kvm_state(ICSState *ics, int version_id)
                 state |= KVM_XICS_QUEUED;
         }
 
-        kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
-                          i + ics->offset, &state, true, &local_err);
+        ret = kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
+                                i + ics->offset, &state, true, &local_err);
         if (local_err) {
-            error_report("Unable to restore KVM interrupt controller state"
-                    " for IRQs %d: %s", i + ics->offset, strerror(errno));
+            error_report_err(local_err);
             return ret;
         }
     }
-- 
2.13.6

Re: [Qemu-devel] [PATCH v2] xics_kvm: fix a build break

[Greg Kurz]

On Tue, 12 Jun 2018 12:11:35 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On CentOS 7.5, gcc-4.8.5-28.el7_5.1.ppc64le fails to build QEMU due to :
> 
>   hw/intc/xics_kvm.c: In function ‘ics_set_kvm_state’:
>   hw/intc/xics_kvm.c:281:13: error: ‘ret’ may be used uninitialized in this
>     function [-Werror=maybe-uninitialized]
>              return ret;
> 
> Fix the breakage and also remove the extra error reporting as
> kvm_device_access() already provides a substantial error message.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> 
>  Greg, can you please test ? RHEL7.5 does not catch the issue with the
>  same compiler level :/ I don't understand why.
> 

This fixes the build breakage for me. Now this should really be squashed
into your original patch to preserve bisect on master.

Just one remark:

>  hw/intc/xics_kvm.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 8bdf6afe82a0..8dba2f84e71e 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -186,8 +186,7 @@ static void ics_get_kvm_state(ICSState *ics)
>          kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
>                            i + ics->offset, &state, false, &local_err);
>          if (local_err) {
> -            error_report("Unable to retrieve KVM interrupt controller state"
> -                    " for IRQ %d: %s", i + ics->offset, strerror(errno));
> +            error_report_err(local_err);

This would change the error message from:

Unable to retrieve KVM interrupt controller state for IRQ 4096: Invalid argument

to

KVM_GET_DEVICE_ATTR failed: Group 1 attr 0x0000000000001000: Invalid argument

I find it a bit cryptic, and we loose both the IRQ number and direct indication
that it is related to XICS on KVM. That's why I had suggested to print both with
error_reportf_err().

Since this would result in a very long line, alternatively, you can
keep the current error message, using -ret instead of errno, and call
error_free(local_err).

>              exit(1);
>          }
>  
> @@ -273,11 +272,10 @@ static int ics_set_kvm_state(ICSState *ics, int version_id)
>                  state |= KVM_XICS_QUEUED;
>          }
>  
> -        kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
> -                          i + ics->offset, &state, true, &local_err);
> +        ret = kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
> +                                i + ics->offset, &state, true, &local_err);
>          if (local_err) {
> -            error_report("Unable to restore KVM interrupt controller state"
> -                    " for IRQs %d: %s", i + ics->offset, strerror(errno));
> +            error_report_err(local_err);



>              return ret;
>          }
>      }

Re: [Qemu-devel] [PATCH v2] xics_kvm: fix a build break

[Cédric Le Goater]

On 06/12/2018 01:41 PM, Greg Kurz wrote:
> On Tue, 12 Jun 2018 12:11:35 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> On CentOS 7.5, gcc-4.8.5-28.el7_5.1.ppc64le fails to build QEMU due to :
>>
>>   hw/intc/xics_kvm.c: In function ‘ics_set_kvm_state’:
>>   hw/intc/xics_kvm.c:281:13: error: ‘ret’ may be used uninitialized in this
>>     function [-Werror=maybe-uninitialized]
>>              return ret;
>>
>> Fix the breakage and also remove the extra error reporting as
>> kvm_device_access() already provides a substantial error message.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>
>>  Greg, can you please test ? RHEL7.5 does not catch the issue with the
>>  same compiler level :/ I don't understand why.
>>
> 
> This fixes the build breakage for me. Now this should really be squashed
> into your original patch to preserve bisect on master.

After some analysis, you need to configure with '--enable-debug' to reproduce 
the breakage. '--enable-debug' removes -O2. 

With -O2, the compiler doesn't catch the error. It looks very much like a gcc 
issue to me.

Thanks,

C. 

Re: [Qemu-devel] [PATCH v2] xics_kvm: fix a build break

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2414 bytes --]

On Tue, Jun 12, 2018 at 12:11:35PM +0200, Cédric Le Goater wrote:
> On CentOS 7.5, gcc-4.8.5-28.el7_5.1.ppc64le fails to build QEMU due to :
> 
>   hw/intc/xics_kvm.c: In function ‘ics_set_kvm_state’:
>   hw/intc/xics_kvm.c:281:13: error: ‘ret’ may be used uninitialized in this
>     function [-Werror=maybe-uninitialized]
>              return ret;
> 
> Fix the breakage and also remove the extra error reporting as
> kvm_device_access() already provides a substantial error message.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Applied to ppc-for-3.0, thanks.

> ---
> 
>  Greg, can you please test ? RHEL7.5 does not catch the issue with the
>  same compiler level :/ I don't understand why.
> 
>  hw/intc/xics_kvm.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 8bdf6afe82a0..8dba2f84e71e 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -186,8 +186,7 @@ static void ics_get_kvm_state(ICSState *ics)
>          kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
>                            i + ics->offset, &state, false, &local_err);
>          if (local_err) {
> -            error_report("Unable to retrieve KVM interrupt controller state"
> -                    " for IRQ %d: %s", i + ics->offset, strerror(errno));
> +            error_report_err(local_err);
>              exit(1);
>          }
>  
> @@ -273,11 +272,10 @@ static int ics_set_kvm_state(ICSState *ics, int version_id)
>                  state |= KVM_XICS_QUEUED;
>          }
>  
> -        kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
> -                          i + ics->offset, &state, true, &local_err);
> +        ret = kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
> +                                i + ics->offset, &state, true, &local_err);
>          if (local_err) {
> -            error_report("Unable to restore KVM interrupt controller state"
> -                    " for IRQs %d: %s", i + ics->offset, strerror(errno));
> +            error_report_err(local_err);
>              return ret;
>          }
>      }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]