diff mbox series

[v2] target/i386: Remove compilation errors when -Werror=maybe-uninitialized

Message ID 20221221163652.1239362-1-eric.auger@redhat.com (mailing list archive)
State New, archived
Headers show
Series [v2] target/i386: Remove compilation errors when -Werror=maybe-uninitialized | expand

Commit Message

Eric Auger Dec. 21, 2022, 4:36 p.m. UTC
To avoid compilation errors when -Werror=maybe-uninitialized is used,
replace 'case 3' by 'default'.

Otherwise we get:

../target/i386/ops_sse.h: In function ‘helper_vpermdq_ymm’:
../target/i386/ops_sse.h:2495:13: error: ‘r3’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
   2495 |     d->Q(3) = r3;
        |     ~~~~~~~~^~~~
../target/i386/ops_sse.h:2494:13: error: ‘r2’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
   2494 |     d->Q(2) = r2;
        |     ~~~~~~~~^~~~
../target/i386/ops_sse.h:2493:13: error: ‘r1’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
   2493 |     d->Q(1) = r1;
        |     ~~~~~~~~^~~~
../target/i386/ops_sse.h:2492:13: error: ‘r0’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
   2492 |     d->Q(0) = r0;
        |     ~~~~~~~~^~~~

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Suggested-by: Stefan Weil <sw@weilnetz.de>
Fixes: 790684776861 ("target/i386: reimplement 0x0f 0x3a, add AVX")
---
 target/i386/ops_sse.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Stefan Weil Dec. 21, 2022, 4:49 p.m. UTC | #1
Am 21.12.22 um 17:36 schrieb Eric Auger:

> To avoid compilation errors when -Werror=maybe-uninitialized is used,
> replace 'case 3' by 'default'.
>
> Otherwise we get:
>
> ../target/i386/ops_sse.h: In function ‘helper_vpermdq_ymm’:
> ../target/i386/ops_sse.h:2495:13: error: ‘r3’ may be used
> uninitialized in this function [-Werror=maybe-uninitialized]
>     2495 |     d->Q(3) = r3;
>          |     ~~~~~~~~^~~~
> ../target/i386/ops_sse.h:2494:13: error: ‘r2’ may be used
> uninitialized in this function [-Werror=maybe-uninitialized]
>     2494 |     d->Q(2) = r2;
>          |     ~~~~~~~~^~~~
> ../target/i386/ops_sse.h:2493:13: error: ‘r1’ may be used
> uninitialized in this function [-Werror=maybe-uninitialized]
>     2493 |     d->Q(1) = r1;
>          |     ~~~~~~~~^~~~
> ../target/i386/ops_sse.h:2492:13: error: ‘r0’ may be used
> uninitialized in this function [-Werror=maybe-uninitialized]
>     2492 |     d->Q(0) = r0;
>          |     ~~~~~~~~^~~~
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Suggested-by: Stefan Weil <sw@weilnetz.de>
> Fixes: 790684776861 ("target/i386: reimplement 0x0f 0x3a, add AVX")
> ---
>   target/i386/ops_sse.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/i386/ops_sse.h b/target/i386/ops_sse.h
> index 3cbc36a59d..c442c8c10c 100644
> --- a/target/i386/ops_sse.h
> +++ b/target/i386/ops_sse.h
> @@ -2466,7 +2466,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s, uint32_t order)
>           r0 = s->Q(0);
>           r1 = s->Q(1);
>           break;
> -    case 3:
> +    default:
>           r0 = s->Q(2);
>           r1 = s->Q(3);
>           break;
> @@ -2484,7 +2484,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s, uint32_t order)
>           r2 = s->Q(0);
>           r3 = s->Q(1);
>           break;
> -    case 3:
> +    default:
>           r2 = s->Q(2);
>           r3 = s->Q(3);
>           break;


Reviewed-by: Stefan Weil <sw@weilnetz.de>

Thank you and merry Christmas!
Paolo Bonzini Dec. 22, 2022, 8:18 a.m. UTC | #2
On 12/21/22 17:36, Eric Auger wrote:
> To avoid compilation errors when -Werror=maybe-uninitialized is used,
> replace 'case 3' by 'default'.
> 
> Otherwise we get:
> 
> ../target/i386/ops_sse.h: In function ‘helper_vpermdq_ymm’:
> ../target/i386/ops_sse.h:2495:13: error: ‘r3’ may be used
> uninitialized in this function [-Werror=maybe-uninitialized]
>     2495 |     d->Q(3) = r3;
>          |     ~~~~~~~~^~~~
> ../target/i386/ops_sse.h:2494:13: error: ‘r2’ may be used
> uninitialized in this function [-Werror=maybe-uninitialized]
>     2494 |     d->Q(2) = r2;
>          |     ~~~~~~~~^~~~
> ../target/i386/ops_sse.h:2493:13: error: ‘r1’ may be used
> uninitialized in this function [-Werror=maybe-uninitialized]
>     2493 |     d->Q(1) = r1;
>          |     ~~~~~~~~^~~~
> ../target/i386/ops_sse.h:2492:13: error: ‘r0’ may be used
> uninitialized in this function [-Werror=maybe-uninitialized]
>     2492 |     d->Q(0) = r0;
>          |     ~~~~~~~~^~~~
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Suggested-by: Stefan Weil <sw@weilnetz.de>
> Fixes: 790684776861 ("target/i386: reimplement 0x0f 0x3a, add AVX")
> ---
>   target/i386/ops_sse.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/ops_sse.h b/target/i386/ops_sse.h
> index 3cbc36a59d..c442c8c10c 100644
> --- a/target/i386/ops_sse.h
> +++ b/target/i386/ops_sse.h
> @@ -2466,7 +2466,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s, uint32_t order)
>           r0 = s->Q(0);
>           r1 = s->Q(1);
>           break;
> -    case 3:
> +    default:
>           r0 = s->Q(2);
>           r1 = s->Q(3);
>           break;
> @@ -2484,7 +2484,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s, uint32_t order)
>           r2 = s->Q(0);
>           r3 = s->Q(1);
>           break;
> -    case 3:
> +    default:
>           r2 = s->Q(2);
>           r3 = s->Q(3);
>           break;

