[v1] Fix more GCC9 -O3 warnings

[PATCH 3/6] hw/net/imx_fec: Rewrite fall through comments

Posted by Philippe Mathieu-Daudé 6 years, 1 month ago

GCC9 is confused by this comment when building with CFLAG
-Wimplicit-fallthrough=2:

  hw/net/imx_fec.c: In function ‘imx_eth_write’:
  hw/net/imx_fec.c:906:12: error: this statement may fall through [-Werror=implicit-fallthrough=]
    906 |         if (unlikely(single_tx_ring)) {
        |            ^
  hw/net/imx_fec.c:912:5: note: here
    912 |     case ENET_TDAR:     /* FALLTHROUGH */
        |     ^~~~
  cc1: all warnings being treated as errors

Rewrite the comments in the correct place,  using 'fall through'
which is recognized by GCC and static analyzers.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Cc: Peter Chubb <peter.chubb@nicta.com.au>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Jason Wang <jasowang@redhat.com>
Cc: qemu-arm@nongnu.org
---
 hw/net/imx_fec.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
index bd99236864..30cc07753d 100644
--- a/hw/net/imx_fec.c
+++ b/hw/net/imx_fec.c
@@ -901,15 +901,17 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
             s->regs[index] = 0;
         }
         break;
-    case ENET_TDAR1:    /* FALLTHROUGH */
-    case ENET_TDAR2:    /* FALLTHROUGH */
+        /* fall through */
+    case ENET_TDAR1:
+    case ENET_TDAR2:
         if (unlikely(single_tx_ring)) {
             qemu_log_mask(LOG_GUEST_ERROR,
                           "[%s]%s: trying to access TDAR2 or TDAR1\n",
                           TYPE_IMX_FEC, __func__);
             return;
         }
