[Qemu-devel] [PATCH 0/3] spapr: Fix remaining compiler warnings

[Qemu-devel] [PATCH 0/3] spapr: Fix remaining compiler warnings

[Stefan Weil]

Hi Alex,

this mini series fixes the remaining compiler warnings in
my w32/w64 cross build environment on Debian wheezy.

[PATCH 1/3] spapr: Use named enum for function remove_hpte
[PATCH 2/3] spapr: Fix compiler warning for some versions of gcc
[PATCH 3/3] spapr: Fix compiler warning for some versions of gcc

The modifications are nearly trivial, but this time it would
be better if you could apply them to your patch queue.

Regards
Stefan

[Qemu-devel] [PATCH 1/3] spapr: Use named enum for function remove_hpte

[Stefan Weil]

The function returned a target_ulong which was made from unnamed enum
values. The target_ulong was then assigned to an int variable which
was used in a switch statement.

Using a named enum in both cases makes reviews easier.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 hw/ppc/spapr_hcall.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 8f0b7e8..00f21f5 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -121,14 +121,14 @@ static target_ulong h_enter(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     return H_SUCCESS;
 }
 
-enum {
+typedef enum {
     REMOVE_SUCCESS = 0,
     REMOVE_NOT_FOUND = 1,
     REMOVE_PARM = 2,
     REMOVE_HW = 3,
-};
+} RemoveResult;
 
-static target_ulong remove_hpte(CPUPPCState *env, target_ulong ptex,
+static RemoveResult remove_hpte(CPUPPCState *env, target_ulong ptex,
                                 target_ulong avpn,
                                 target_ulong flags,
                                 target_ulong *vp, target_ulong *rp)
@@ -165,7 +165,7 @@ static target_ulong h_remove(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     target_ulong flags = args[0];
     target_ulong pte_index = args[1];
     target_ulong avpn = args[2];
-    int ret;
+    RemoveResult ret;
 
     ret = remove_hpte(env, pte_index, avpn, flags,
                       &args[0], &args[1]);
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 1/3] spapr: Use named enum for function remove_hpte

[Alexander Graf]

On 06/24/2013 07:48 PM, Stefan Weil wrote:
> The function returned a target_ulong which was made from unnamed enum
> values. The target_ulong was then assigned to an int variable which
> was used in a switch statement.
>
> Using a named enum in both cases makes reviews easier.
>
> Signed-off-by: Stefan Weil<sw@weilnetz.de>

Thanks, applied to ppc-next.


Alex

[Qemu-devel] [PATCH 2/3] spapr: Fix compiler warning for some versions of gcc (h_remove)

[Stefan Weil]

i686-w64-mingw32-gcc (GCC) 4.6.3 from Debian wheezy reports this warning:

hw/ppc/spapr_hcall.c:188:1: warning:
 control reaches end of non-void function [-Wreturn-type]

Replacing the 4th case REMOVE_HW (which is currently unused) by the default
case fixes this warning.

The assertion is dead code because all possible cases are handled in the
switch statements, so remove it. This avoids future warnings from static
code analyzers.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 hw/ppc/spapr_hcall.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 00f21f5..d49ce53 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -180,11 +180,9 @@ static target_ulong h_remove(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     case REMOVE_PARM:
         return H_PARAMETER;
 
-    case REMOVE_HW:
+    default: /* REMOVE_HW */
         return H_HARDWARE;
     }
-
-    assert(0);
 }
 
 #define H_BULK_REMOVE_TYPE             0xc000000000000000ULL
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 2/3] spapr: Fix compiler warning for some versions of gcc (h_remove)

[Paolo Bonzini]

Il 24/06/2013 19:48, Stefan Weil ha scritto:
> i686-w64-mingw32-gcc (GCC) 4.6.3 from Debian wheezy reports this warning:
> 
> hw/ppc/spapr_hcall.c:188:1: warning:
>  control reaches end of non-void function [-Wreturn-type]
> 
> Replacing the 4th case REMOVE_HW (which is currently unused) by the default
> case fixes this warning.
> 
> The assertion is dead code because all possible cases are handled in the
> switch statements, so remove it. This avoids future warnings from static
> code analyzers.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  hw/ppc/spapr_hcall.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 00f21f5..d49ce53 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -180,11 +180,9 @@ static target_ulong h_remove(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>      case REMOVE_PARM:
>          return H_PARAMETER;
>  
> -    case REMOVE_HW:
> +    default: /* REMOVE_HW */
>          return H_HARDWARE;
>      }
> -
> -    assert(0);
>  }
>  
>  #define H_BULK_REMOVE_TYPE             0xc000000000000000ULL
> 