Queued, but this compiler sucks. :)

Paolo
Philippe Mathieu-Daudé Dec. 22, 2022, 9:01 a.m. UTC | #3
On 22/12/22 09:18, Paolo Bonzini wrote:
> On 12/21/22 17:36, Eric Auger wrote:
>> To avoid compilation errors when -Werror=maybe-uninitialized is used,
>> replace 'case 3' by 'default'.
>>
>> Otherwise we get:
>>
>> ../target/i386/ops_sse.h: In function ‘helper_vpermdq_ymm’:
>> ../target/i386/ops_sse.h:2495:13: error: ‘r3’ may be used
>> uninitialized in this function [-Werror=maybe-uninitialized]
>>     2495 |     d->Q(3) = r3;
>>          |     ~~~~~~~~^~~~
>> ../target/i386/ops_sse.h:2494:13: error: ‘r2’ may be used
>> uninitialized in this function [-Werror=maybe-uninitialized]
>>     2494 |     d->Q(2) = r2;
>>          |     ~~~~~~~~^~~~
>> ../target/i386/ops_sse.h:2493:13: error: ‘r1’ may be used
>> uninitialized in this function [-Werror=maybe-uninitialized]
>>     2493 |     d->Q(1) = r1;
>>          |     ~~~~~~~~^~~~
>> ../target/i386/ops_sse.h:2492:13: error: ‘r0’ may be used
>> uninitialized in this function [-Werror=maybe-uninitialized]
>>     2492 |     d->Q(0) = r0;
>>          |     ~~~~~~~~^~~~

With what compiler? Is that a supported one?

>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Suggested-by: Stefan Weil <sw@weilnetz.de>
>> Fixes: 790684776861 ("target/i386: reimplement 0x0f 0x3a, add AVX")
>> ---
>>   target/i386/ops_sse.h | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/i386/ops_sse.h b/target/i386/ops_sse.h
>> index 3cbc36a59d..c442c8c10c 100644
>> --- a/target/i386/ops_sse.h
>> +++ b/target/i386/ops_sse.h
>> @@ -2466,7 +2466,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s, 
>> uint32_t order)
>>           r0 = s->Q(0);
>>           r1 = s->Q(1);
>>           break;
>> -    case 3:
>> +    default:
>>           r0 = s->Q(2);
>>           r1 = s->Q(3);
>>           break;
>> @@ -2484,7 +2484,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s, 
>> uint32_t order)
>>           r2 = s->Q(0);
>>           r3 = s->Q(1);
>>           break;
>> -    case 3:
>> +    default:
>>           r2 = s->Q(2);
>>           r3 = s->Q(3);
>>           break;
> 
> Queued, but this compiler sucks. :)

Can't we simply add a dumb 'default' case? So when reviewing we don't
have to evaluate 'default' means 3 here.

-- >8 --
--- a/target/i386/ops_sse.h
+++ b/target/i386/ops_sse.h
@@ -2470,6 +2470,8 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s, 
uint32_t order)
          r0 = s->Q(2);
          r1 = s->Q(3);
          break;
+    default:
+        qemu_build_not_reached();
      }
      switch ((order >> 4) & 3) {
      case 0:
@@ -2488,6 +2490,8 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s, 
uint32_t order)
          r2 = s->Q(2);
          r3 = s->Q(3);
          break;
+    default:
+        qemu_build_not_reached();
      }
      d->Q(0) = r0;
      d->Q(1) = r1;
---
Eric Auger Dec. 22, 2022, 10:07 a.m. UTC | #4
Hi Philippe,

On 12/22/22 10:01, Philippe Mathieu-Daudé wrote:
> On 22/12/22 09:18, Paolo Bonzini wrote:
>> On 12/21/22 17:36, Eric Auger wrote:
>>> To avoid compilation errors when -Werror=maybe-uninitialized is used,
>>> replace 'case 3' by 'default'.
>>>
>>> Otherwise we get:
>>>
>>> ../target/i386/ops_sse.h: In function ‘helper_vpermdq_ymm’:
>>> ../target/i386/ops_sse.h:2495:13: error: ‘r3’ may be used
>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>     2495 |     d->Q(3) = r3;
>>>          |     ~~~~~~~~^~~~
>>> ../target/i386/ops_sse.h:2494:13: error: ‘r2’ may be used
>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>     2494 |     d->Q(2) = r2;
>>>          |     ~~~~~~~~^~~~
>>> ../target/i386/ops_sse.h:2493:13: error: ‘r1’ may be used
>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>     2493 |     d->Q(1) = r1;
>>>          |     ~~~~~~~~^~~~
>>> ../target/i386/ops_sse.h:2492:13: error: ‘r0’ may be used
>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>     2492 |     d->Q(0) = r0;
>>>          |     ~~~~~~~~^~~~
>
> With what compiler? Is that a supported one?
https://lore.kernel.org/qemu-devel/3aab489e-9d90-c1ad-0b6b-b2b5d80db723@redhat.com/
>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> Suggested-by: Stefan Weil <sw@weilnetz.de>
>>> Fixes: 790684776861 ("target/i386: reimplement 0x0f 0x3a, add AVX")
>>> ---
>>>   target/i386/ops_sse.h | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/target/i386/ops_sse.h b/target/i386/ops_sse.h
>>> index 3cbc36a59d..c442c8c10c 100644
>>> --- a/target/i386/ops_sse.h
>>> +++ b/target/i386/ops_sse.h
>>> @@ -2466,7 +2466,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg
>>> *s, uint32_t order)
>>>           r0 = s->Q(0);
>>>           r1 = s->Q(1);
>>>           break;
>>> -    case 3:
>>> +    default:
>>>           r0 = s->Q(2);
>>>           r1 = s->Q(3);
>>>           break;
>>> @@ -2484,7 +2484,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg
>>> *s, uint32_t order)
>>>           r2 = s->Q(0);
>>>           r3 = s->Q(1);
>>>           break;
>>> -    case 3:
>>> +    default:
>>>           r2 = s->Q(2);
>>>           r3 = s->Q(3);
>>>           break;
>>
>> Queued, but this compiler sucks. :)
>
> Can't we simply add a dumb 'default' case? So when reviewing we don't
> have to evaluate 'default' means 3 here.
>
> -- >8 --
> --- a/target/i386/ops_sse.h
> +++ b/target/i386/ops_sse.h
> @@ -2470,6 +2470,8 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s,
> uint32_t order)
>          r0 = s->Q(2);
>          r1 = s->Q(3);
>          break;
> +    default:
> +        qemu_build_not_reached();
>      }
>      switch ((order >> 4) & 3) {
>      case 0:
> @@ -2488,6 +2490,8 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s,
> uint32_t order)
>          r2 = s->Q(2);
>          r3 = s->Q(3);
>          break;
> +    default:
> +        qemu_build_not_reached();
>      }
I guess this won't fix the fact r0, r1, r2, r3 are not initialized, will it?

