[PATCH 0/6] Fix more GCC9 -O3 warnings

[PATCH 0/6] Fix more GCC9 -O3 warnings

[Philippe Mathieu-Daudé]

Fix some trivial warnings when building with -O3.

Philippe Mathieu-Daudé (6):
  audio/audio: Add missing fall through comment
  hw/display/tcx: Add missing fall through comments
  hw/net/imx_fec: Rewrite fall through comments
  hw/timer/aspeed_timer: Add a fall through comment
  hw/scsi/megasas: Silent GCC9 duplicated-cond warning
  qemu-io-cmds: Silent GCC9 format-overflow warning

 audio/audio.c           | 1 +
 hw/display/tcx.c        | 2 ++
 hw/net/imx_fec.c        | 8 +++++---
 hw/scsi/megasas.c       | 3 ++-
 hw/timer/aspeed_timer.c | 2 +-
 qemu-io-cmds.c          | 1 +
 6 files changed, 12 insertions(+), 5 deletions(-)

-- 
2.21.0

[PATCH 1/6] audio/audio: Add missing fall through comment

[Philippe Mathieu-Daudé]

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

  audio/audio.c: In function ‘audio_pcm_init_info’:
  audio/audio.c:306:14: error: this statement may fall through [-Werror=implicit-fallthrough=]
    306 |         sign = 1;
        |         ~~~~~^~~
  audio/audio.c:307:5: note: here
    307 |     case AUDIO_FORMAT_U8:
        |     ^~~~
  cc1: all warnings being treated as errors

Add the missing fall through comment, similarly to e46349414.

Fixes: 2b9cce8c8c
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: "Kővágó, Zoltán" <dirty.ice.hu@gmail.com>
---
 audio/audio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/audio/audio.c b/audio/audio.c