Does s/assert(0)/abort()/ fix the warning too?  That's clearer.

Paolo

Re: [Qemu-devel] [PATCH 2/3] spapr: Fix compiler warning for some versions of gcc (h_remove)

[Stefan Weil]

Am 24.06.2013 22:27, schrieb Paolo Bonzini:
> Il 24/06/2013 19:48, Stefan Weil ha scritto:
>> i686-w64-mingw32-gcc (GCC) 4.6.3 from Debian wheezy reports this warning:
>>
>> hw/ppc/spapr_hcall.c:188:1: warning:
>>  control reaches end of non-void function [-Wreturn-type]
>>
>> Replacing the 4th case REMOVE_HW (which is currently unused) by the default
>> case fixes this warning.
>>
>> The assertion is dead code because all possible cases are handled in the
>> switch statements, so remove it. This avoids future warnings from static
>> code analyzers.
>>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>>  hw/ppc/spapr_hcall.c |    4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 00f21f5..d49ce53 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -180,11 +180,9 @@ static target_ulong h_remove(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>      case REMOVE_PARM:
>>          return H_PARAMETER;
>>  
>> -    case REMOVE_HW:
>> +    default: /* REMOVE_HW */
>>          return H_HARDWARE;
>>      }
>> -
>> -    assert(0);
>>  }
>>  
>>  #define H_BULK_REMOVE_TYPE             0xc000000000000000ULL
>>
> Does s/assert(0)/abort()/ fix the warning too?  That's clearer.
>
> Paolo

Yes, that also fixes this warning. A better compiler or a static
code checker might then complain about dead code because all
possible cases are handled in the switch statement.

Stefan

Re: [Qemu-devel] [PATCH 2/3] spapr: Fix compiler warning for some versions of gcc (h_remove)

[Paolo Bonzini]

Il 24/06/2013 22:35, Stefan Weil ha scritto:
> Am 24.06.2013 22:27, schrieb Paolo Bonzini:
>> Il 24/06/2013 19:48, Stefan Weil ha scritto:
>>> i686-w64-mingw32-gcc (GCC) 4.6.3 from Debian wheezy reports this warning:
>>>
>>> hw/ppc/spapr_hcall.c:188:1: warning:
>>>  control reaches end of non-void function [-Wreturn-type]
>>>
>>> Replacing the 4th case REMOVE_HW (which is currently unused) by the default
>>> case fixes this warning.
>>>
>>> The assertion is dead code because all possible cases are handled in the
>>> switch statements, so remove it. This avoids future warnings from static
>>> code analyzers.
>>>
>>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>>> ---
>>>  hw/ppc/spapr_hcall.c |    4 +---
>>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>>> index 00f21f5..d49ce53 100644
>>> --- a/hw/ppc/spapr_hcall.c
>>> +++ b/hw/ppc/spapr_hcall.c
>>> @@ -180,11 +180,9 @@ static target_ulong h_remove(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>>      case REMOVE_PARM:
>>>          return H_PARAMETER;
>>>  
>>> -    case REMOVE_HW:
>>> +    default: /* REMOVE_HW */
>>>          return H_HARDWARE;
>>>      }
>>> -
>>> -    assert(0);
>>>  }
>>>  
>>>  #define H_BULK_REMOVE_TYPE             0xc000000000000000ULL
>>>
>> Does s/assert(0)/abort()/ fix the warning too?  That's clearer.
> 
> Yes, that also fixes this warning. A better compiler or a static
> code checker might then complain about dead code because all
> possible cases are handled in the switch statement.

We'll get there when we see it... and such a compiler can well learn to
ignore a dead abort().

Paolo

[Qemu-devel] [PATCH 3/3] spapr: Fix compiler warning for some versions of gcc (spapr_io_read)

[Stefan Weil]

i686-w64-mingw32-gcc (GCC) 4.6.3 from Debian wheezy reports this warning:

hw/ppc/spapr_pci.c:454:1: warning:
 control reaches end of non-void function [-Wreturn-type]

Adding a default case to the switch statement satisfies the compiler.
This modification requires moving the assert statement.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 hw/ppc/spapr_pci.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 04e8362..c04086c 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -448,9 +448,10 @@ static uint64_t spapr_io_read(void *opaque, hwaddr addr,
     case 2:
         return cpu_inw(addr);
     case 4:
+    default:
+        assert(size == 4);
         return cpu_inl(addr);
     }
-    assert(0);
 }
 
 static void spapr_io_write(void *opaque, hwaddr addr,
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 3/3] spapr: Fix compiler warning for some versions of gcc (spapr_io_read)

[Alexander Graf]