Thanks

Eric
>      d->Q(0) = r0;
>      d->Q(1) = r1;
> ---
>
Philippe Mathieu-Daudé Dec. 22, 2022, 10:33 a.m. UTC | #5
On 22/12/22 11:07, Eric Auger wrote:
> Hi Philippe,
> 
> On 12/22/22 10:01, Philippe Mathieu-Daudé wrote:
>> On 22/12/22 09:18, Paolo Bonzini wrote:
>>> On 12/21/22 17:36, Eric Auger wrote:
>>>> To avoid compilation errors when -Werror=maybe-uninitialized is used,
>>>> replace 'case 3' by 'default'.
>>>>
>>>> Otherwise we get:
>>>>
>>>> ../target/i386/ops_sse.h: In function ‘helper_vpermdq_ymm’:
>>>> ../target/i386/ops_sse.h:2495:13: error: ‘r3’ may be used
>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>      2495 |     d->Q(3) = r3;
>>>>           |     ~~~~~~~~^~~~
>>>> ../target/i386/ops_sse.h:2494:13: error: ‘r2’ may be used
>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>      2494 |     d->Q(2) = r2;
>>>>           |     ~~~~~~~~^~~~
>>>> ../target/i386/ops_sse.h:2493:13: error: ‘r1’ may be used
>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>      2493 |     d->Q(1) = r1;
>>>>           |     ~~~~~~~~^~~~
>>>> ../target/i386/ops_sse.h:2492:13: error: ‘r0’ may be used
>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>      2492 |     d->Q(0) = r0;
>>>>           |     ~~~~~~~~^~~~
>>
>> With what compiler? Is that a supported one?
> https://lore.kernel.org/qemu-devel/3aab489e-9d90-c1ad-0b6b-b2b5d80db723@redhat.com/

Adding the compiler version in the commit description would help:

--
Using GCC 11.3.1 "cc (GCC) 11.3.1 20220421 (Red Hat 11.3.1-2)" we get:
--

>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>> Suggested-by: Stefan Weil <sw@weilnetz.de>
>>>> Fixes: 790684776861 ("target/i386: reimplement 0x0f 0x3a, add AVX")
>>>> ---
>>>>    target/i386/ops_sse.h | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/target/i386/ops_sse.h b/target/i386/ops_sse.h
>>>> index 3cbc36a59d..c442c8c10c 100644
>>>> --- a/target/i386/ops_sse.h
>>>> +++ b/target/i386/ops_sse.h
>>>> @@ -2466,7 +2466,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg
>>>> *s, uint32_t order)
>>>>            r0 = s->Q(0);
>>>>            r1 = s->Q(1);
>>>>            break;
>>>> -    case 3:
>>>> +    default:
>>>>            r0 = s->Q(2);
>>>>            r1 = s->Q(3);
>>>>            break;
>>>> @@ -2484,7 +2484,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg
>>>> *s, uint32_t order)
>>>>            r2 = s->Q(0);
>>>>            r3 = s->Q(1);
>>>>            break;
>>>> -    case 3:
>>>> +    default:
>>>>            r2 = s->Q(2);
>>>>            r3 = s->Q(3);
>>>>            break;
>>>
>>> Queued, but this compiler sucks. :)
>>
>> Can't we simply add a dumb 'default' case? So when reviewing we don't
>> have to evaluate 'default' means 3 here.
>>
>> -- >8 --
>> --- a/target/i386/ops_sse.h
>> +++ b/target/i386/ops_sse.h
>> @@ -2470,6 +2470,8 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s,
>> uint32_t order)
>>           r0 = s->Q(2);
>>           r1 = s->Q(3);
>>           break;
>> +    default:
>> +        qemu_build_not_reached();
>>       }
>>       switch ((order >> 4) & 3) {
>>       case 0:
>> @@ -2488,6 +2490,8 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s,
>> uint32_t order)
>>           r2 = s->Q(2);
>>           r3 = s->Q(3);
>>           break;
>> +    default:
>> +        qemu_build_not_reached();
>>       }
> I guess this won't fix the fact r0, r1, r2, r3 are not initialized, will it?