index 56fae55047..57daf3f620 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -304,6 +304,7 @@ void audio_pcm_init_info (struct audio_pcm_info *info, struct audsettings *as)
     switch (as->fmt) {
     case AUDIO_FORMAT_S8:
         sign = 1;
+        /* fall through */
     case AUDIO_FORMAT_U8:
         mul = 1;
         break;
-- 
2.21.0

Re: [PATCH 1/6] audio/audio: Add missing fall through comment

[Richard Henderson]

On 12/17/19 7:34 AM, Philippe Mathieu-Daudé wrote:
> GCC9 is confused by this comment when building with
> CFLAG -Wimplicit-fallthrough=2:
> 
>   audio/audio.c: In function ‘audio_pcm_init_info’:
>   audio/audio.c:306:14: error: this statement may fall through [-Werror=implicit-fallthrough=]
>     306 |         sign = 1;
>         |         ~~~~~^~~
>   audio/audio.c:307:5: note: here
>     307 |     case AUDIO_FORMAT_U8:
>         |     ^~~~
>   cc1: all warnings being treated as errors
> 
> Add the missing fall through comment, similarly to e46349414.
...
> diff --git a/audio/audio.c b/audio/audio.c
> index 56fae55047..57daf3f620 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -304,6 +304,7 @@ void audio_pcm_init_info (struct audio_pcm_info *info, struct audsettings *as)
>      switch (as->fmt) {
>      case AUDIO_FORMAT_S8:
>          sign = 1;
> +        /* fall through */
>      case AUDIO_FORMAT_U8:
>          mul = 1;
>          break;

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

r~

Re: [PATCH 1/6] audio/audio: Add missing fall through comment

[Aleksandar Markovic]

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

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:
>
>
Gcc is not confused whatsoever.



>   audio/audio.c: In function ‘audio_pcm_init_info’:
>   audio/audio.c:306:14: error: this statement may fall through
> [-Werror=implicit-fallthrough=]
>     306 |         sign = 1;
>         |         ~~~~~^~~
>   audio/audio.c:307:5: note: here
>     307 |     case AUDIO_FORMAT_U8:
>         |     ^~~~
>   cc1: all warnings being treated as errors
>
> Add the missing fall through comment, similarly to e46349414.
>
> Fixes: 2b9cce8c8c
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: "Kővágó, Zoltán" <dirty.ice.hu@gmail.com>
> ---
>  audio/audio.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/audio/audio.c b/audio/audio.c
> index 56fae55047..57daf3f620 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -304,6 +304,7 @@ void audio_pcm_init_info (struct audio_pcm_info *info,
> struct audsettings *as)
>      switch (as->fmt) {
>      case AUDIO_FORMAT_S8:
>          sign = 1;
> +        /* fall through */
>      case AUDIO_FORMAT_U8:
>          mul = 1;
>          break;
> --
> 2.21.0
>
>
>

[-- Attachment #2: Type: text/html, Size: 1975 bytes --]

Re: [PATCH 1/6] audio/audio: Add missing fall through comment

[Philippe Mathieu-Daudé]

On 12/18/19 9:02 AM, Aleksandar Markovic wrote:
> 
> 
> On Tuesday, December 17, 2019, Philippe Mathieu-Daudé <philmd@redhat.com 
> <mailto:philmd@redhat.com>> wrote:
> 
>     GCC9 is confused by this comment when building with
>     CFLAG -Wimplicit-fallthrough=2:
> 
> 
> Gcc is not confused whatsoever.

It is right! I'll update the description :)

> 
>        audio/audio.c: In function ‘audio_pcm_init_info’:
>        audio/audio.c:306:14: error: this statement may fall through
>     [-Werror=implicit-fallthrough=]
>          306 |         sign = 1;
>              |         ~~~~~^~~
>        audio/audio.c:307:5: note: here
>          307 |     case AUDIO_FORMAT_U8:
>              |     ^~~~
>        cc1: all warnings being treated as errors
> 
>     Add the missing fall through comment, similarly to e46349414.
> 
>     Fixes: 2b9cce8c8c
>     Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com
>     <mailto:philmd@redhat.com>>
>     ---
>     Cc: Gerd Hoffmann <kraxel@redhat.com <mailto:kraxel@redhat.com>>
>     Cc: "Kővágó, Zoltán" <dirty.ice.hu@gmail.com
>     <mailto:dirty.ice.hu@gmail.com>>
>     ---
>       audio/audio.c | 1 +
>       1 file changed, 1 insertion(+)
> 
>     diff --git a/audio/audio.c b/audio/audio.c
>     index 56fae55047..57daf3f620 100644
>     --- a/audio/audio.c
>     +++ b/audio/audio.c
>     @@ -304,6 +304,7 @@ void audio_pcm_init_info (struct audio_pcm_info
>     *info, struct audsettings *as)
>           switch (as->fmt) {
>           case AUDIO_FORMAT_S8:
>               sign = 1;
>     +        /* fall through */
>           case AUDIO_FORMAT_U8:
>               mul = 1;
>               break;
>     -- 
>     2.21.0
> 
> 

[PATCH 2/6] hw/display/tcx: Add missing fall through comments

[Philippe Mathieu-Daudé]

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

  hw/display/tcx.c: In function ‘tcx_dac_writel’:
  hw/display/tcx.c:453:26: error: this statement may fall through [-Werror=implicit-fallthrough=]
    453 |             s->dac_index = (s->dac_index + 1) & 0xff; /* Index autoincrement */
        |             ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
  hw/display/tcx.c:454:9: note: here
    454 |         default:
        |         ^~~~~~~
  hw/display/tcx.c: In function ‘tcx_dac_readl’:
  hw/display/tcx.c:412:22: error: this statement may fall through [-Werror=implicit-fallthrough=]
    412 |         s->dac_index = (s->dac_index + 1) & 0xff; /* Index autoincrement */
        |         ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
  hw/display/tcx.c:413:5: note: here
    413 |     default:
        |     ^~~~~~~
  cc1: all warnings being treated as errors

Add the missing fall through comments.

Fixes: 55d7bfe22
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Cc: Olivier Danet <odanet@caramail.com>
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/tcx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 14e829d3fa..abbeb30284 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -410,6 +410,7 @@ static uint64_t tcx_dac_readl(void *opaque, hwaddr addr,
     case 2:
         val = s->b[s->dac_index] << 24;
         s->dac_index = (s->dac_index + 1) & 0xff; /* Index autoincrement */
+        /* fall through */
     default:
         s->dac_state = 0;
         break;
@@ -451,6 +452,7 @@ static void tcx_dac_writel(void *opaque, hwaddr addr, uint64_t val,
             s->b[index] = val >> 24;
             update_palette_entries(s, index, index + 1);
             s->dac_index = (s->dac_index + 1) & 0xff; /* Index autoincrement */
+            /* fall through */
         default:
             s->dac_state = 0;
             break;
-- 
2.21.0

Re: [PATCH 2/6] hw/display/tcx: Add missing fall through comments

[Richard Henderson]

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/display/tcx.c: In function ‘tcx_dac_writel’:
>   hw/display/tcx.c:453:26: error: this statement may fall through [-Werror=implicit-fallthrough=]
>     453 |             s->dac_index = (s->dac_index + 1) & 0xff; /* Index autoincrement */
>         |             ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
>   hw/display/tcx.c:454:9: note: here
>     454 |         default:
>         |         ^~~~~~~
>   hw/display/tcx.c: In function ‘tcx_dac_readl’:
>   hw/display/tcx.c:412:22: error: this statement may fall through [-Werror=implicit-fallthrough=]
>     412 |         s->dac_index = (s->dac_index + 1) & 0xff; /* Index autoincrement */
>         |         ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
>   hw/display/tcx.c:413:5: note: here
>     413 |     default:
>         |     ^~~~~~~
>   cc1: all warnings being treated as errors
> 
> Add the missing fall through comments.
> 
> Fixes: 55d7bfe22
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Cc: Olivier Danet <odanet@caramail.com>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/tcx.c | 2 ++
>  1 file changed, 2 insertions(+)

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

r~

Re: [PATCH 2/6] hw/display/tcx: Add missing fall through comments

[Aleksandar Markovic]

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

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/display/tcx.c: In function ‘tcx_dac_writel’:
>   hw/display/tcx.c:453:26: error: this statement may fall through
> [-Werror=implicit-fallthrough=]
>     453 |             s->dac_index = (s->dac_index + 1) & 0xff; /* Index
> autoincrement */
>         |             ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
>   hw/display/tcx.c:454:9: note: here
>     454 |         default:
>         |         ^~~~~~~
>   hw/display/tcx.c: In function ‘tcx_dac_readl’:
>   hw/display/tcx.c:412:22: error: this statement may fall through
> [-Werror=implicit-fallthrough=]
>     412 |         s->dac_index = (s->dac_index + 1) & 0xff; /* Index
> autoincrement */
>         |         ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
>   hw/display/tcx.c:413:5: note: here
>     413 |     default:
>         |     ^~~~~~~
>   cc1: all warnings being treated as errors
>
> Add the missing fall through comments.
>
> Fixes: 55d7bfe22
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Cc: Olivier Danet <odanet@caramail.com>
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/display/tcx.c | 2 ++
>  1 file changed, 2 insertions(+)
>
>
The content of the patch is fine, but the commit message is, I think,
inacurate: gcc is not confused at all, it does what it was told to.

The title is fine.


> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
> index 14e829d3fa..abbeb30284 100644
> --- a/hw/display/tcx.c
> +++ b/hw/display/tcx.c
> @@ -410,6 +410,7 @@ static uint64_t tcx_dac_readl(void *opaque, hwaddr
> addr,
>      case 2:
>          val = s->b[s->dac_index] << 24;
>          s->dac_index = (s->dac_index + 1) & 0xff; /* Index autoincrement
> */
> +        /* fall through */
>      default:
>          s->dac_state = 0;
>          break;
> @@ -451,6 +452,7 @@ static void tcx_dac_writel(void *opaque, hwaddr addr,
> uint64_t val,
>              s->b[index] = val >> 24;
>              update_palette_entries(s, index, index + 1);
>              s->dac_index = (s->dac_index + 1) & 0xff; /* Index
> autoincrement */
> +            /* fall through */
>          default:
>              s->dac_state = 0;
>              break;
> --
> 2.21.0
>
>
>

[-- Attachment #2: Type: text/html, Size: 3227 bytes --]

Re: [PATCH 2/6] hw/display/tcx: Add missing fall through comments

[Philippe Mathieu-Daudé]

On 12/18/19 8:54 AM, Aleksandar Markovic wrote:
> 
> 
> On Tuesday, December 17, 2019, Philippe Mathieu-Daudé <philmd@redhat.com 
> <mailto:philmd@redhat.com>> wrote:
> 
>     GCC9 is confused by this comment when building with
>     CFLAG -Wimplicit-fallthrough=2:
> 
>        hw/display/tcx.c: In function ‘tcx_dac_writel’:
>        hw/display/tcx.c:453:26: error: this statement may fall through
>     [-Werror=implicit-fallthrough=]
>          453 |             s->dac_index = (s->dac_index + 1) & 0xff; /*
>     Index autoincrement */
>              |             ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
>        hw/display/tcx.c:454:9: note: here
>          454 |         default:
>              |         ^~~~~~~
>        hw/display/tcx.c: In function ‘tcx_dac_readl’:
>        hw/display/tcx.c:412:22: error: this statement may fall through
>     [-Werror=implicit-fallthrough=]
>          412 |         s->dac_index = (s->dac_index + 1) & 0xff; /*
>     Index autoincrement */
>              |         ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
>        hw/display/tcx.c:413:5: note: here
>          413 |     default:
>              |     ^~~~~~~
>        cc1: all warnings being treated as errors
> 
>     Add the missing fall through comments.
> 
>     Fixes: 55d7bfe22
>     Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com
>     <mailto:philmd@redhat.com>>
>     ---
>     Cc: Olivier Danet <odanet@caramail.com <mailto:odanet@caramail.com>>
>     Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk
>     <mailto:mark.cave-ayland@ilande.co.uk>>
>     ---
>       hw/display/tcx.c | 2 ++
>       1 file changed, 2 insertions(+)
> 
> 
> The content of the patch is fine, but the commit message is, I think, 
> inacurate: gcc is not confused at all, it does what it was told to.

You are correct, I'll update the comment.

> The title is fine.
> 
>     diff --git a/hw/display/tcx.c b/hw/display/tcx.c
>     index 14e829d3fa..abbeb30284 100644
>     --- a/hw/display/tcx.c
>     +++ b/hw/display/tcx.c
>     @@ -410,6 +410,7 @@ static uint64_t tcx_dac_readl(void *opaque,
>     hwaddr addr,
>           case 2:
>               val = s->b[s->dac_index] << 24;
>               s->dac_index = (s->dac_index + 1) & 0xff; /* Index
>     autoincrement */
>     +        /* fall through */
>           default:
>               s->dac_state = 0;
>               break;
>     @@ -451,6 +452,7 @@ static void tcx_dac_writel(void *opaque, hwaddr
>     addr, uint64_t val,
>                   s->b[index] = val >> 24;
>                   update_palette_entries(s, index, index + 1);
>                   s->dac_index = (s->dac_index + 1) & 0xff; /* Index
>     autoincrement */
>     +            /* fall through */
>               default:
>                   s->dac_state = 0;
>                   break;
>     -- 
>     2.21.0
> 
> 

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

[Philippe Mathieu-Daudé]

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

[Thomas Huth]

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

[Thomas Huth]

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

[Richard Henderson]

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~

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

[Aleksandar Markovic]

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

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
>
>
>

[-- Attachment #2: Type: text/html, Size: 3220 bytes --]

[PATCH 4/6] hw/timer/aspeed_timer: Add a fall through comment

[Philippe Mathieu-Daudé]

Reported by GCC9 when building with CFLAG -Wimplicit-fallthrough=2:

  hw/timer/aspeed_timer.c: In function ‘aspeed_timer_set_value’:
  hw/timer/aspeed_timer.c:283:24: error: this statement may fall through [-Werror=implicit-fallthrough=]
    283 |         if (old_reload || !t->reload) {
        |             ~~~~~~~~~~~^~~~~~~~~~~~~
  hw/timer/aspeed_timer.c:287:5: note: here
    287 |     case TIMER_REG_STATUS:
        |     ^~~~
  cc1: all warnings being treated as errors

Add the missing fall through comment.

Fixes: 1403f364472
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Cc: "Cédric Le Goater" <clg@kaod.org>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Andrew Jeffery <andrew@aj.id.au>
Cc: Joel Stanley <joel@jms.id.au>
Cc: qemu-arm@nongnu.org
---
 hw/timer/aspeed_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index a8c38cc118..c91f18415c 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -283,7 +283,7 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
         if (old_reload || !t->reload) {
             break;
         }
-
+        /* fall through to re-enable */
     case TIMER_REG_STATUS:
         if (timer_enabled(t)) {
             uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-- 
2.21.0

Re: [PATCH 4/6] hw/timer/aspeed_timer: Add a fall through comment

[Cédric Le Goater]

On 17/12/2019 18:34, Philippe Mathieu-Daudé wrote:
> Reported by GCC9 when building with CFLAG -Wimplicit-fallthrough=2:
> 
>   hw/timer/aspeed_timer.c: In function ‘aspeed_timer_set_value’:
>   hw/timer/aspeed_timer.c:283:24: error: this statement may fall through [-Werror=implicit-fallthrough=]
>     283 |         if (old_reload || !t->reload) {
>         |             ~~~~~~~~~~~^~~~~~~~~~~~~
>   hw/timer/aspeed_timer.c:287:5: note: here
>     287 |     case TIMER_REG_STATUS:
>         |     ^~~~
>   cc1: all warnings being treated as errors
> 
> Add the missing fall through comment.
> 
> Fixes: 1403f364472
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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


> ---
> Cc: "Cédric Le Goater" <clg@kaod.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Andrew Jeffery <andrew@aj.id.au>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: qemu-arm@nongnu.org
> ---
>  hw/timer/aspeed_timer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> index a8c38cc118..c91f18415c 100644
> --- a/hw/timer/aspeed_timer.c
> +++ b/hw/timer/aspeed_timer.c
> @@ -283,7 +283,7 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
>          if (old_reload || !t->reload) {
>              break;
>          }
> -
> +        /* fall through to re-enable */
>      case TIMER_REG_STATUS:
>          if (timer_enabled(t)) {
>              uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> 

Re: [PATCH 4/6] hw/timer/aspeed_timer: Add a fall through comment

[Aleksandar Markovic]

On Tue, Dec 17, 2019 at 6:37 PM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Reported by GCC9 when building with CFLAG -Wimplicit-fallthrough=2:
>
>   hw/timer/aspeed_timer.c: In function ‘aspeed_timer_set_value’:
>   hw/timer/aspeed_timer.c:283:24: error: this statement may fall through [-Werror=implicit-fallthrough=]
>     283 |         if (old_reload || !t->reload) {
>         |             ~~~~~~~~~~~^~~~~~~~~~~~~
>   hw/timer/aspeed_timer.c:287:5: note: here
>     287 |     case TIMER_REG_STATUS:
>         |     ^~~~
>   cc1: all warnings being treated as errors
>
> Add the missing fall through comment.
>
> Fixes: 1403f364472
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---

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

> Cc: "Cédric Le Goater" <clg@kaod.org>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: Andrew Jeffery <andrew@aj.id.au>
> Cc: Joel Stanley <joel@jms.id.au>
> Cc: qemu-arm@nongnu.org
> ---
>  hw/timer/aspeed_timer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> index a8c38cc118..c91f18415c 100644
> --- a/hw/timer/aspeed_timer.c
> +++ b/hw/timer/aspeed_timer.c
> @@ -283,7 +283,7 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
>          if (old_reload || !t->reload) {
>              break;
>          }
> -
> +        /* fall through to re-enable */
>      case TIMER_REG_STATUS:
>          if (timer_enabled(t)) {
>              uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> --
> 2.21.0
>
>

[PATCH 5/6] hw/scsi/megasas: Silent GCC9 duplicated-cond warning

[Philippe Mathieu-Daudé]

GCC9 is confused when building with CFLAG -O3:

  hw/scsi/megasas.c: In function ‘megasas_scsi_realize’:
  hw/scsi/megasas.c:2387:26: error: duplicated ‘if’ condition [-Werror=duplicated-cond]
   2387 |     } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
  hw/scsi/megasas.c:2385:19: note: previously used here
   2385 |     if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
  cc1: all warnings being treated as errors

When this device was introduced in commit e8f943c3bcc, the author
cared about modularity, using a definition for the firmware limit.
If we modify the limit, the code is valid. Add a check if the
definition got modified to a bigger limit.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Cc: Hannes Reinecke <hare@suse.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Fam Zheng <fam@euphon.net>
Cc: qemu-block@nongnu.org
---
 hw/scsi/megasas.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index de9bd20887..ece1601b66 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -2382,7 +2382,8 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
     if (!s->hba_serial) {
         s->hba_serial = g_strdup(MEGASAS_HBA_SERIAL);
     }
-    if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
+    if (MEGASAS_MAX_SGE > 128
+        && s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
         s->fw_sge = MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE;
     } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
         s->fw_sge = 128 - MFI_PASS_FRAME_SIZE;
-- 
2.21.0

Re: [PATCH 5/6] hw/scsi/megasas: Silent GCC9 duplicated-cond warning

[Richard Henderson]

On 12/17/19 7:34 AM, Philippe Mathieu-Daudé wrote:
> GCC9 is confused when building with CFLAG -O3:
> 
>   hw/scsi/megasas.c: In function ‘megasas_scsi_realize’:
>   hw/scsi/megasas.c:2387:26: error: duplicated ‘if’ condition [-Werror=duplicated-cond]
>    2387 |     } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
>   hw/scsi/megasas.c:2385:19: note: previously used here
>    2385 |     if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>   cc1: all warnings being treated as errors
> 
> When this device was introduced in commit e8f943c3bcc, the author
> cared about modularity, using a definition for the firmware limit.
> If we modify the limit, the code is valid. Add a check if the
> definition got modified to a bigger limit.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Fam Zheng <fam@euphon.net>
> Cc: qemu-block@nongnu.org
> ---
>  hw/scsi/megasas.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> index de9bd20887..ece1601b66 100644
> --- a/hw/scsi/megasas.c
> +++ b/hw/scsi/megasas.c
> @@ -2382,7 +2382,8 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>      if (!s->hba_serial) {
>          s->hba_serial = g_strdup(MEGASAS_HBA_SERIAL);
>      }
> -    if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
> +    if (MEGASAS_MAX_SGE > 128
> +        && s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>          s->fw_sge = MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE;
>      } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
>          s->fw_sge = 128 - MFI_PASS_FRAME_SIZE;
> 

I'm not keen on this.  It looks to me like the raw 128 case should be removed
-- surely that's the point of the symbolic constant.  But I'll defer if a
maintainer disagrees.


r~

Re: [PATCH 5/6] hw/scsi/megasas: Silent GCC9 duplicated-cond warning

[Philippe Mathieu-Daudé]

On 12/18/19 5:03 AM, Richard Henderson wrote:
> On 12/17/19 7:34 AM, Philippe Mathieu-Daudé wrote:
>> GCC9 is confused when building with CFLAG -O3:
>>
>>    hw/scsi/megasas.c: In function ‘megasas_scsi_realize’:
>>    hw/scsi/megasas.c:2387:26: error: duplicated ‘if’ condition [-Werror=duplicated-cond]
>>     2387 |     } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
>>    hw/scsi/megasas.c:2385:19: note: previously used here
>>     2385 |     if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>>    cc1: all warnings being treated as errors
>>
>> When this device was introduced in commit e8f943c3bcc, the author
>> cared about modularity, using a definition for the firmware limit.
>> If we modify the limit, the code is valid. Add a check if the
>> definition got modified to a bigger limit.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> Cc: Hannes Reinecke <hare@suse.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Fam Zheng <fam@euphon.net>
>> Cc: qemu-block@nongnu.org
>> ---
>>   hw/scsi/megasas.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
>> index de9bd20887..ece1601b66 100644
>> --- a/hw/scsi/megasas.c
>> +++ b/hw/scsi/megasas.c
>> @@ -2382,7 +2382,8 @@ static void megasas_scsi_realize(PCIDevice *dev, Error **errp)
>>       if (!s->hba_serial) {
>>           s->hba_serial = g_strdup(MEGASAS_HBA_SERIAL);
>>       }
>> -    if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>> +    if (MEGASAS_MAX_SGE > 128
>> +        && s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>>           s->fw_sge = MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE;
>>       } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
>>           s->fw_sge = 128 - MFI_PASS_FRAME_SIZE;
>>
> 
> I'm not keen on this.  It looks to me like the raw 128 case should be removed
> -- surely that's the point of the symbolic constant.  But I'll defer if a
> maintainer disagrees.

Is that approach acceptable?

-- >8 --
@@ -54,6 +54,9 @@
  #define MEGASAS_FLAG_USE_QUEUE64   1
  #define MEGASAS_MASK_USE_QUEUE64   (1 << MEGASAS_FLAG_USE_QUEUE64)

+QEMU_BUILD_BUG_MSG(MEGASAS_MAX_SGE > 128,
+                   "Firmware limit too big for this device model");
+
  static const char *mfi_frame_desc[] = {
      "MFI init", "LD Read", "LD Write", "LD SCSI", "PD SCSI",
      "MFI Doorbell", "MFI Abort", "MFI SMP", "MFI Stop"};
@@ -2382,9 +2385,7 @@ static void megasas_scsi_realize(PCIDevice *dev, 
Error **errp)
      if (!s->hba_serial) {
          s->hba_serial = g_strdup(MEGASAS_HBA_SERIAL);
      }
-    if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
-        s->fw_sge = MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE;
-    } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
+    if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
          s->fw_sge = 128 - MFI_PASS_FRAME_SIZE;
      } else {
          s->fw_sge = 64 - MFI_PASS_FRAME_SIZE;
---

Re: [PATCH 5/6] hw/scsi/megasas: Silent GCC9 duplicated-cond warning

[Richard Henderson]

On 12/18/19 7:52 AM, Philippe Mathieu-Daudé wrote:
>>> -    if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>>> +    if (MEGASAS_MAX_SGE > 128
>>> +        && s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>>>           s->fw_sge = MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE;
>>>       } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
>>>           s->fw_sge = 128 - MFI_PASS_FRAME_SIZE;
>>>
>>
>> I'm not keen on this.  It looks to me like the raw 128 case should be removed
>> -- surely that's the point of the symbolic constant.  But I'll defer if a
>> maintainer disagrees.
> 
> Is that approach acceptable?
> 
> -- >8 --
> @@ -54,6 +54,9 @@
>  #define MEGASAS_FLAG_USE_QUEUE64   1
>  #define MEGASAS_MASK_USE_QUEUE64   (1 << MEGASAS_FLAG_USE_QUEUE64)
> 
> +QEMU_BUILD_BUG_MSG(MEGASAS_MAX_SGE > 128,
> +                   "Firmware limit too big for this device model");
> +
>  static const char *mfi_frame_desc[] = {
>      "MFI init", "LD Read", "LD Write", "LD SCSI", "PD SCSI",
>      "MFI Doorbell", "MFI Abort", "MFI SMP", "MFI Stop"};
> @@ -2382,9 +2385,7 @@ static void megasas_scsi_realize(PCIDevice *dev, Error
> **errp)
>      if (!s->hba_serial) {
>          s->hba_serial = g_strdup(MEGASAS_HBA_SERIAL);
>      }
> -    if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
> -        s->fw_sge = MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE;
> -    } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
> +    if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
>          s->fw_sge = 128 - MFI_PASS_FRAME_SIZE;
>      } else {
>          s->fw_sge = 64 - MFI_PASS_FRAME_SIZE;

Eh.  I suppose.  But what's the point of leaving the hard-coded 128?  There's
certainly something I'm missing here...


r~

Re: [PATCH 5/6] hw/scsi/megasas: Silent GCC9 duplicated-cond warning

[Paolo Bonzini]

On 18/12/19 05:03, Richard Henderson wrote:
>>      if (!s->hba_serial) {
>>          s->hba_serial = g_strdup(MEGASAS_HBA_SERIAL);
>>      }
>> -    if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>> +    if (MEGASAS_MAX_SGE > 128
>> +        && s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>>          s->fw_sge = MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE;
>>      } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
>>          s->fw_sge = 128 - MFI_PASS_FRAME_SIZE;
>>      } else {
>>          s->fw_sge = 64 - MFI_PASS_FRAME_SIZE;
>>      }
>
> I'm not keen on this.  It looks to me like the raw 128 case should be removed
> -- surely that's the point of the symbolic constant.  But I'll defer if a
> maintainer disagrees.

I don't really understand this chain of ifs.  Hannes, does it make sense
to just remove the "if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE)" case,
or does Phil's variation (quoted in the patch fragment above) makes sense?

Or perhaps this rewrite:

        max_sge = s->fw_sge + MFI_PASS_FRAME_SIZE;
        if (max_sge < MEGASAS_MAX_SGE) {
            if (max_sge < 64) {
                error(...);
            } else {
                max_sge = max_sge < 128 ? 64 : 128;
            }
        }
	s->fw_sge = max_sge - MFI_PASS_FRAME_SIZE;

Paolo

Re: [PATCH 5/6] hw/scsi/megasas: Silent GCC9 duplicated-cond warning

[Hannes Reinecke]

On 1/7/20 3:56 PM, Paolo Bonzini wrote:
> On 18/12/19 05:03, Richard Henderson wrote:
>>>      if (!s->hba_serial) {
>>>          s->hba_serial = g_strdup(MEGASAS_HBA_SERIAL);
>>>      }
>>> -    if (s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>>> +    if (MEGASAS_MAX_SGE > 128
>>> +        && s->fw_sge >= MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE) {
>>>          s->fw_sge = MEGASAS_MAX_SGE - MFI_PASS_FRAME_SIZE;
>>>      } else if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE) {
>>>          s->fw_sge = 128 - MFI_PASS_FRAME_SIZE;
>>>      } else {
>>>          s->fw_sge = 64 - MFI_PASS_FRAME_SIZE;
>>>      }
>>
>> I'm not keen on this.  It looks to me like the raw 128 case should be removed
>> -- surely that's the point of the symbolic constant.  But I'll defer if a
>> maintainer disagrees.
> 
> I don't really understand this chain of ifs.  Hannes, does it make sense
> to just remove the "if (s->fw_sge >= 128 - MFI_PASS_FRAME_SIZE)" case,
> or does Phil's variation (quoted in the patch fragment above) makes sense?
> 
> Or perhaps this rewrite:
> 
>         max_sge = s->fw_sge + MFI_PASS_FRAME_SIZE;
>         if (max_sge < MEGASAS_MAX_SGE) {
>             if (max_sge < 64) {
>                 error(...);
>             } else {
>                 max_sge = max_sge < 128 ? 64 : 128;
>             }
>         }
> 	s->fw_sge = max_sge - MFI_PASS_FRAME_SIZE;
> 
Yeah, do it.
The original code assumed that one could change MFI_PASS_FRAME_SIZE, but
it turned out not to be possible as it's being hardcoded in the drivers
themselves (even though the interface provides mechanisms to query it).
So we can remove the duplicate lines.
But then I prefer to stick with the define, and avoid having to check
the magic '128' value directly; rather use the define throughout the code.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer

[PATCH 6/6] qemu-io-cmds: Silent GCC9 format-overflow warning

[Philippe Mathieu-Daudé]

GCC9 is confused when building with CFLAG -O3:

  In function ‘help_oneline’,
      inlined from ‘help_all’ at qemu-io-cmds.c:2414:9,
      inlined from ‘help_f’ at qemu-io-cmds.c:2424:9:
  qemu-io-cmds.c:2389:9: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
   2389 |         printf("%s ", ct->name);
        |         ^~~~~~~~~~~~~~~~~~~~~~~

Audit shows this can't happen. Give a hint to GCC adding an
assert() call.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: qemu-block@nongnu.org
---
 qemu-io-cmds.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 1b7e700020..9e956a5dd4 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -2411,6 +2411,7 @@ static void help_all(void)
     const cmdinfo_t *ct;
 
     for (ct = cmdtab; ct < &cmdtab[ncmds]; ct++) {
+        assert(ct->name);
         help_oneline(ct->name, ct);
     }
     printf("\nUse 'help commandname' for extended help.\n");
-- 
2.21.0

Re: [PATCH 6/6] qemu-io-cmds: Silent GCC9 format-overflow warning

[Richard Henderson]

On 12/17/19 7:34 AM, Philippe Mathieu-Daudé wrote:
> GCC9 is confused when building with CFLAG -O3:
> 
>   In function ‘help_oneline’,
>       inlined from ‘help_all’ at qemu-io-cmds.c:2414:9,
>       inlined from ‘help_f’ at qemu-io-cmds.c:2424:9:
>   qemu-io-cmds.c:2389:9: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
>    2389 |         printf("%s ", ct->name);
>         |         ^~~~~~~~~~~~~~~~~~~~~~~
> 
> Audit shows this can't happen. Give a hint to GCC adding an
> assert() call.

This deserves more investigation.  From my glance it appears you are right --
and moreover impossible for gcc to have come to this conclusion.  Which begs
the question of how that is.

Did you file a gcc bug report?


r~

> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Max Reitz <mreitz@redhat.com>
> Cc: qemu-block@nongnu.org
> ---
>  qemu-io-cmds.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 1b7e700020..9e956a5dd4 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -2411,6 +2411,7 @@ static void help_all(void)
>      const cmdinfo_t *ct;
>  
>      for (ct = cmdtab; ct < &cmdtab[ncmds]; ct++) {
> +        assert(ct->name);
>          help_oneline(ct->name, ct);
>      }
>      printf("\nUse 'help commandname' for extended help.\n");
> 

Re: [PATCH 6/6] qemu-io-cmds: Silent GCC9 format-overflow warning

[Philippe Mathieu-Daudé]

On 12/18/19 5:15 AM, Richard Henderson wrote:
> On 12/17/19 7:34 AM, Philippe Mathieu-Daudé wrote:
>> GCC9 is confused when building with CFLAG -O3:
>>
>>    In function ‘help_oneline’,
>>        inlined from ‘help_all’ at qemu-io-cmds.c:2414:9,
>>        inlined from ‘help_f’ at qemu-io-cmds.c:2424:9:
>>    qemu-io-cmds.c:2389:9: error: ‘%s’ directive argument is null [-Werror=format-overflow=]
>>     2389 |         printf("%s ", ct->name);
>>          |         ^~~~~~~~~~~~~~~~~~~~~~~
>>
>> Audit shows this can't happen. Give a hint to GCC adding an
>> assert() call.
> 
> This deserves more investigation.  From my glance it appears you are right --
> and moreover impossible for gcc to have come to this conclusion.  Which begs
> the question of how that is.

New entries are added to cmdtab[] in qemuio_add_command() which is 
public (also called in qemu-io.c). So you can insert a cmdinfo_t with a 
name being NULL. This is why I thought GCC was correct first, and I tried:

-- >8 --
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -42,6 +42,7 @@ void qemuio_add_command(const cmdinfo_t *ci)
       * Catch it now rather than letting it manifest as a crash if a
       * particular set of command line options are used.
       */
+    assert(ci->name);
      assert(ci->perm == 0 ||
             (ci->flags & (CMD_FLAG_GLOBAL | CMD_NOFILE_OK)) == 0);
      cmdtab = g_renew(cmdinfo_t, cmdtab, ++ncmds);
---

But this didn't fix the warning... So I moved the assert() in the other 
location.

> 
> Did you file a gcc bug report?

No because I'm not sure this is a bug, but if you confirm I'll file one :)

>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> Cc: Kevin Wolf <kwolf@redhat.com>
>> Cc: Max Reitz <mreitz@redhat.com>
>> Cc: qemu-block@nongnu.org
>> ---
>>   qemu-io-cmds.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
>> index 1b7e700020..9e956a5dd4 100644
>> --- a/qemu-io-cmds.c
>> +++ b/qemu-io-cmds.c
>> @@ -2411,6 +2411,7 @@ static void help_all(void)
>>       const cmdinfo_t *ct;
>>   
>>       for (ct = cmdtab; ct < &cmdtab[ncmds]; ct++) {
>> +        assert(ct->name);
>>           help_oneline(ct->name, ct);
>>       }
>>       printf("\nUse 'help commandname' for extended help.\n");
>>
> 

Re: [PATCH 6/6] qemu-io-cmds: Silent GCC9 format-overflow warning

[Richard Henderson]

On 12/18/19 7:45 AM, Philippe Mathieu-Daudé wrote:
> On 12/18/19 5:15 AM, Richard Henderson wrote:
>> On 12/17/19 7:34 AM, Philippe Mathieu-Daudé wrote:
>>> GCC9 is confused when building with CFLAG -O3:
>>>
>>>    In function ‘help_oneline’,
>>>        inlined from ‘help_all’ at qemu-io-cmds.c:2414:9,
>>>        inlined from ‘help_f’ at qemu-io-cmds.c:2424:9:
>>>    qemu-io-cmds.c:2389:9: error: ‘%s’ directive argument is null
>>> [-Werror=format-overflow=]
>>>     2389 |         printf("%s ", ct->name);
>>>          |         ^~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> Audit shows this can't happen. Give a hint to GCC adding an
>>> assert() call.
>>
>> This deserves more investigation.  From my glance it appears you are right --
>> and moreover impossible for gcc to have come to this conclusion.  Which begs
>> the question of how that is.
> 
> New entries are added to cmdtab[] in qemuio_add_command() which is public (also
> called in qemu-io.c). So you can insert a cmdinfo_t with a name being NULL.
> This is why I thought GCC was correct first, and I tried:
> 
> -- >8 --
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -42,6 +42,7 @@ void qemuio_add_command(const cmdinfo_t *ci)
>       * Catch it now rather than letting it manifest as a crash if a
>       * particular set of command line options are used.
>       */
> +    assert(ci->name);
>      assert(ci->perm == 0 ||
>             (ci->flags & (CMD_FLAG_GLOBAL | CMD_NOFILE_OK)) == 0);
>      cmdtab = g_renew(cmdinfo_t, cmdtab, ++ncmds);
> ---
> 
> But this didn't fix the warning... So I moved the assert() in the other location.
> 
>>
>> Did you file a gcc bug report?
> 
> No because I'm not sure this is a bug, but if you confirm I'll file one :)

The error message is not saying the value *might* be null, but that it *is*
null.  That is, the compiler thinks that it has proven the value can be no
other value than null.

Can I assume that you've tested this particular code path, rather than merely
removed the Werror?

If the compiler really is "proving" that the value must be null, then the
assert should be transformed to assert(false), and qemu will abort at runtime.
 In this case, the reason why the Werror  went away is that the printf is
removed as unreachable beforehand.

But of course the value assigned in qemuio_add_command is truly variable, and
since -O3 does not imply -flto the compiler cannot have proven to see all
callers.  So it follows that the compiler should not have proven that the value
is null.

So there *must* be a compiler bug.  The only question is whether it is isolated
to the printf warning or if it goes further into value propagation and wrong
code generation.


r~