cpu: Remove pointless check

[PATCH] cpu: Remove pointless check

Posted by Martin Kletzander 3 years, 9 months ago

These two pointers can never be NULL since they are initialised to a reference
of a struct.  This became apparent when commit 210a19539447 added a VIR_DEBUG
which used both pointers because due to the concise condition the compiler saw
that if the "and" part of the condition did short-circuit (and it assumed that
can happen) the second variable would not be initialised, but it is used in the
debugging message, so the build failed with:

  In file included from ../src/cpu/cpu_x86.c:27:
  ../src/cpu/cpu_x86.c: In function ‘virCPUx86DataIsIdentical’:
  ../src/util/virlog.h:79:5: error: ‘bdata’ may be used uninitialized in this
  function [-Werror=maybe-uninitialized]

Fix this by just assigning the helper pointers and remove the condition
altogether.

Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
Pushed under the build-breaker rule, but feel free to review/object.

 src/cpu/cpu_x86.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
index 7e9d1cea47d1..a5eac7601cad 100644
--- a/src/cpu/cpu_x86.c
+++ b/src/cpu/cpu_x86.c
@@ -3314,10 +3314,8 @@ virCPUx86DataIsIdentical(const virCPUData *a,
         return VIR_CPU_COMPARE_INCOMPATIBLE;
     }
 
-    if (!((adata = &a->data.x86) && (bdata = &b->data.x86))) {
-        VIR_DEBUG("missing x86 data: a:%p b:%p", adata, bdata);
-        return VIR_CPU_COMPARE_ERROR;
-    }
+    adata = &a->data.x86;
+    bdata = &b->data.x86;
 
     if (adata->len != bdata->len) {
         VIR_DEBUG("unequal length a:%zu b:%zu", adata->len, bdata->len);
-- 
2.35.1

Re: [PATCH] cpu: Remove pointless check

Posted by Peter Krempa 3 years, 9 months ago

On Wed, Apr 27, 2022 at 09:51:34 +0200, Martin Kletzander wrote:
> These two pointers can never be NULL since they are initialised to a reference
> of a struct.  This became apparent when commit 210a19539447 added a VIR_DEBUG
> which used both pointers because due to the concise condition the compiler saw
> that if the "and" part of the condition did short-circuit (and it assumed that
> can happen) the second variable would not be initialised, but it is used in the
> debugging message, so the build failed with:
> 
>   In file included from ../src/cpu/cpu_x86.c:27:
>   ../src/cpu/cpu_x86.c: In function ‘virCPUx86DataIsIdentical’:
>   ../src/util/virlog.h:79:5: error: ‘bdata’ may be used uninitialized in this
>   function [-Werror=maybe-uninitialized]
> 
> Fix this by just assigning the helper pointers and remove the condition
> altogether.
> 
> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
> ---
> Pushed under the build-breaker rule, but feel free to review/object.

Oops! Unfortunately neither my complilers:

 clang version 13.0.0 (Fedora 13.0.0-3.fc35),
 gcc (GCC) 11.2.1 20220127 (Red Hat 11.2.1-9)

nor the CI:

 https://gitlab.com/pipo.sk/libvirt/-/pipelines/524030525 (besides the
 usual macos-11 fail)

complained. Which compiler did you use?


> 
>  src/cpu/cpu_x86.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
> index 7e9d1cea47d1..a5eac7601cad 100644
> --- a/src/cpu/cpu_x86.c
> +++ b/src/cpu/cpu_x86.c
> @@ -3314,10 +3314,8 @@ virCPUx86DataIsIdentical(const virCPUData *a,
>          return VIR_CPU_COMPARE_INCOMPATIBLE;
>      }
>  
> -    if (!((adata = &a->data.x86) && (bdata = &b->data.x86))) {
> -        VIR_DEBUG("missing x86 data: a:%p b:%p", adata, bdata);
> -        return VIR_CPU_COMPARE_ERROR;
> -    }
> +    adata = &a->data.x86;
> +    bdata = &b->data.x86;

Yup, checking whether the result of the '&' operator is non-null doesn't
make sense.

Re: [PATCH] cpu: Remove pointless check

Posted by Martin Kletzander 3 years, 9 months ago

On Wed, Apr 27, 2022 at 10:12:25AM +0200, Peter Krempa wrote:
>On Wed, Apr 27, 2022 at 09:51:34 +0200, Martin Kletzander wrote:
>> These two pointers can never be NULL since they are initialised to a reference
>> of a struct.  This became apparent when commit 210a19539447 added a VIR_DEBUG
>> which used both pointers because due to the concise condition the compiler saw
>> that if the "and" part of the condition did short-circuit (and it assumed that
>> can happen) the second variable would not be initialised, but it is used in the
>> debugging message, so the build failed with:
>>
>>   In file included from ../src/cpu/cpu_x86.c:27:
>>   ../src/cpu/cpu_x86.c: In function ‘virCPUx86DataIsIdentical’:
>>   ../src/util/virlog.h:79:5: error: ‘bdata’ may be used uninitialized in this
>>   function [-Werror=maybe-uninitialized]
>>
>> Fix this by just assigning the helper pointers and remove the condition
>> altogether.
>>
>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
>> ---
>> Pushed under the build-breaker rule, but feel free to review/object.
>
>Oops! Unfortunately neither my complilers:
>
> clang version 13.0.0 (Fedora 13.0.0-3.fc35),
> gcc (GCC) 11.2.1 20220127 (Red Hat 11.2.1-9)
>
>nor the CI:
>
> https://gitlab.com/pipo.sk/libvirt/-/pipelines/524030525 (besides the
> usual macos-11 fail)
>
>complained. Which compiler did you use?
>

gcc (Gentoo 11.3.0 p4) 11.3.0

>
>>
>>  src/cpu/cpu_x86.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
>> index 7e9d1cea47d1..a5eac7601cad 100644
>> --- a/src/cpu/cpu_x86.c
>> +++ b/src/cpu/cpu_x86.c
>> @@ -3314,10 +3314,8 @@ virCPUx86DataIsIdentical(const virCPUData *a,
>>          return VIR_CPU_COMPARE_INCOMPATIBLE;
>>      }
>>
>> -    if (!((adata = &a->data.x86) && (bdata = &b->data.x86))) {
>> -        VIR_DEBUG("missing x86 data: a:%p b:%p", adata, bdata);
>> -        return VIR_CPU_COMPARE_ERROR;
>> -    }
>> +    adata = &a->data.x86;
>> +    bdata = &b->data.x86;
>
>Yup, checking whether the result of the '&' operator is non-null doesn't
>make sense.
>