Well my compiler (Apple clang version 14.0.0 (clang-1400.0.29.202))
doesn't display the warning, I don't have yours handy to test it :)
Bernhard Beschow Dec. 22, 2022, 10:52 a.m. UTC | #6
Am 22. Dezember 2022 09:01:34 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 22/12/22 09:18, Paolo Bonzini wrote:
>> On 12/21/22 17:36, Eric Auger wrote:
>>> To avoid compilation errors when -Werror=maybe-uninitialized is used,
>>> replace 'case 3' by 'default'.
>>> 
>>> Otherwise we get:
>>> 
>>> ../target/i386/ops_sse.h: In function ‘helper_vpermdq_ymm’:
>>> ../target/i386/ops_sse.h:2495:13: error: ‘r3’ may be used
>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>     2495 |     d->Q(3) = r3;
>>>          |     ~~~~~~~~^~~~
>>> ../target/i386/ops_sse.h:2494:13: error: ‘r2’ may be used
>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>     2494 |     d->Q(2) = r2;
>>>          |     ~~~~~~~~^~~~
>>> ../target/i386/ops_sse.h:2493:13: error: ‘r1’ may be used
>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>     2493 |     d->Q(1) = r1;
>>>          |     ~~~~~~~~^~~~
>>> ../target/i386/ops_sse.h:2492:13: error: ‘r0’ may be used
>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>     2492 |     d->Q(0) = r0;
>>>          |     ~~~~~~~~^~~~
>
>With what compiler? Is that a supported one?
>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>> Suggested-by: Stefan Weil <sw@weilnetz.de>
>>> Fixes: 790684776861 ("target/i386: reimplement 0x0f 0x3a, add AVX")
>>> ---
>>>   target/i386/ops_sse.h | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/target/i386/ops_sse.h b/target/i386/ops_sse.h
>>> index 3cbc36a59d..c442c8c10c 100644
>>> --- a/target/i386/ops_sse.h
>>> +++ b/target/i386/ops_sse.h
>>> @@ -2466,7 +2466,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s, uint32_t order)
>>>           r0 = s->Q(0);
>>>           r1 = s->Q(1);
>>>           break;
>>> -    case 3:
>>> +    default:
>>>           r0 = s->Q(2);
>>>           r1 = s->Q(3);
>>>           break;
>>> @@ -2484,7 +2484,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s, uint32_t order)
>>>           r2 = s->Q(0);
>>>           r3 = s->Q(1);
>>>           break;
>>> -    case 3:
>>> +    default:
>>>           r2 = s->Q(2);
>>>           r3 = s->Q(3);
>>>           break;
>> 
>> Queued, but this compiler sucks. :)
>
>Can't we simply add a dumb 'default' case? So when reviewing we don't
>have to evaluate 'default' means 3 here.

I agree. At least it deserves a comment to preserve the original intention.

Best regards,
Bernhard

>-- >8 --
>--- a/target/i386/ops_sse.h
>+++ b/target/i386/ops_sse.h
>@@ -2470,6 +2470,8 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s, uint32_t order)
>         r0 = s->Q(2);
>         r1 = s->Q(3);
>         break;
>+    default:
>+        qemu_build_not_reached();
>     }
>     switch ((order >> 4) & 3) {
>     case 0:
>@@ -2488,6 +2490,8 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s, uint32_t order)
>         r2 = s->Q(2);
>         r3 = s->Q(3);
>         break;
>+    default:
>+        qemu_build_not_reached();
>     }
>     d->Q(0) = r0;
>     d->Q(1) = r1;
>---
>
Daniel P. Berrangé Dec. 22, 2022, 11:09 a.m. UTC | #7
On Thu, Dec 22, 2022 at 11:07:31AM +0100, Eric Auger wrote:
> Hi Philippe,
> 
> On 12/22/22 10:01, Philippe Mathieu-Daudé wrote:
> > On 22/12/22 09:18, Paolo Bonzini wrote:
> >> On 12/21/22 17:36, Eric Auger wrote:
> >>> To avoid compilation errors when -Werror=maybe-uninitialized is used,
> >>> replace 'case 3' by 'default'.
> >>>
> >>> Otherwise we get:
> >>>
> >>> ../target/i386/ops_sse.h: In function ‘helper_vpermdq_ymm’:
> >>> ../target/i386/ops_sse.h:2495:13: error: ‘r3’ may be used
> >>> uninitialized in this function [-Werror=maybe-uninitialized]
> >>>     2495 |     d->Q(3) = r3;
> >>>          |     ~~~~~~~~^~~~
> >>> ../target/i386/ops_sse.h:2494:13: error: ‘r2’ may be used
> >>> uninitialized in this function [-Werror=maybe-uninitialized]
> >>>     2494 |     d->Q(2) = r2;
> >>>          |     ~~~~~~~~^~~~
> >>> ../target/i386/ops_sse.h:2493:13: error: ‘r1’ may be used
> >>> uninitialized in this function [-Werror=maybe-uninitialized]
> >>>     2493 |     d->Q(1) = r1;
> >>>          |     ~~~~~~~~^~~~
> >>> ../target/i386/ops_sse.h:2492:13: error: ‘r0’ may be used
> >>> uninitialized in this function [-Werror=maybe-uninitialized]
> >>>     2492 |     d->Q(0) = r0;
> >>>          |     ~~~~~~~~^~~~
> >
> > With what compiler? Is that a supported one?
> https://lore.kernel.org/qemu-devel/3aab489e-9d90-c1ad-0b6b-b2b5d80db723@redhat.com/
> >
> >>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >>> Suggested-by: Stefan Weil <sw@weilnetz.de>
> >>> Fixes: 790684776861 ("target/i386: reimplement 0x0f 0x3a, add AVX")
> >>> ---
> >>>   target/i386/ops_sse.h | 4 ++--
> >>>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/target/i386/ops_sse.h b/target/i386/ops_sse.h
> >>> index 3cbc36a59d..c442c8c10c 100644
> >>> --- a/target/i386/ops_sse.h
> >>> +++ b/target/i386/ops_sse.h
> >>> @@ -2466,7 +2466,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg
> >>> *s, uint32_t order)
> >>>           r0 = s->Q(0);
> >>>           r1 = s->Q(1);
> >>>           break;
> >>> -    case 3:
> >>> +    default:
> >>>           r0 = s->Q(2);
> >>>           r1 = s->Q(3);
> >>>           break;
> >>> @@ -2484,7 +2484,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg
> >>> *s, uint32_t order)
> >>>           r2 = s->Q(0);
> >>>           r3 = s->Q(1);
> >>>           break;
> >>> -    case 3:
> >>> +    default:
> >>>           r2 = s->Q(2);
> >>>           r3 = s->Q(3);
> >>>           break;
> >>
> >> Queued, but this compiler sucks. :)
> >
> > Can't we simply add a dumb 'default' case? So when reviewing we don't
> > have to evaluate 'default' means 3 here.
> >
> > -- >8 --
> > --- a/target/i386/ops_sse.h
> > +++ b/target/i386/ops_sse.h
> > @@ -2470,6 +2470,8 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s,
> > uint32_t order)
> >          r0 = s->Q(2);
> >          r1 = s->Q(3);
> >          break;
> > +    default:
> > +        qemu_build_not_reached();
> >      }
> >      switch ((order >> 4) & 3) {
> >      case 0:
> > @@ -2488,6 +2490,8 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s,
> > uint32_t order)
> >          r2 = s->Q(2);
> >          r3 = s->Q(3);
> >          break;
> > +    default:
> > +        qemu_build_not_reached();
> >      }
> I guess this won't fix the fact r0, r1, r2, r3 are not initialized, will it?