On 06/24/2013 07:48 PM, Stefan Weil wrote:
> i686-w64-mingw32-gcc (GCC) 4.6.3 from Debian wheezy reports this warning:
>
> hw/ppc/spapr_pci.c:454:1: warning:
>   control reaches end of non-void function [-Wreturn-type]
>
> Adding a default case to the switch statement satisfies the compiler.
> This modification requires moving the assert statement.
>
> Signed-off-by: Stefan Weil<sw@weilnetz.de>
> ---
>   hw/ppc/spapr_pci.c |    3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 04e8362..c04086c 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -448,9 +448,10 @@ static uint64_t spapr_io_read(void *opaque, hwaddr addr,
>       case 2:
>           return cpu_inw(addr);
>       case 4:
> +    default:
> +        assert(size == 4);
>           return cpu_inl(addr);

Please fix this one up with g_assert_not_reached() as well.


Alex

Re: [Qemu-devel] [PATCH 3/3] spapr: Fix compiler warning for some versions of gcc (spapr_io_read)

[Stefan Weil]

Am 02.07.2013 14:45, schrieb Alexander Graf:
> On 06/24/2013 07:48 PM, Stefan Weil wrote:
>> i686-w64-mingw32-gcc (GCC) 4.6.3 from Debian wheezy reports this
>> warning:
>>
>> hw/ppc/spapr_pci.c:454:1: warning:
>>   control reaches end of non-void function [-Wreturn-type]
>>
>> Adding a default case to the switch statement satisfies the compiler.
>> This modification requires moving the assert statement.
>>
>> Signed-off-by: Stefan Weil<sw@weilnetz.de>
>> ---
>>   hw/ppc/spapr_pci.c |    3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index 04e8362..c04086c 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -448,9 +448,10 @@ static uint64_t spapr_io_read(void *opaque,
>> hwaddr addr,
>>       case 2:
>>           return cpu_inw(addr);
>>       case 4:
>> +    default:
>> +        assert(size == 4);
>>           return cpu_inl(addr);
>
> Please fix this one up with g_assert_not_reached() as well.
>
> Alex

I did, and you applied it to ppc-next. :-)

See http://patchwork.ozlabs.org/patch/255738/.

Cheers
Stefan

Re: [Qemu-devel] [PATCH 3/3] spapr: Fix compiler warning for some versions of gcc (spapr_io_read)

[Alexander Graf]

Am 02.07.2013 um 22:22 schrieb Stefan Weil <sw@weilnetz.de>:

> Am 02.07.2013 14:45, schrieb Alexander Graf:
>> On 06/24/2013 07:48 PM, Stefan Weil wrote:
>>> i686-w64-mingw32-gcc (GCC) 4.6.3 from Debian wheezy reports this
>>> warning:
>>> 
>>> hw/ppc/spapr_pci.c:454:1: warning:
>>>  control reaches end of non-void function [-Wreturn-type]
>>> 
>>> Adding a default case to the switch statement satisfies the compiler.
>>> This modification requires moving the assert statement.
>>> 
>>> Signed-off-by: Stefan Weil<sw@weilnetz.de>
>>> ---
>>>  hw/ppc/spapr_pci.c |    3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>>> index 04e8362..c04086c 100644
>>> --- a/hw/ppc/spapr_pci.c
>>> +++ b/hw/ppc/spapr_pci.c
>>> @@ -448,9 +448,10 @@ static uint64_t spapr_io_read(void *opaque,
>>> hwaddr addr,
>>>      case 2:
>>>          return cpu_inw(addr);
>>>      case 4:
>>> +    default:
>>> +        assert(size == 4);
>>>          return cpu_inl(addr);
>> 
>> Please fix this one up with g_assert_not_reached() as well.
>> 
>> Alex
> 
> I did, and you applied it to ppc-next. :-)
> 
> See http://patchwork.ozlabs.org/patch/255738/.

Oops, just consider me stupid then :)


Alex

> 
> Cheers
> Stefan
> 

Re: [Qemu-devel] [PATCH 0/3] spapr: Fix remaining compiler warnings

[Paolo Bonzini]

Il 24/06/2013 19:48, Stefan Weil ha scritto:
> Hi Alex,
> 
> this mini series fixes the remaining compiler warnings in
> my w32/w64 cross build environment on Debian wheezy.
> 
> [PATCH 1/3] spapr: Use named enum for function remove_hpte
> [PATCH 2/3] spapr: Fix compiler warning for some versions of gcc
> [PATCH 3/3] spapr: Fix compiler warning for some versions of gcc
> 
> The modifications are nearly trivial, but this time it would
> be better if you could apply them to your patch queue.

I think these patches are a bit backwards.

The right fix is just to use abort() instead of assert(0).

Paolo