-    case ENET_TDAR:     /* FALLTHROUGH */
+        /* fall through */
+    case ENET_TDAR:
         if (s->regs[ENET_ECR] & ENET_ECR_ETHEREN) {
             s->regs[index] = ENET_TDAR_TDAR;
             imx_eth_do_tx(s, index);
-- 
2.21.0

Re: [PATCH 3/6] hw/net/imx_fec: Rewrite fall through comments

Posted by Thomas Huth 6 years, 1 month ago

On 17/12/2019 18.34, Philippe Mathieu-Daudé wrote:
> GCC9 is confused by this comment when building with CFLAG
> -Wimplicit-fallthrough=2:
> 
>   hw/net/imx_fec.c: In function ‘imx_eth_write’:
>   hw/net/imx_fec.c:906:12: error: this statement may fall through [-Werror=implicit-fallthrough=]
>     906 |         if (unlikely(single_tx_ring)) {
>         |            ^
>   hw/net/imx_fec.c:912:5: note: here
>     912 |     case ENET_TDAR:     /* FALLTHROUGH */
>         |     ^~~~
>   cc1: all warnings being treated as errors
> 
> Rewrite the comments in the correct place,  using 'fall through'
> which is recognized by GCC and static analyzers.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Cc: Peter Chubb <peter.chubb@nicta.com.au>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: qemu-arm@nongnu.org
> ---
>  hw/net/imx_fec.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index bd99236864..30cc07753d 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -901,15 +901,17 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
>              s->regs[index] = 0;
>          }
>          break;
> -    case ENET_TDAR1:    /* FALLTHROUGH */
> -    case ENET_TDAR2:    /* FALLTHROUGH */
> +        /* fall through */

Wrong location. And I think you don't need any comment here at all, GCC
should stay silent without it?

> +    case ENET_TDAR1:
> +    case ENET_TDAR2:
>          if (unlikely(single_tx_ring)) {
>              qemu_log_mask(LOG_GUEST_ERROR,
>                            "[%s]%s: trying to access TDAR2 or TDAR1\n",
>                            TYPE_IMX_FEC, __func__);
>              return;
>          }
> -    case ENET_TDAR:     /* FALLTHROUGH */
> +        /* fall through */

I'd suggest to simply remove it, too.

> +    case ENET_TDAR:
>          if (s->regs[ENET_ECR] & ENET_ECR_ETHEREN) {
>              s->regs[index] = ENET_TDAR_TDAR;
>              imx_eth_do_tx(s, index);
> 

 Thomas

Re: [PATCH 3/6] hw/net/imx_fec: Rewrite fall through comments

Posted by Thomas Huth 6 years, 1 month ago

On 17/12/2019 18.55, Thomas Huth wrote:
> On 17/12/2019 18.34, Philippe Mathieu-Daudé wrote:
>> GCC9 is confused by this comment when building with CFLAG
>> -Wimplicit-fallthrough=2:
>>
>>   hw/net/imx_fec.c: In function ‘imx_eth_write’:
>>   hw/net/imx_fec.c:906:12: error: this statement may fall through [-Werror=implicit-fallthrough=]
>>     906 |         if (unlikely(single_tx_ring)) {
>>         |            ^
>>   hw/net/imx_fec.c:912:5: note: here
>>     912 |     case ENET_TDAR:     /* FALLTHROUGH */
>>         |     ^~~~
>>   cc1: all warnings being treated as errors
>>
>> Rewrite the comments in the correct place,  using 'fall through'
>> which is recognized by GCC and static analyzers.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> Cc: Peter Chubb <peter.chubb@nicta.com.au>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: qemu-arm@nongnu.org
>> ---
>>  hw/net/imx_fec.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
>> index bd99236864..30cc07753d 100644
>> --- a/hw/net/imx_fec.c
>> +++ b/hw/net/imx_fec.c
>> @@ -901,15 +901,17 @@ static void imx_eth_write(void *opaque, hwaddr offset, uint64_t value,
>>              s->regs[index] = 0;
>>          }
>>          break;
>> -    case ENET_TDAR1:    /* FALLTHROUGH */
>> -    case ENET_TDAR2:    /* FALLTHROUGH */
>> +        /* fall through */
> 
> Wrong location. And I think you don't need any comment here at all, GCC
> should stay silent without it?
> 
>> +    case ENET_TDAR1:
>> +    case ENET_TDAR2:
>>          if (unlikely(single_tx_ring)) {
>>              qemu_log_mask(LOG_GUEST_ERROR,
>>                            "[%s]%s: trying to access TDAR2 or TDAR1\n",
>>                            TYPE_IMX_FEC, __func__);
>>              return;
>>          }
>> -    case ENET_TDAR:     /* FALLTHROUGH */
>> +        /* fall through */
> 
> I'd suggest to simply remove it, too.

/me needsmorecoffee

... of course this hunk was fine. Good that you kept it in v2.

 Thomas


>> +    case ENET_TDAR:
>>          if (s->regs[ENET_ECR] & ENET_ECR_ETHEREN) {
>>              s->regs[index] = ENET_TDAR_TDAR;
>>              imx_eth_do_tx(s, index);
>>
> 
>  Thomas
>

Re: [PATCH 3/6] hw/net/imx_fec: Rewrite fall through comments

Posted by Aleksandar Markovic 6 years, 1 month ago

On Tuesday, December 17, 2019, Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> GCC9 is confused by this comment when building with CFLAG
> -Wimplicit-fallthrough=2:
>
>   hw/net/imx_fec.c: In function ‘imx_eth_write’:
>   hw/net/imx_fec.c:906:12: error: this statement may fall through
> [-Werror=implicit-fallthrough=]
>     906 |         if (unlikely(single_tx_ring)) {
>         |            ^
>   hw/net/imx_fec.c:912:5: note: here
>     912 |     case ENET_TDAR:     /* FALLTHROUGH */
>         |     ^~~~
>   cc1: all warnings being treated as errors
>
> Rewrite the comments in the correct place,  using 'fall through'
> which is recognized by GCC and static analyzers.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Cc: Peter Chubb <peter.chubb@nicta.com.au>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: qemu-arm@nongnu.org
> ---
>  hw/net/imx_fec.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
>
Here we can truly say that gcc is confused (as opposed to another patch
from this series).

The new positions of comments/annotations are good.

Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>



> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> index bd99236864..30cc07753d 100644
> --- a/hw/net/imx_fec.c
> +++ b/hw/net/imx_fec.c
> @@ -901,15 +901,17 @@ static void imx_eth_write(void *opaque, hwaddr
> offset, uint64_t value,
>              s->regs[index] = 0;
>          }
>          break;
> -    case ENET_TDAR1:    /* FALLTHROUGH */
> -    case ENET_TDAR2:    /* FALLTHROUGH */
> +        /* fall through */
> +    case ENET_TDAR1:
> +    case ENET_TDAR2:
>          if (unlikely(single_tx_ring)) {
>              qemu_log_mask(LOG_GUEST_ERROR,
>                            "[%s]%s: trying to access TDAR2 or TDAR1\n",
>                            TYPE_IMX_FEC, __func__);
>              return;
>          }
> -    case ENET_TDAR:     /* FALLTHROUGH */
> +        /* fall through */
> +    case ENET_TDAR:
>          if (s->regs[ENET_ECR] & ENET_ECR_ETHEREN) {
>              s->regs[index] = ENET_TDAR_TDAR;
>              imx_eth_do_tx(s, index);
> --
> 2.21.0
>
>
>

Re: [PATCH 3/6] hw/net/imx_fec: Rewrite fall through comments

Posted by Richard Henderson 6 years, 1 month ago

On 12/17/19 7:34 AM, Philippe Mathieu-Daudé wrote:
> GCC9 is confused by this comment when building with CFLAG
> -Wimplicit-fallthrough=2:
> 
>   hw/net/imx_fec.c: In function ‘imx_eth_write’:
>   hw/net/imx_fec.c:906:12: error: this statement may fall through [-Werror=implicit-fallthrough=]
>     906 |         if (unlikely(single_tx_ring)) {
>         |            ^
>   hw/net/imx_fec.c:912:5: note: here
>     912 |     case ENET_TDAR:     /* FALLTHROUGH */
>         |     ^~~~
>   cc1: all warnings being treated as errors
> 
> Rewrite the comments in the correct place,  using 'fall through'
> which is recognized by GCC and static analyzers.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Cc: Peter Chubb <peter.chubb@nicta.com.au>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: qemu-arm@nongnu.org
> ---
>  hw/net/imx_fec.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~