This ultimately expands to assert() and the compiler should see that it
terminates the control flow at this point, so shouldn't have a reason
to warn.


With regards,
Daniel
Eric Auger Dec. 22, 2022, 11:18 a.m. UTC | #8
Hi All,

On 12/22/22 12:09, Daniel P. Berrangé wrote:
> On Thu, Dec 22, 2022 at 11:07:31AM +0100, Eric Auger wrote:
>> Hi Philippe,
>>
>> On 12/22/22 10:01, Philippe Mathieu-Daudé wrote:
>>> On 22/12/22 09:18, Paolo Bonzini wrote:
>>>> On 12/21/22 17:36, Eric Auger wrote:
>>>>> To avoid compilation errors when -Werror=maybe-uninitialized is used,
>>>>> replace 'case 3' by 'default'.
>>>>>
>>>>> Otherwise we get:
>>>>>
>>>>> ../target/i386/ops_sse.h: In function ‘helper_vpermdq_ymm’:
>>>>> ../target/i386/ops_sse.h:2495:13: error: ‘r3’ may be used
>>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>>     2495 |     d->Q(3) = r3;
>>>>>          |     ~~~~~~~~^~~~
>>>>> ../target/i386/ops_sse.h:2494:13: error: ‘r2’ may be used
>>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>>     2494 |     d->Q(2) = r2;
>>>>>          |     ~~~~~~~~^~~~
>>>>> ../target/i386/ops_sse.h:2493:13: error: ‘r1’ may be used
>>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>>     2493 |     d->Q(1) = r1;
>>>>>          |     ~~~~~~~~^~~~
>>>>> ../target/i386/ops_sse.h:2492:13: error: ‘r0’ may be used
>>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>>     2492 |     d->Q(0) = r0;
>>>>>          |     ~~~~~~~~^~~~
>>> With what compiler? Is that a supported one?
>> https://lore.kernel.org/qemu-devel/3aab489e-9d90-c1ad-0b6b-b2b5d80db723@redhat.com/
>>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>>> Suggested-by: Stefan Weil <sw@weilnetz.de>
>>>>> Fixes: 790684776861 ("target/i386: reimplement 0x0f 0x3a, add AVX")
>>>>> ---
>>>>>   target/i386/ops_sse.h | 4 ++--
>>>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/target/i386/ops_sse.h b/target/i386/ops_sse.h
>>>>> index 3cbc36a59d..c442c8c10c 100644
>>>>> --- a/target/i386/ops_sse.h
>>>>> +++ b/target/i386/ops_sse.h
>>>>> @@ -2466,7 +2466,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg
>>>>> *s, uint32_t order)
>>>>>           r0 = s->Q(0);
>>>>>           r1 = s->Q(1);
>>>>>           break;
>>>>> -    case 3:
>>>>> +    default:
>>>>>           r0 = s->Q(2);
>>>>>           r1 = s->Q(3);
>>>>>           break;
>>>>> @@ -2484,7 +2484,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg
>>>>> *s, uint32_t order)
>>>>>           r2 = s->Q(0);
>>>>>           r3 = s->Q(1);
>>>>>           break;
>>>>> -    case 3:
>>>>> +    default:
>>>>>           r2 = s->Q(2);
>>>>>           r3 = s->Q(3);
>>>>>           break;
>>>> Queued, but this compiler sucks. :)
>>> Can't we simply add a dumb 'default' case? So when reviewing we don't
>>> have to evaluate 'default' means 3 here.
>>>
>>> -- >8 --
>>> --- a/target/i386/ops_sse.h
>>> +++ b/target/i386/ops_sse.h
>>> @@ -2470,6 +2470,8 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s,
>>> uint32_t order)
>>>          r0 = s->Q(2);
>>>          r1 = s->Q(3);
>>>          break;
>>> +    default:
>>> +        qemu_build_not_reached();
>>>      }
>>>      switch ((order >> 4) & 3) {
>>>      case 0:
>>> @@ -2488,6 +2490,8 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s,
>>> uint32_t order)
>>>          r2 = s->Q(2);
>>>          r3 = s->Q(3);
>>>          break;
>>> +    default:
>>> +        qemu_build_not_reached();
>>>      }
>> I guess this won't fix the fact r0, r1, r2, r3 are not initialized, will it?
> This ultimately expands to assert() and the compiler should see that it
> terminates the control flow at this point, so shouldn't have a reason
> to warn.

OK so with qemu_build_not_reached(); I get

/home/augere/UPSTREAM/qemu/include/qemu/osdep.h:184:35: error: call to
‘qemu_build_not_reached_always’ declared with attribute error: code path
is reachable
  184 | #define qemu_build_not_reached()  qemu_build_not_reached_always()
      |                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


However with g_assert_not_reached(), it does not complain and errors are
removed. So I will respin with g_assert_not_reached() if nobody advises
me against that.

Thanks

Eric

>
>
> With regards,
> Daniel
Philippe Mathieu-Daudé Dec. 22, 2022, 11:51 a.m. UTC | #9
On 22/12/22 12:18, Eric Auger wrote:
> Hi All,
> 
> On 12/22/22 12:09, Daniel P. Berrangé wrote:
>> On Thu, Dec 22, 2022 at 11:07:31AM +0100, Eric Auger wrote:
>>> Hi Philippe,
>>>
>>> On 12/22/22 10:01, Philippe Mathieu-Daudé wrote:
>>>> On 22/12/22 09:18, Paolo Bonzini wrote:
>>>>> On 12/21/22 17:36, Eric Auger wrote:
>>>>>> To avoid compilation errors when -Werror=maybe-uninitialized is used,
>>>>>> replace 'case 3' by 'default'.
>>>>>>
>>>>>> Otherwise we get:
>>>>>>
>>>>>> ../target/i386/ops_sse.h: In function ‘helper_vpermdq_ymm’:
>>>>>> ../target/i386/ops_sse.h:2495:13: error: ‘r3’ may be used
>>>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>>>      2495 |     d->Q(3) = r3;
>>>>>>           |     ~~~~~~~~^~~~
>>>>>> ../target/i386/ops_sse.h:2494:13: error: ‘r2’ may be used
>>>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>>>      2494 |     d->Q(2) = r2;
>>>>>>           |     ~~~~~~~~^~~~
>>>>>> ../target/i386/ops_sse.h:2493:13: error: ‘r1’ may be used
>>>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>>>      2493 |     d->Q(1) = r1;
>>>>>>           |     ~~~~~~~~^~~~
>>>>>> ../target/i386/ops_sse.h:2492:13: error: ‘r0’ may be used
>>>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>>>      2492 |     d->Q(0) = r0;
>>>>>>           |     ~~~~~~~~^~~~
>>>> With what compiler? Is that a supported one?
>>> https://lore.kernel.org/qemu-devel/3aab489e-9d90-c1ad-0b6b-b2b5d80db723@redhat.com/
>>>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>>>> Suggested-by: Stefan Weil <sw@weilnetz.de>
>>>>>> Fixes: 790684776861 ("target/i386: reimplement 0x0f 0x3a, add AVX")
>>>>>> ---
>>>>>>    target/i386/ops_sse.h | 4 ++--
>>>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/target/i386/ops_sse.h b/target/i386/ops_sse.h
>>>>>> index 3cbc36a59d..c442c8c10c 100644
>>>>>> --- a/target/i386/ops_sse.h
>>>>>> +++ b/target/i386/ops_sse.h
>>>>>> @@ -2466,7 +2466,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg
>>>>>> *s, uint32_t order)
>>>>>>            r0 = s->Q(0);
>>>>>>            r1 = s->Q(1);
>>>>>>            break;
>>>>>> -    case 3:
>>>>>> +    default:
>>>>>>            r0 = s->Q(2);
>>>>>>            r1 = s->Q(3);
>>>>>>            break;
>>>>>> @@ -2484,7 +2484,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg
>>>>>> *s, uint32_t order)
>>>>>>            r2 = s->Q(0);
>>>>>>            r3 = s->Q(1);
>>>>>>            break;
>>>>>> -    case 3:
>>>>>> +    default:
>>>>>>            r2 = s->Q(2);
>>>>>>            r3 = s->Q(3);
>>>>>>            break;
>>>>> Queued, but this compiler sucks. :)
>>>> Can't we simply add a dumb 'default' case? So when reviewing we don't
>>>> have to evaluate 'default' means 3 here.
>>>>
>>>> -- >8 --
>>>> --- a/target/i386/ops_sse.h
>>>> +++ b/target/i386/ops_sse.h
>>>> @@ -2470,6 +2470,8 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s,
>>>> uint32_t order)
>>>>           r0 = s->Q(2);
>>>>           r1 = s->Q(3);
>>>>           break;
>>>> +    default:
>>>> +        qemu_build_not_reached();
>>>>       }
>>>>       switch ((order >> 4) & 3) {
>>>>       case 0:
>>>> @@ -2488,6 +2490,8 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s,
>>>> uint32_t order)
>>>>           r2 = s->Q(2);
>>>>           r3 = s->Q(3);
>>>>           break;
>>>> +    default:
>>>> +        qemu_build_not_reached();
>>>>       }
>>> I guess this won't fix the fact r0, r1, r2, r3 are not initialized, will it?
>> This ultimately expands to assert() and the compiler should see that it
>> terminates the control flow at this point, so shouldn't have a reason
>> to warn.
> 
> OK so with qemu_build_not_reached(); I get
> 
> /home/augere/UPSTREAM/qemu/include/qemu/osdep.h:184:35: error: call to
> ‘qemu_build_not_reached_always’ declared with attribute error: code path
> is reachable
>    184 | #define qemu_build_not_reached()  qemu_build_not_reached_always()
>        |                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> 
> However with g_assert_not_reached(), it does not complain and errors are
> removed. So I will respin with g_assert_not_reached() if nobody advises
> me against that.

Thank you!
Stefan Weil Dec. 22, 2022, 12:32 p.m. UTC | #10
Am 22.12.22 um 12:51 schrieb Philippe Mathieu-Daudé:

> On 22/12/22 12:18, Eric Auger wrote:
>> Hi All,
>>
>> On 12/22/22 12:09, Daniel P. Berrangé wrote:
>>> On Thu, Dec 22, 2022 at 11:07:31AM +0100, Eric Auger wrote:
>>>> Hi Philippe,
>>>>
>>>> On 12/22/22 10:01, Philippe Mathieu-Daudé wrote:
>>>>> On 22/12/22 09:18, Paolo Bonzini wrote:
>>>>>> On 12/21/22 17:36, Eric Auger wrote:
>>>>>>> To avoid compilation errors when -Werror=maybe-uninitialized is 
>>>>>>> used,
>>>>>>> replace 'case 3' by 'default'.
>>>>>>>
>>>>>>> Otherwise we get:
>>>>>>>
>>>>>>> ../target/i386/ops_sse.h: In function ‘helper_vpermdq_ymm’:
>>>>>>> ../target/i386/ops_sse.h:2495:13: error: ‘r3’ may be used
>>>>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>>>>      2495 |     d->Q(3) = r3;
>>>>>>>           |     ~~~~~~~~^~~~
>>>>>>> ../target/i386/ops_sse.h:2494:13: error: ‘r2’ may be used
>>>>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>>>>      2494 |     d->Q(2) = r2;
>>>>>>>           |     ~~~~~~~~^~~~
>>>>>>> ../target/i386/ops_sse.h:2493:13: error: ‘r1’ may be used
>>>>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>>>>      2493 |     d->Q(1) = r1;
>>>>>>>           |     ~~~~~~~~^~~~
>>>>>>> ../target/i386/ops_sse.h:2492:13: error: ‘r0’ may be used
>>>>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>>>>      2492 |     d->Q(0) = r0;
>>>>>>>           |     ~~~~~~~~^~~~
>>>>> With what compiler? Is that a supported one?
>>>> https://lore.kernel.org/qemu-devel/3aab489e-9d90-c1ad-0b6b-b2b5d80db723@redhat.com/ 
>>>>
>>>>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>>>>> Suggested-by: Stefan Weil <sw@weilnetz.de>
>>>>>>> Fixes: 790684776861 ("target/i386: reimplement 0x0f 0x3a, add AVX")
>>>>>>> ---
>>>>>>>    target/i386/ops_sse.h | 4 ++--
>>>>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/target/i386/ops_sse.h b/target/i386/ops_sse.h
>>>>>>> index 3cbc36a59d..c442c8c10c 100644
>>>>>>> --- a/target/i386/ops_sse.h
>>>>>>> +++ b/target/i386/ops_sse.h
>>>>>>> @@ -2466,7 +2466,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg
>>>>>>> *s, uint32_t order)
>>>>>>>            r0 = s->Q(0);
>>>>>>>            r1 = s->Q(1);
>>>>>>>            break;
>>>>>>> -    case 3:
>>>>>>> +    default:
>>>>>>>            r0 = s->Q(2);
>>>>>>>            r1 = s->Q(3);
>>>>>>>            break;
>>>>>>> @@ -2484,7 +2484,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg
>>>>>>> *s, uint32_t order)
>>>>>>>            r2 = s->Q(0);
>>>>>>>            r3 = s->Q(1);
>>>>>>>            break;
>>>>>>> -    case 3:
>>>>>>> +    default:
>>>>>>>            r2 = s->Q(2);
>>>>>>>            r3 = s->Q(3);
>>>>>>>            break;
>>>>>> Queued, but this compiler sucks. :)
>>>>> Can't we simply add a dumb 'default' case? So when reviewing we don't
>>>>> have to evaluate 'default' means 3 here.
>>>>>
>>>>> -- >8 --
>>>>> --- a/target/i386/ops_sse.h
>>>>> +++ b/target/i386/ops_sse.h
>>>>> @@ -2470,6 +2470,8 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s,
>>>>> uint32_t order)
>>>>>           r0 = s->Q(2);
>>>>>           r1 = s->Q(3);
>>>>>           break;
>>>>> +    default:
>>>>> +        qemu_build_not_reached();
>>>>>       }
>>>>>       switch ((order >> 4) & 3) {
>>>>>       case 0:
>>>>> @@ -2488,6 +2490,8 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s,
>>>>> uint32_t order)
>>>>>           r2 = s->Q(2);
>>>>>           r3 = s->Q(3);
>>>>>           break;
>>>>> +    default:
>>>>> +        qemu_build_not_reached();
>>>>>       }
>>>> I guess this won't fix the fact r0, r1, r2, r3 are not initialized, 
>>>> will it?
>>> This ultimately expands to assert() and the compiler should see that it
>>> terminates the control flow at this point, so shouldn't have a reason
>>> to warn.
>>
>> OK so with qemu_build_not_reached(); I get
>>
>> /home/augere/UPSTREAM/qemu/include/qemu/osdep.h:184:35: error: call to
>> ‘qemu_build_not_reached_always’ declared with attribute error: code path
>> is reachable
>>    184 | #define qemu_build_not_reached() 
>> qemu_build_not_reached_always()
>>        | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>>
>> However with g_assert_not_reached(), it does not complain and errors are
>> removed. So I will respin with g_assert_not_reached() if nobody advises
>> me against that.
>
> Thank you!


As noted by Paolo a better compiler could know that 0, 1, 2 and 3 are 
the only possible cases. Such a better compiler might complain that an 
additional default case is never reached. Therefore the proposed code 
might cause future compiler warnings.

But we could use this code pattern to make the intention of the code 
clearer:

case 3:
default: /* default case added to help the compiler to avoid warnings */
     ...

Stefan
Philippe Mathieu-Daudé Dec. 22, 2022, 12:44 p.m. UTC | #11
On 22/12/22 13:32, Stefan Weil wrote:
> Am 22.12.22 um 12:51 schrieb Philippe Mathieu-Daudé:
> 
>> On 22/12/22 12:18, Eric Auger wrote:
>>> Hi All,
>>>
>>> On 12/22/22 12:09, Daniel P. Berrangé wrote:
>>>> On Thu, Dec 22, 2022 at 11:07:31AM +0100, Eric Auger wrote:
>>>>> Hi Philippe,
>>>>>
>>>>> On 12/22/22 10:01, Philippe Mathieu-Daudé wrote:
>>>>>> On 22/12/22 09:18, Paolo Bonzini wrote:
>>>>>>> On 12/21/22 17:36, Eric Auger wrote:
>>>>>>>> To avoid compilation errors when -Werror=maybe-uninitialized is 
>>>>>>>> used,
>>>>>>>> replace 'case 3' by 'default'.
>>>>>>>>
>>>>>>>> Otherwise we get:
>>>>>>>>
>>>>>>>> ../target/i386/ops_sse.h: In function ‘helper_vpermdq_ymm’:
>>>>>>>> ../target/i386/ops_sse.h:2495:13: error: ‘r3’ may be used
>>>>>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>>>>>      2495 |     d->Q(3) = r3;
>>>>>>>>           |     ~~~~~~~~^~~~
>>>>>>>> ../target/i386/ops_sse.h:2494:13: error: ‘r2’ may be used
>>>>>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>>>>>      2494 |     d->Q(2) = r2;
>>>>>>>>           |     ~~~~~~~~^~~~
>>>>>>>> ../target/i386/ops_sse.h:2493:13: error: ‘r1’ may be used
>>>>>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>>>>>      2493 |     d->Q(1) = r1;
>>>>>>>>           |     ~~~~~~~~^~~~
>>>>>>>> ../target/i386/ops_sse.h:2492:13: error: ‘r0’ may be used
>>>>>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>>>>>>      2492 |     d->Q(0) = r0;
>>>>>>>>           |     ~~~~~~~~^~~~
>>>>>> With what compiler? Is that a supported one?
>>>>> https://lore.kernel.org/qemu-devel/3aab489e-9d90-c1ad-0b6b-b2b5d80db723@redhat.com/
>>>>>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>>>>>> Suggested-by: Stefan Weil <sw@weilnetz.de>
>>>>>>>> Fixes: 790684776861 ("target/i386: reimplement 0x0f 0x3a, add AVX")
>>>>>>>> ---
>>>>>>>>    target/i386/ops_sse.h | 4 ++--
>>>>>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/target/i386/ops_sse.h b/target/i386/ops_sse.h
>>>>>>>> index 3cbc36a59d..c442c8c10c 100644
>>>>>>>> --- a/target/i386/ops_sse.h
>>>>>>>> +++ b/target/i386/ops_sse.h
>>>>>>>> @@ -2466,7 +2466,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg
>>>>>>>> *s, uint32_t order)
>>>>>>>>            r0 = s->Q(0);
>>>>>>>>            r1 = s->Q(1);
>>>>>>>>            break;
>>>>>>>> -    case 3:
>>>>>>>> +    default:
>>>>>>>>            r0 = s->Q(2);
>>>>>>>>            r1 = s->Q(3);
>>>>>>>>            break;
>>>>>>>> @@ -2484,7 +2484,7 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg
>>>>>>>> *s, uint32_t order)
>>>>>>>>            r2 = s->Q(0);
>>>>>>>>            r3 = s->Q(1);
>>>>>>>>            break;
>>>>>>>> -    case 3:
>>>>>>>> +    default:
>>>>>>>>            r2 = s->Q(2);
>>>>>>>>            r3 = s->Q(3);
>>>>>>>>            break;
>>>>>>> Queued, but this compiler sucks. :)
>>>>>> Can't we simply add a dumb 'default' case? So when reviewing we don't
>>>>>> have to evaluate 'default' means 3 here.
>>>>>>
>>>>>> -- >8 --
>>>>>> --- a/target/i386/ops_sse.h
>>>>>> +++ b/target/i386/ops_sse.h
>>>>>> @@ -2470,6 +2470,8 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s,
>>>>>> uint32_t order)
>>>>>>           r0 = s->Q(2);
>>>>>>           r1 = s->Q(3);
>>>>>>           break;
>>>>>> +    default:
>>>>>> +        qemu_build_not_reached();
>>>>>>       }
>>>>>>       switch ((order >> 4) & 3) {
>>>>>>       case 0:
>>>>>> @@ -2488,6 +2490,8 @@ void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s,
>>>>>> uint32_t order)
>>>>>>           r2 = s->Q(2);
>>>>>>           r3 = s->Q(3);
>>>>>>           break;
>>>>>> +    default:
>>>>>> +        qemu_build_not_reached();
>>>>>>       }
>>>>> I guess this won't fix the fact r0, r1, r2, r3 are not initialized, 
>>>>> will it?
>>>> This ultimately expands to assert() and the compiler should see that it
>>>> terminates the control flow at this point, so shouldn't have a reason
>>>> to warn.
>>>
>>> OK so with qemu_build_not_reached(); I get
>>>
>>> /home/augere/UPSTREAM/qemu/include/qemu/osdep.h:184:35: error: call to
>>> ‘qemu_build_not_reached_always’ declared with attribute error: code path
>>> is reachable
>>>    184 | #define qemu_build_not_reached() 
>>> qemu_build_not_reached_always()
>>>        | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>>
>>> However with g_assert_not_reached(), it does not complain and errors are
>>> removed. So I will respin with g_assert_not_reached() if nobody advises
>>> me against that.
>>
>> Thank you!
> 
> 
> As noted by Paolo a better compiler could know that 0, 1, 2 and 3 are 
> the only possible cases. Such a better compiler might complain that an 
> additional default case is never reached. Therefore the proposed code 
> might cause future compiler warnings.
> 
> But we could use this code pattern to make the intention of the code 
> clearer:
> 
> case 3:
> default: /* default case added to help the compiler to avoid warnings */
>      ...

I'm fine with that, as long as we don't obfuscate the code for clever
compiler's sake. QEMU code base is already complex enough (the devil 
diff mbox series

Patch

diff --git a/target/i386/ops_sse.h b/target/i386/ops_sse.h
index 3cbc36a59d..c442c8c10c 100644
--- a/target/i386/ops_sse.h
+++ b/target/i386/ops_sse.h
@@ -2466,7 +2466,7 @@  void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s, uint32_t order)
         r0 = s->Q(0);
         r1 = s->Q(1);
         break;
-    case 3:
+    default:
         r0 = s->Q(2);
         r1 = s->Q(3);
         break;
@@ -2484,7 +2484,7 @@  void helper_vpermdq_ymm(Reg *d, Reg *v, Reg *s, uint32_t order)
         r2 = s->Q(0);
         r3 = s->Q(1);
         break;
-    case 3:
+    default:
         r2 = s->Q(2);
         r3 = s->Q(3);
         break;