[PATCH 0/9] silence the compiler warnings

[PATCH 0/9] silence the compiler warnings

[Chen Qun]

Hi all,

When building with GCC9 using CFLAG -Wimplicit-fallthrough=2 we get a
lot of warning. Some problems may be missing break statements which 
I have submitted the patch separately.

This series is all add the corresponding "fall through" comment to fix them.


Chen Qun (9):
  target/i386: silence the compiler warnings in gen_shiftd_rm_T1
  hw/intc/arm_gicv3_kvm: silence the compiler warnings
  accel/tcg/user-exec: silence the compiler warnings
  linux-user/mips/cpu_loop: silence the compiler warnings
  target/sparc/translate: silence the compiler warnings
  target/sparc/win_helper: silence the compiler warnings
  ppc: silence the compiler warnings
  target/ppc: silence the compiler warnings
  hw/timer/renesas_tmr: silence the compiler warnings

 accel/tcg/user-exec.c      | 2 +-
 hw/intc/arm_gicv3_kvm.c    | 8 ++++++++
 hw/ppc/ppc.c               | 1 +
 hw/timer/renesas_tmr.c     | 1 +
 linux-user/mips/cpu_loop.c | 4 ++++
 target/i386/translate.c    | 4 ++--
 target/ppc/mmu_helper.c    | 1 +
 target/sparc/translate.c   | 2 +-
 target/sparc/win_helper.c  | 1 +
 9 files changed, 20 insertions(+), 4 deletions(-)

-- 
2.27.0

[PATCH 1/9] target/i386: silence the compiler warnings in gen_shiftd_rm_T1

[Chen Qun]

The current "#ifdef TARGET_X86_64" statement affects
the compiler's determination of fall through.

When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
target/i386/translate.c: In function ‘gen_shiftd_rm_T1’:
target/i386/translate.c:1773:12: warning: this statement may fall through [-Wimplicit-fallthrough=]
         if (is_right) {
            ^
target/i386/translate.c:1782:5: note: here
     case MO_32:
     ^~~~

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Eduardo Habkost <ehabkost@redhat.com>
---
 target/i386/translate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/i386/translate.c b/target/i386/translate.c
index caea6f5fb1..4c353427d7 100644
--- a/target/i386/translate.c
+++ b/target/i386/translate.c
@@ -1777,9 +1777,9 @@ static void gen_shiftd_rm_T1(DisasContext *s, MemOp ot, int op1,
         } else {
             tcg_gen_deposit_tl(s->T1, s->T0, s->T1, 16, 16);
         }
-        /* FALLTHRU */
-#ifdef TARGET_X86_64
+        /* fall through */
     case MO_32:
+#ifdef TARGET_X86_64
         /* Concatenate the two 32-bit values and use a 64-bit shift.  */
         tcg_gen_subi_tl(s->tmp0, count, 1);
         if (is_right) {
-- 
2.27.0

Re: [PATCH 1/9] target/i386: silence the compiler warnings in gen_shiftd_rm_T1

[Thomas Huth]

On 28/10/2020 05.18, Chen Qun wrote:
> The current "#ifdef TARGET_X86_64" statement affects
> the compiler's determination of fall through.
> 
> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
> target/i386/translate.c: In function ‘gen_shiftd_rm_T1’:
> target/i386/translate.c:1773:12: warning: this statement may fall through [-Wimplicit-fallthrough=]
>          if (is_right) {
>             ^
> target/i386/translate.c:1782:5: note: here
>      case MO_32:
>      ^~~~
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target/i386/translate.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/translate.c b/target/i386/translate.c
> index caea6f5fb1..4c353427d7 100644
> --- a/target/i386/translate.c
> +++ b/target/i386/translate.c
> @@ -1777,9 +1777,9 @@ static void gen_shiftd_rm_T1(DisasContext *s, MemOp ot, int op1,
>          } else {
>              tcg_gen_deposit_tl(s->T1, s->T0, s->T1, 16, 16);
>          }
> -        /* FALLTHRU */
> -#ifdef TARGET_X86_64
> +        /* fall through */
>      case MO_32:
> +#ifdef TARGET_X86_64
>          /* Concatenate the two 32-bit values and use a 64-bit shift.  */
>          tcg_gen_subi_tl(s->tmp0, count, 1);
>          if (is_right) {

The whole code here looks a little bit fishy to me ... in case TARGET_X86_64
is defined, the MO_16 code falls through to MO_32 ... but in case it is not
defined, it falls through to the default case that comes after the #ifdef
block? Is this really the right thing here? If so, I think there should be
some additional comments explaining this behavior.

Richard, maybe you could help to judge what is right here...?

 Thomas

Re: [PATCH 1/9] target/i386: silence the compiler warnings in gen_shiftd_rm_T1

[Philippe Mathieu-Daudé]

+Tony

On 10/28/20 1:57 PM, Thomas Huth wrote:
> On 28/10/2020 05.18, Chen Qun wrote:
>> The current "#ifdef TARGET_X86_64" statement affects
>> the compiler's determination of fall through.
>>
>> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
>> target/i386/translate.c: In function ‘gen_shiftd_rm_T1’:
>> target/i386/translate.c:1773:12: warning: this statement may fall through [-Wimplicit-fallthrough=]
>>          if (is_right) {
>>             ^
>> target/i386/translate.c:1782:5: note: here
>>      case MO_32:
>>      ^~~~
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>> ---
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Richard Henderson <richard.henderson@linaro.org>
>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>> ---
>>  target/i386/translate.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/i386/translate.c b/target/i386/translate.c
>> index caea6f5fb1..4c353427d7 100644
>> --- a/target/i386/translate.c
>> +++ b/target/i386/translate.c
>> @@ -1777,9 +1777,9 @@ static void gen_shiftd_rm_T1(DisasContext *s, MemOp ot, int op1,
>>          } else {
>>              tcg_gen_deposit_tl(s->T1, s->T0, s->T1, 16, 16);
>>          }
>> -        /* FALLTHRU */
>> -#ifdef TARGET_X86_64
>> +        /* fall through */
>>      case MO_32:
>> +#ifdef TARGET_X86_64
>>          /* Concatenate the two 32-bit values and use a 64-bit shift.  */
>>          tcg_gen_subi_tl(s->tmp0, count, 1);
>>          if (is_right) {
> 
> The whole code here looks a little bit fishy to me ... in case TARGET_X86_64
> is defined, the MO_16 code falls through to MO_32 ... but in case it is not
> defined, it falls through to the default case that comes after the #ifdef
> block? Is this really the right thing here? If so, I think there should be
> some additional comments explaining this behavior.
> 
> Richard, maybe you could help to judge what is right here...?

I think the previous discussion is this thread:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg632245.html

Re: [PATCH 1/9] target/i386: silence the compiler warnings in gen_shiftd_rm_T1

[Richard Henderson]

On 10/28/20 5:57 AM, Thomas Huth wrote:
> On 28/10/2020 05.18, Chen Qun wrote:
>> The current "#ifdef TARGET_X86_64" statement affects
>> the compiler's determination of fall through.
>>
>> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
>> target/i386/translate.c: In function ‘gen_shiftd_rm_T1’:
>> target/i386/translate.c:1773:12: warning: this statement may fall through [-Wimplicit-fallthrough=]
>>          if (is_right) {
>>             ^
>> target/i386/translate.c:1782:5: note: here
>>      case MO_32:
>>      ^~~~
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>> ---
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Richard Henderson <richard.henderson@linaro.org>
>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>> ---
>>  target/i386/translate.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/i386/translate.c b/target/i386/translate.c
>> index caea6f5fb1..4c353427d7 100644
>> --- a/target/i386/translate.c
>> +++ b/target/i386/translate.c
>> @@ -1777,9 +1777,9 @@ static void gen_shiftd_rm_T1(DisasContext *s, MemOp ot, int op1,
>>          } else {
>>              tcg_gen_deposit_tl(s->T1, s->T0, s->T1, 16, 16);
>>          }
>> -        /* FALLTHRU */
>> -#ifdef TARGET_X86_64
>> +        /* fall through */
>>      case MO_32:
>> +#ifdef TARGET_X86_64
>>          /* Concatenate the two 32-bit values and use a 64-bit shift.  */
>>          tcg_gen_subi_tl(s->tmp0, count, 1);
>>          if (is_right) {
> 
> The whole code here looks a little bit fishy to me ... in case TARGET_X86_64
> is defined, the MO_16 code falls through to MO_32 ... but in case it is not
> defined, it falls through to the default case that comes after the #ifdef
> block? Is this really the right thing here? If so, I think there should be
> some additional comments explaining this behavior.
> 
> Richard, maybe you could help to judge what is right here...?

It could definitely be rewritten, but it's not wrong as is.


r~

Re: [PATCH 1/9] target/i386: silence the compiler warnings in gen_shiftd_rm_T1

[Thomas Huth]

On 28/10/2020 16.31, Richard Henderson wrote:
> On 10/28/20 5:57 AM, Thomas Huth wrote:
>> On 28/10/2020 05.18, Chen Qun wrote:
>>> The current "#ifdef TARGET_X86_64" statement affects
>>> the compiler's determination of fall through.
>>>
>>> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
>>> target/i386/translate.c: In function ‘gen_shiftd_rm_T1’:
>>> target/i386/translate.c:1773:12: warning: this statement may fall through [-Wimplicit-fallthrough=]
>>>          if (is_right) {
>>>             ^
>>> target/i386/translate.c:1782:5: note: here
>>>      case MO_32:
>>>      ^~~~
>>>
>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>>> ---
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: Richard Henderson <richard.henderson@linaro.org>
>>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>>> ---
>>>  target/i386/translate.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/target/i386/translate.c b/target/i386/translate.c
>>> index caea6f5fb1..4c353427d7 100644
>>> --- a/target/i386/translate.c
>>> +++ b/target/i386/translate.c
>>> @@ -1777,9 +1777,9 @@ static void gen_shiftd_rm_T1(DisasContext *s, MemOp ot, int op1,
>>>          } else {
>>>              tcg_gen_deposit_tl(s->T1, s->T0, s->T1, 16, 16);
>>>          }
>>> -        /* FALLTHRU */
>>> -#ifdef TARGET_X86_64
>>> +        /* fall through */
>>>      case MO_32:
>>> +#ifdef TARGET_X86_64
>>>          /* Concatenate the two 32-bit values and use a 64-bit shift.  */
>>>          tcg_gen_subi_tl(s->tmp0, count, 1);
>>>          if (is_right) {
>>
>> The whole code here looks a little bit fishy to me ... in case TARGET_X86_64
>> is defined, the MO_16 code falls through to MO_32 ... but in case it is not
>> defined, it falls through to the default case that comes after the #ifdef
>> block? Is this really the right thing here? If so, I think there should be
>> some additional comments explaining this behavior.
>>
>> Richard, maybe you could help to judge what is right here...?
> 
> It could definitely be rewritten, but it's not wrong as is.

Ok, thanks for the clarification! In that case:

Reviewed-by: Thomas Huth <thuth@redhat.com>

RE: [PATCH 1/9] target/i386: silence the compiler warnings in gen_shiftd_rm_T1

[Chenqun (kuhn)]

> -----Original Message-----
> From: Thomas Huth [mailto:thuth@redhat.com]
> Sent: Thursday, October 29, 2020 12:52 AM
> To: Richard Henderson <richard.henderson@linaro.org>; Chenqun (kuhn)
> <kuhn.chenqun@huawei.com>; qemu-devel@nongnu.org;
> qemu-trivial@nongnu.org
> Cc: Eduardo Habkost <ehabkost@redhat.com>; Zhanghailiang
> <zhang.zhanghailiang@huawei.com>; ganqixin <ganqixin@huawei.com>; Euler
> Robot <euler.robot@huawei.com>; Paolo Bonzini <pbonzini@redhat.com>;
> Richard Henderson <rth@twiddle.net>
> Subject: Re: [PATCH 1/9] target/i386: silence the compiler warnings in
> gen_shiftd_rm_T1
> 
> On 28/10/2020 16.31, Richard Henderson wrote:
> > On 10/28/20 5:57 AM, Thomas Huth wrote:
> >> On 28/10/2020 05.18, Chen Qun wrote:
> >>> The current "#ifdef TARGET_X86_64" statement affects the compiler's
> >>> determination of fall through.
> >>>
> >>> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed
> warning:
> >>> target/i386/translate.c: In function ‘gen_shiftd_rm_T1’:
> >>> target/i386/translate.c:1773:12: warning: this statement may fall through
> [-Wimplicit-fallthrough=]
> >>>          if (is_right) {
> >>>             ^
> >>> target/i386/translate.c:1782:5: note: here
> >>>      case MO_32:
> >>>      ^~~~
> >>>
> >>> Reported-by: Euler Robot <euler.robot@huawei.com>
> >>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> >>> ---
> >>> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >>> Cc: Richard Henderson <richard.henderson@linaro.org>
> >>> Cc: Eduardo Habkost <ehabkost@redhat.com>
> >>> ---
> >>>  target/i386/translate.c | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/target/i386/translate.c b/target/i386/translate.c index
> >>> caea6f5fb1..4c353427d7 100644
> >>> --- a/target/i386/translate.c
> >>> +++ b/target/i386/translate.c
> >>> @@ -1777,9 +1777,9 @@ static void gen_shiftd_rm_T1(DisasContext *s,
> MemOp ot, int op1,
> >>>          } else {
> >>>              tcg_gen_deposit_tl(s->T1, s->T0, s->T1, 16, 16);
> >>>          }
> >>> -        /* FALLTHRU */
> >>> -#ifdef TARGET_X86_64
> >>> +        /* fall through */
> >>>      case MO_32:
> >>> +#ifdef TARGET_X86_64
> >>>          /* Concatenate the two 32-bit values and use a 64-bit shift.
> */
> >>>          tcg_gen_subi_tl(s->tmp0, count, 1);
> >>>          if (is_right) {
> >>
> >> The whole code here looks a little bit fishy to me ... in case
> >> TARGET_X86_64 is defined, the MO_16 code falls through to MO_32 ...
> >> but in case it is not defined, it falls through to the default case
> >> that comes after the #ifdef block? Is this really the right thing
> >> here? If so, I think there should be some additional comments explaining
> this behavior.
> >>
> >> Richard, maybe you could help to judge what is right here...?
> >
> > It could definitely be rewritten, but it's not wrong as is.
> 
> Ok, thanks for the clarification! In that case:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Thanks for the discussion!
I might add a little comment to describe this behavior base on this patch.
Just like: /* If TARGET_X86_64 defined then fall through into MO_32, otherwise fall through default. */

If there is no other suggestion, I'll keep Richard's and Thomas 's "Reviewed-by" tag.

Thanks,
Chen Qun

Re: [PATCH 1/9] target/i386: silence the compiler warnings in gen_shiftd_rm_T1

[Richard Henderson]

On 10/27/20 9:18 PM, Chen Qun wrote:
> The current "#ifdef TARGET_X86_64" statement affects
> the compiler's determination of fall through.
> 
> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
> target/i386/translate.c: In function ‘gen_shiftd_rm_T1’:
> target/i386/translate.c:1773:12: warning: this statement may fall through [-Wimplicit-fallthrough=]
>          if (is_right) {
>             ^
> target/i386/translate.c:1782:5: note: here
>      case MO_32:
>      ^~~~
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target/i386/translate.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/translate.c b/target/i386/translate.c
> index caea6f5fb1..4c353427d7 100644
> --- a/target/i386/translate.c
> +++ b/target/i386/translate.c
> @@ -1777,9 +1777,9 @@ static void gen_shiftd_rm_T1(DisasContext *s, MemOp ot, int op1,
>          } else {
>              tcg_gen_deposit_tl(s->T1, s->T0, s->T1, 16, 16);
>          }
> -        /* FALLTHRU */
> -#ifdef TARGET_X86_64
> +        /* fall through */
>      case MO_32:
> +#ifdef TARGET_X86_64
>          /* Concatenate the two 32-bit values and use a 64-bit shift.  *
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH 2/9] hw/intc/arm_gicv3_kvm: silence the compiler warnings

[Chen Qun]

When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
hw/intc/arm_gicv3_kvm.c: In function ‘kvm_arm_gicv3_put’:
hw/intc/arm_gicv3_kvm.c:484:13: warning: this statement may fall through [-Wimplicit-fallthrough=]
             kvm_gicc_access(s, ICC_AP0R_EL1(1), ncpu, &reg64, true);
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
hw/intc/arm_gicv3_kvm.c:485:9: note: here
         default:
         ^~~~~~~
hw/intc/arm_gicv3_kvm.c:495:13: warning: this statement may fall through [-Wimplicit-fallthrough=]
             kvm_gicc_access(s, ICC_AP1R_EL1(2), ncpu, &reg64, true);
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
hw/intc/arm_gicv3_kvm.c:496:9: note: here
         case 6:
         ^~~~
hw/intc/arm_gicv3_kvm.c:498:13: warning: this statement may fall through [-Wimplicit-fallthrough=]
             kvm_gicc_access(s, ICC_AP1R_EL1(1), ncpu, &reg64, true);
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
hw/intc/arm_gicv3_kvm.c:499:9: note: here
         default:
         ^~~~~~~

hw/intc/arm_gicv3_kvm.c: In function ‘kvm_arm_gicv3_get’:
hw/intc/arm_gicv3_kvm.c:634:37: warning: this statement may fall through [-Wimplicit-fallthrough=]
             c->icc_apr[GICV3_G0][2] = reg64;
             ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
hw/intc/arm_gicv3_kvm.c:635:9: note: here
         case 6:
         ^~~~
hw/intc/arm_gicv3_kvm.c:637:37: warning: this statement may fall through [-Wimplicit-fallthrough=]
             c->icc_apr[GICV3_G0][1] = reg64;
             ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
hw/intc/arm_gicv3_kvm.c:638:9: note: here
         default:
         ^~~~~~~
hw/intc/arm_gicv3_kvm.c:648:39: warning: this statement may fall through [-Wimplicit-fallthrough=]
             c->icc_apr[GICV3_G1NS][2] = reg64;
             ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
hw/intc/arm_gicv3_kvm.c:649:9: note: here
         case 6:
         ^~~~
hw/intc/arm_gicv3_kvm.c:651:39: warning: this statement may fall through [-Wimplicit-fallthrough=]
             c->icc_apr[GICV3_G1NS][1] = reg64;
             ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
hw/intc/arm_gicv3_kvm.c:652:9: note: here
         default:
         ^~~~~~~

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
---
 hw/intc/arm_gicv3_kvm.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index 187eb054e0..d040a5d1e9 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -478,9 +478,11 @@ static void kvm_arm_gicv3_put(GICv3State *s)
             kvm_gicc_access(s, ICC_AP0R_EL1(3), ncpu, &reg64, true);
             reg64 = c->icc_apr[GICV3_G0][2];
             kvm_gicc_access(s, ICC_AP0R_EL1(2), ncpu, &reg64, true);
+            /* fall through */
         case 6:
             reg64 = c->icc_apr[GICV3_G0][1];
             kvm_gicc_access(s, ICC_AP0R_EL1(1), ncpu, &reg64, true);
+            /* fall through */
         default:
             reg64 = c->icc_apr[GICV3_G0][0];
             kvm_gicc_access(s, ICC_AP0R_EL1(0), ncpu, &reg64, true);
@@ -492,9 +494,11 @@ static void kvm_arm_gicv3_put(GICv3State *s)
             kvm_gicc_access(s, ICC_AP1R_EL1(3), ncpu, &reg64, true);
             reg64 = c->icc_apr[GICV3_G1NS][2];
             kvm_gicc_access(s, ICC_AP1R_EL1(2), ncpu, &reg64, true);
+            /* fall through */
         case 6:
             reg64 = c->icc_apr[GICV3_G1NS][1];
             kvm_gicc_access(s, ICC_AP1R_EL1(1), ncpu, &reg64, true);
+            /* fall through */
         default:
             reg64 = c->icc_apr[GICV3_G1NS][0];
             kvm_gicc_access(s, ICC_AP1R_EL1(0), ncpu, &reg64, true);
@@ -631,9 +635,11 @@ static void kvm_arm_gicv3_get(GICv3State *s)
             c->icc_apr[GICV3_G0][3] = reg64;
             kvm_gicc_access(s, ICC_AP0R_EL1(2), ncpu, &reg64, false);
             c->icc_apr[GICV3_G0][2] = reg64;
+            /* fall through */
         case 6:
             kvm_gicc_access(s, ICC_AP0R_EL1(1), ncpu, &reg64, false);
             c->icc_apr[GICV3_G0][1] = reg64;
+            /* fall through */
         default:
             kvm_gicc_access(s, ICC_AP0R_EL1(0), ncpu, &reg64, false);
             c->icc_apr[GICV3_G0][0] = reg64;
@@ -645,9 +651,11 @@ static void kvm_arm_gicv3_get(GICv3State *s)
             c->icc_apr[GICV3_G1NS][3] = reg64;
             kvm_gicc_access(s, ICC_AP1R_EL1(2), ncpu, &reg64, false);
             c->icc_apr[GICV3_G1NS][2] = reg64;
+            /* fall through */
         case 6:
             kvm_gicc_access(s, ICC_AP1R_EL1(1), ncpu, &reg64, false);
             c->icc_apr[GICV3_G1NS][1] = reg64;
+            /* fall through */
         default:
             kvm_gicc_access(s, ICC_AP1R_EL1(0), ncpu, &reg64, false);
             c->icc_apr[GICV3_G1NS][0] = reg64;
-- 
2.27.0

Re: [PATCH 2/9] hw/intc/arm_gicv3_kvm: silence the compiler warnings

[Peter Maydell]

On Wed, 28 Oct 2020 at 04:19, Chen Qun <kuhn.chenqun@huawei.com> wrote:
>
> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
> hw/intc/arm_gicv3_kvm.c: In function ‘kvm_arm_gicv3_put’:
> hw/intc/arm_gicv3_kvm.c:484:13: warning: this statement may fall through [-Wimplicit-fallthrough=]
>              kvm_gicc_access(s, ICC_AP0R_EL1(1), ncpu, &reg64, true);
>              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> hw/intc/arm_gicv3_kvm.c:485:9: note: here
>          default:
>          ^~~~~~~
> hw/intc/arm_gicv3_kvm.c:495:13: warning: this statement may fall through [-Wimplicit-fallthrough=]
>              kvm_gicc_access(s, ICC_AP1R_EL1(2), ncpu, &reg64, true);
>              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> hw/intc/arm_gicv3_kvm.c:496:9: note: here
>          case 6:
>          ^~~~
> hw/intc/arm_gicv3_kvm.c:498:13: warning: this statement may fall through [-Wimplicit-fallthrough=]
>              kvm_gicc_access(s, ICC_AP1R_EL1(1), ncpu, &reg64, true);
>              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> hw/intc/arm_gicv3_kvm.c:499:9: note: here
>          default:
>          ^~~~~~~
>
> hw/intc/arm_gicv3_kvm.c: In function ‘kvm_arm_gicv3_get’:
> hw/intc/arm_gicv3_kvm.c:634:37: warning: this statement may fall through [-Wimplicit-fallthrough=]
>              c->icc_apr[GICV3_G0][2] = reg64;
>              ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
> hw/intc/arm_gicv3_kvm.c:635:9: note: here
>          case 6:
>          ^~~~
> hw/intc/arm_gicv3_kvm.c:637:37: warning: this statement may fall through [-Wimplicit-fallthrough=]
>              c->icc_apr[GICV3_G0][1] = reg64;
>              ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
> hw/intc/arm_gicv3_kvm.c:638:9: note: here
>          default:
>          ^~~~~~~
> hw/intc/arm_gicv3_kvm.c:648:39: warning: this statement may fall through [-Wimplicit-fallthrough=]
>              c->icc_apr[GICV3_G1NS][2] = reg64;
>              ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
> hw/intc/arm_gicv3_kvm.c:649:9: note: here
>          case 6:
>          ^~~~
> hw/intc/arm_gicv3_kvm.c:651:39: warning: this statement may fall through [-Wimplicit-fallthrough=]
>              c->icc_apr[GICV3_G1NS][1] = reg64;
>              ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
> hw/intc/arm_gicv3_kvm.c:652:9: note: here
>          default:
>          ^~~~~~~
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>

Yep, these are all intentionall fallthrough.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 3/9] accel/tcg/user-exec: silence the compiler warnings

[Chen Qun]

When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
../accel/tcg/user-exec.c: In function ‘handle_cpu_signal’:
../accel/tcg/user-exec.c:169:13: warning: this statement may fall through [-Wimplicit-fallthrough=]
  169 |             cpu_exit_tb_from_sighandler(cpu, old_set);
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../accel/tcg/user-exec.c:172:9: note: here
  172 |         default:

This exception branch fall through the 'default' branch and run the 'g_assert_not_reached' statement.
So we could use "fall through" instead of "NORETURN" here.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Riku Voipio <riku.voipio@iki.fi>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 accel/tcg/user-exec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 4ebe25461a..330468e990 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -167,7 +167,7 @@ static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,
              */
             clear_helper_retaddr();
             cpu_exit_tb_from_sighandler(cpu, old_set);
-            /* NORETURN */
+            /* fall through */
 
         default:
             g_assert_not_reached();
-- 
2.27.0

Re: [PATCH 3/9] accel/tcg/user-exec: silence the compiler warnings

[Thomas Huth]

On 28/10/2020 05.18, Chen Qun wrote:
> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
> ../accel/tcg/user-exec.c: In function ‘handle_cpu_signal’:
> ../accel/tcg/user-exec.c:169:13: warning: this statement may fall through [-Wimplicit-fallthrough=]
>   169 |             cpu_exit_tb_from_sighandler(cpu, old_set);
>       |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../accel/tcg/user-exec.c:172:9: note: here
>   172 |         default:
> 
> This exception branch fall through the 'default' branch and run the 'g_assert_not_reached' statement.
> So we could use "fall through" instead of "NORETURN" here.
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> Cc: Riku Voipio <riku.voipio@iki.fi>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  accel/tcg/user-exec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
> index 4ebe25461a..330468e990 100644
> --- a/accel/tcg/user-exec.c
> +++ b/accel/tcg/user-exec.c
> @@ -167,7 +167,7 @@ static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,
>               */
>              clear_helper_retaddr();
>              cpu_exit_tb_from_sighandler(cpu, old_set);
> -            /* NORETURN */
> +            /* fall through */

There should not be a fall through here since the previous function should
never return. Does the warning go away if you mark the
cpu_exit_tb_from_sighandler() function with QEMU_NORETURN ? If so, I think
that would be the better fix.

 Thomas

Re: [PATCH 3/9] accel/tcg/user-exec: silence the compiler warnings

[Richard Henderson]

On 10/28/20 6:52 AM, Thomas Huth wrote:
> On 28/10/2020 05.18, Chen Qun wrote:
>> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
>> ../accel/tcg/user-exec.c: In function ‘handle_cpu_signal’:
>> ../accel/tcg/user-exec.c:169:13: warning: this statement may fall through [-Wimplicit-fallthrough=]
>>   169 |             cpu_exit_tb_from_sighandler(cpu, old_set);
>>       |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> ../accel/tcg/user-exec.c:172:9: note: here
>>   172 |         default:
>>
>> This exception branch fall through the 'default' branch and run the 'g_assert_not_reached' statement.
>> So we could use "fall through" instead of "NORETURN" here.
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>> ---
>> Cc: Riku Voipio <riku.voipio@iki.fi>
>> Cc: Richard Henderson <richard.henderson@linaro.org>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  accel/tcg/user-exec.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
>> index 4ebe25461a..330468e990 100644
>> --- a/accel/tcg/user-exec.c
>> +++ b/accel/tcg/user-exec.c
>> @@ -167,7 +167,7 @@ static inline int handle_cpu_signal(uintptr_t pc, siginfo_t *info,
>>               */
>>              clear_helper_retaddr();
>>              cpu_exit_tb_from_sighandler(cpu, old_set);
>> -            /* NORETURN */
>> +            /* fall through */
> 
> There should not be a fall through here since the previous function should
> never return. Does the warning go away if you mark the
> cpu_exit_tb_from_sighandler() function with QEMU_NORETURN ? If so, I think
> that would be the better fix.

The compiler should have figured that out itself, due to cpu_loop_exit_noexc
being marked QEMU_NORETURN.  However,
if adding a second QEMU_NORETURN works, I'm fine with that.

As a very last resort, we can change the comment to

    /* no return, but fall through to assert not reached */

which correctly documents both the function preceding and also contains the
regexp that the compiler is using for the warning.


r~

RE: [PATCH 3/9] accel/tcg/user-exec: silence the compiler warnings

[Chenqun (kuhn)]

> -----Original Message-----
> From: Richard Henderson [mailto:richard.henderson@linaro.org]
> Sent: Wednesday, October 28, 2020 11:38 PM
> To: Thomas Huth <thuth@redhat.com>; Chenqun (kuhn)
> <kuhn.chenqun@huawei.com>; qemu-devel@nongnu.org;
> qemu-trivial@nongnu.org
> Cc: Zhanghailiang <zhang.zhanghailiang@huawei.com>; Riku Voipio
> <riku.voipio@iki.fi>; Paolo Bonzini <pbonzini@redhat.com>; ganqixin
> <ganqixin@huawei.com>; Euler Robot <euler.robot@huawei.com>
> Subject: Re: [PATCH 3/9] accel/tcg/user-exec: silence the compiler warnings
> 
> On 10/28/20 6:52 AM, Thomas Huth wrote:
> > On 28/10/2020 05.18, Chen Qun wrote:
> >> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed
> warning:
> >> ../accel/tcg/user-exec.c: In function ‘handle_cpu_signal’:
> >> ../accel/tcg/user-exec.c:169:13: warning: this statement may fall through
> [-Wimplicit-fallthrough=]
> >>   169 |             cpu_exit_tb_from_sighandler(cpu, old_set);
> >>       |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> ../accel/tcg/user-exec.c:172:9: note: here
> >>   172 |         default:
> >>
> >> This exception branch fall through the 'default' branch and run the
> 'g_assert_not_reached' statement.
> >> So we could use "fall through" instead of "NORETURN" here.
> >>
> >> Reported-by: Euler Robot <euler.robot@huawei.com>
> >> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> >> ---
> >> Cc: Riku Voipio <riku.voipio@iki.fi>
> >> Cc: Richard Henderson <richard.henderson@linaro.org>
> >> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >>  accel/tcg/user-exec.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c index
> >> 4ebe25461a..330468e990 100644
> >> --- a/accel/tcg/user-exec.c
> >> +++ b/accel/tcg/user-exec.c
> >> @@ -167,7 +167,7 @@ static inline int handle_cpu_signal(uintptr_t pc,
> siginfo_t *info,
> >>               */
> >>              clear_helper_retaddr();
> >>              cpu_exit_tb_from_sighandler(cpu, old_set);
> >> -            /* NORETURN */
> >> +            /* fall through */
> >
> > There should not be a fall through here since the previous function
> > should never return. Does the warning go away if you mark the
> > cpu_exit_tb_from_sighandler() function with QEMU_NORETURN ? If so, I
> > think that would be the better fix.
> 
> The compiler should have figured that out itself, due to cpu_loop_exit_noexc
> being marked QEMU_NORETURN.  However, if adding a second
> QEMU_NORETURN works, I'm fine with that.
> 
  I tried to add QEMU_NORETURN to the cpu_exit_tb_from_sighandler() function.
And then the compiler warning was cleared. It seems to be a better fix.

Thanks,
Chen Qun
> As a very last resort, we can change the comment to
> 
>     /* no return, but fall through to assert not reached */
> 
> which correctly documents both the function preceding and also contains the
> regexp that the compiler is using for the warning.
> 

[PATCH 4/9] linux-user/mips/cpu_loop: silence the compiler warnings

[Chen Qun]

When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
linux-user/mips/cpu_loop.c: In function ‘cpu_loop’:
linux-user/mips/cpu_loop.c:104:24: warning: this statement may fall through [-Wimplicit-fallthrough=]
  104 |                     if ((ret = get_user_ual(arg8, sp_reg + 28)) != 0) {
      |                        ^
linux-user/mips/cpu_loop.c:107:17: note: here
  107 |                 case 7:
      |                 ^~~~
linux-user/mips/cpu_loop.c:108:24: warning: this statement may fall through [-Wimplicit-fallthrough=]
  108 |                     if ((ret = get_user_ual(arg7, sp_reg + 24)) != 0) {
      |                        ^
linux-user/mips/cpu_loop.c:111:17: note: here
  111 |                 case 6:
      |                 ^~~~
linux-user/mips/cpu_loop.c:112:24: warning: this statement may fall through [-Wimplicit-fallthrough=]
  112 |                     if ((ret = get_user_ual(arg6, sp_reg + 20)) != 0) {
      |                        ^
linux-user/mips/cpu_loop.c:115:17: note: here
  115 |                 case 5:
      |                 ^~~~

Add the corresponding "fall through" comment to fix it.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Laurent Vivier <laurent@vivier.eu>
---
 linux-user/mips/cpu_loop.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/linux-user/mips/cpu_loop.c b/linux-user/mips/cpu_loop.c
index 553e8ca7f5..cfe7ba5c47 100644
--- a/linux-user/mips/cpu_loop.c
+++ b/linux-user/mips/cpu_loop.c
@@ -104,18 +104,22 @@ void cpu_loop(CPUMIPSState *env)
                     if ((ret = get_user_ual(arg8, sp_reg + 28)) != 0) {
                         goto done_syscall;
                     }
+                    /* fall through */
                 case 7:
                     if ((ret = get_user_ual(arg7, sp_reg + 24)) != 0) {
                         goto done_syscall;
                     }
+                    /* fall through */
                 case 6:
                     if ((ret = get_user_ual(arg6, sp_reg + 20)) != 0) {
                         goto done_syscall;
                     }
+                    /* fall through */
                 case 5:
                     if ((ret = get_user_ual(arg5, sp_reg + 16)) != 0) {
                         goto done_syscall;
                     }
+                    /* fall through */
                 default:
                     break;
                 }
-- 
2.27.0

Re: [PATCH 4/9] linux-user/mips/cpu_loop: silence the compiler warnings

[Thomas Huth]

On 28/10/2020 05.18, Chen Qun wrote:
> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
> linux-user/mips/cpu_loop.c: In function ‘cpu_loop’:
> linux-user/mips/cpu_loop.c:104:24: warning: this statement may fall through [-Wimplicit-fallthrough=]
>   104 |                     if ((ret = get_user_ual(arg8, sp_reg + 28)) != 0) {
>       |                        ^
> linux-user/mips/cpu_loop.c:107:17: note: here
>   107 |                 case 7:
>       |                 ^~~~
> linux-user/mips/cpu_loop.c:108:24: warning: this statement may fall through [-Wimplicit-fallthrough=]
>   108 |                     if ((ret = get_user_ual(arg7, sp_reg + 24)) != 0) {
>       |                        ^
> linux-user/mips/cpu_loop.c:111:17: note: here
>   111 |                 case 6:
>       |                 ^~~~
> linux-user/mips/cpu_loop.c:112:24: warning: this statement may fall through [-Wimplicit-fallthrough=]
>   112 |                     if ((ret = get_user_ual(arg6, sp_reg + 20)) != 0) {
>       |                        ^
> linux-user/mips/cpu_loop.c:115:17: note: here
>   115 |                 case 5:
>       |                 ^~~~
> 
> Add the corresponding "fall through" comment to fix it.
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> Cc: Laurent Vivier <laurent@vivier.eu>
> ---
>  linux-user/mips/cpu_loop.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/linux-user/mips/cpu_loop.c b/linux-user/mips/cpu_loop.c
> index 553e8ca7f5..cfe7ba5c47 100644
> --- a/linux-user/mips/cpu_loop.c
> +++ b/linux-user/mips/cpu_loop.c
> @@ -104,18 +104,22 @@ void cpu_loop(CPUMIPSState *env)
>                      if ((ret = get_user_ual(arg8, sp_reg + 28)) != 0) {
>                          goto done_syscall;
>                      }
> +                    /* fall through */
>                  case 7:
>                      if ((ret = get_user_ual(arg7, sp_reg + 24)) != 0) {
>                          goto done_syscall;
>                      }
> +                    /* fall through */
>                  case 6:
>                      if ((ret = get_user_ual(arg6, sp_reg + 20)) != 0) {
>                          goto done_syscall;
>                      }
> +                    /* fall through */
>                  case 5:
>                      if ((ret = get_user_ual(arg5, sp_reg + 16)) != 0) {
>                          goto done_syscall;
>                      }
> +                    /* fall through */
>                  default:
>                      break;
>                  }

Reviewed-by: Thomas Huth <thuth@redhat.com>

[PATCH 5/9] target/sparc/translate: silence the compiler warnings

[Chen Qun]

When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
target/sparc/translate.c: In function ‘gen_st_asi’:
target/sparc/translate.c:2320:12: warning: this statement may fall through [-Wimplicit-fallthrough=]
 2320 |         if (!(dc->def->features & CPU_FEATURE_HYPV)) {
      |            ^
target/sparc/translate.c:2329:5: note: here
 2329 |     case GET_ASI_DIRECT:
      |     ^~~~

The "fall through" statement place is not correctly identified by the compiler.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: Artyom Tarasenko <atar4qemu@gmail.com>
---
 target/sparc/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index 1a4efd4ed6..a3d9aaa46b 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -2324,8 +2324,8 @@ static void gen_st_asi(DisasContext *dc, TCGv src, TCGv addr,
         }
         /* in OpenSPARC T1+ CPUs TWINX ASIs in store instructions
          * are ST_BLKINIT_ ASIs */
-        /* fall through */
 #endif
+        /* fall through */
     case GET_ASI_DIRECT:
         gen_address_mask(dc, addr);
         tcg_gen_qemu_st_tl(src, addr, da.mem_idx, da.memop);
-- 
2.27.0

Re: [PATCH 5/9] target/sparc/translate: silence the compiler warnings

[Artyom Tarasenko]

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

ср, 28 окт. 2020 г., 5:19 Chen Qun <kuhn.chenqun@huawei.com>:

> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed
> warning:
> target/sparc/translate.c: In function ‘gen_st_asi’:
> target/sparc/translate.c:2320:12: warning: this statement may fall through
> [-Wimplicit-fallthrough=]
>  2320 |         if (!(dc->def->features & CPU_FEATURE_HYPV)) {
>       |            ^
> target/sparc/translate.c:2329:5: note: here
>  2329 |     case GET_ASI_DIRECT:
>       |     ^~~~
>
> The "fall through" statement place is not correctly identified by the
> compiler.
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>

Reviewed-by: Artyom Tarasenko <atar4qemu@gmail.com>

---
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: Artyom Tarasenko <atar4qemu@gmail.com>
> ---
>  target/sparc/translate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/sparc/translate.c b/target/sparc/translate.c
> index 1a4efd4ed6..a3d9aaa46b 100644
> --- a/target/sparc/translate.c
> +++ b/target/sparc/translate.c
> @@ -2324,8 +2324,8 @@ static void gen_st_asi(DisasContext *dc, TCGv src,
> TCGv addr,
>          }
>          /* in OpenSPARC T1+ CPUs TWINX ASIs in store instructions
>           * are ST_BLKINIT_ ASIs */
> -        /* fall through */
>  #endif
> +        /* fall through */
>      case GET_ASI_DIRECT:
>          gen_address_mask(dc, addr);
>          tcg_gen_qemu_st_tl(src, addr, da.mem_idx, da.memop);
> --
> 2.27.0
>
>

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

Re: [PATCH 5/9] target/sparc/translate: silence the compiler warnings

[Philippe Mathieu-Daudé]

On 10/28/20 5:18 AM, Chen Qun wrote:
> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
> target/sparc/translate.c: In function ‘gen_st_asi’:
> target/sparc/translate.c:2320:12: warning: this statement may fall through [-Wimplicit-fallthrough=]
>  2320 |         if (!(dc->def->features & CPU_FEATURE_HYPV)) {
>       |            ^
> target/sparc/translate.c:2329:5: note: here
>  2329 |     case GET_ASI_DIRECT:
>       |     ^~~~
> 
> The "fall through" statement place is not correctly identified by the compiler.
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: Artyom Tarasenko <atar4qemu@gmail.com>
> ---
>  target/sparc/translate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/sparc/translate.c b/target/sparc/translate.c
> index 1a4efd4ed6..a3d9aaa46b 100644
> --- a/target/sparc/translate.c
> +++ b/target/sparc/translate.c
> @@ -2324,8 +2324,8 @@ static void gen_st_asi(DisasContext *dc, TCGv src, TCGv addr,
>          }
>          /* in OpenSPARC T1+ CPUs TWINX ASIs in store instructions
>           * are ST_BLKINIT_ ASIs */
> -        /* fall through */
>  #endif
> +        /* fall through */
>      case GET_ASI_DIRECT:
>          gen_address_mask(dc, addr);
>          tcg_gen_qemu_st_tl(src, addr, da.mem_idx, da.memop);
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH 6/9] target/sparc/win_helper: silence the compiler warnings

[Chen Qun]

When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
target/sparc/win_helper.c: In function ‘get_gregset’:
target/sparc/win_helper.c:304:9: warning: this statement may fall through [-Wimplicit-fallthrough=]
  304 |         trace_win_helper_gregset_error(pstate);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
target/sparc/win_helper.c:306:5: note: here
  306 |     case 0:
      |     ^~~~

Add the corresponding "fall through" comment to fix it.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: Artyom Tarasenko <atar4qemu@gmail.com>
---
 target/sparc/win_helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/sparc/win_helper.c b/target/sparc/win_helper.c
index 8290a21142..32eacc05e6 100644
--- a/target/sparc/win_helper.c
+++ b/target/sparc/win_helper.c
@@ -303,6 +303,7 @@ static inline uint64_t *get_gregset(CPUSPARCState *env, uint32_t pstate)
     default:
         trace_win_helper_gregset_error(pstate);
         /* pass through to normal set of global registers */
+        /* fall through */
     case 0:
         return env->bgregs;
     case PS_AG:
-- 
2.27.0

Re: [PATCH 6/9] target/sparc/win_helper: silence the compiler warnings

[Artyom Tarasenko]

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

ср, 28 окт. 2020 г., 5:19 Chen Qun <kuhn.chenqun@huawei.com>:

> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed
> warning:
> target/sparc/win_helper.c: In function ‘get_gregset’:
> target/sparc/win_helper.c:304:9: warning: this statement may fall through
> [-Wimplicit-fallthrough=]
>   304 |         trace_win_helper_gregset_error(pstate);
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> target/sparc/win_helper.c:306:5: note: here
>   306 |     case 0:
>       |     ^~~~
>
> Add the corresponding "fall through" comment to fix it.
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>

Reviewed-by: Artyom Tarasenko <atar4qemu@gmail.com>

---
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: Artyom Tarasenko <atar4qemu@gmail.com>
> ---
>  target/sparc/win_helper.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/target/sparc/win_helper.c b/target/sparc/win_helper.c
> index 8290a21142..32eacc05e6 100644
> --- a/target/sparc/win_helper.c
> +++ b/target/sparc/win_helper.c
> @@ -303,6 +303,7 @@ static inline uint64_t *get_gregset(CPUSPARCState
> *env, uint32_t pstate)
>      default:
>          trace_win_helper_gregset_error(pstate);
>          /* pass through to normal set of global registers */
> +        /* fall through */
>      case 0:
>          return env->bgregs;
>      case PS_AG:
> --
> 2.27.0
>
>

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

Re: [PATCH 6/9] target/sparc/win_helper: silence the compiler warnings

[Philippe Mathieu-Daudé]

On 10/28/20 5:18 AM, Chen Qun wrote:
> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
> target/sparc/win_helper.c: In function ‘get_gregset’:
> target/sparc/win_helper.c:304:9: warning: this statement may fall through [-Wimplicit-fallthrough=]
>   304 |         trace_win_helper_gregset_error(pstate);
>       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> target/sparc/win_helper.c:306:5: note: here
>   306 |     case 0:
>       |     ^~~~
> 
> Add the corresponding "fall through" comment to fix it.
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: Artyom Tarasenko <atar4qemu@gmail.com>
> ---
>  target/sparc/win_helper.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/target/sparc/win_helper.c b/target/sparc/win_helper.c
> index 8290a21142..32eacc05e6 100644
> --- a/target/sparc/win_helper.c
> +++ b/target/sparc/win_helper.c
> @@ -303,6 +303,7 @@ static inline uint64_t *get_gregset(CPUSPARCState *env, uint32_t pstate)
>      default:
>          trace_win_helper_gregset_error(pstate);
>          /* pass through to normal set of global registers */
> +        /* fall through */

Can you keep the comment, doing s/pass/fall/?

>      case 0:
>          return env->bgregs;
>      case PS_AG:
> 

RE: [PATCH 6/9] target/sparc/win_helper: silence the compiler warnings

[Chenqun (kuhn)]

> -----Original Message-----
> From: Philippe Mathieu-Daudé [mailto:philippe.mathieu.daude@gmail.com]
> On Behalf Of Philippe Mathieu-Daudé
> Sent: Wednesday, October 28, 2020 5:51 PM
> To: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; qemu-devel@nongnu.org;
> qemu-trivial@nongnu.org
> Cc: Zhanghailiang <zhang.zhanghailiang@huawei.com>; Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk>; ganqixin <ganqixin@huawei.com>; Euler
> Robot <euler.robot@huawei.com>; Artyom Tarasenko
> <atar4qemu@gmail.com>
> Subject: Re: [PATCH 6/9] target/sparc/win_helper: silence the compiler
> warnings
> 
> On 10/28/20 5:18 AM, Chen Qun wrote:
> > When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed
> warning:
> > target/sparc/win_helper.c: In function ‘get_gregset’:
> > target/sparc/win_helper.c:304:9: warning: this statement may fall through
> [-Wimplicit-fallthrough=]
> >   304 |         trace_win_helper_gregset_error(pstate);
> >       |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > target/sparc/win_helper.c:306:5: note: here
> >   306 |     case 0:
> >       |     ^~~~
> >
> > Add the corresponding "fall through" comment to fix it.
> >
> > Reported-by: Euler Robot <euler.robot@huawei.com>
> > Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> > ---
> > Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > Cc: Artyom Tarasenko <atar4qemu@gmail.com>
> > ---
> >  target/sparc/win_helper.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/target/sparc/win_helper.c b/target/sparc/win_helper.c
> > index 8290a21142..32eacc05e6 100644
> > --- a/target/sparc/win_helper.c
> > +++ b/target/sparc/win_helper.c
> > @@ -303,6 +303,7 @@ static inline uint64_t *get_gregset(CPUSPARCState
> *env, uint32_t pstate)
> >      default:
> >          trace_win_helper_gregset_error(pstate);
> >          /* pass through to normal set of global registers */
> > +        /* fall through */
> 
> Can you keep the comment, doing s/pass/fall/?
> 
That looks good, too.

Thanks,
Chen Qun
> >      case 0:
> >          return env->bgregs;
> >      case PS_AG:
> >

[PATCH 7/9] ppc: silence the compiler warnings

[Chen Qun]

When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
hw/ppc/ppc.c: In function ‘ppc6xx_set_irq’:
hw/ppc/ppc.c:118:16: warning: this statement may fall through [-Wimplicit-fallthrough=]
  118 |             if (level) {
      |                ^
hw/ppc/ppc.c:123:9: note: here
  123 |         case PPC6xx_INPUT_INT:
      |         ^~~~

Add the corresponding "fall through" comment to fix it.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/ppc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 4a11fb1640..f9eb8f21b4 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -120,6 +120,7 @@ static void ppc6xx_set_irq(void *opaque, int pin, int level)
             } else {
                 cpu_ppc_tb_stop(env);
             }
+            /* fall through */
         case PPC6xx_INPUT_INT:
             /* Level sensitive - active high */
             LOG_IRQ("%s: set the external IRQ state to %d\n",
-- 
2.27.0

Re: [PATCH 7/9] ppc: silence the compiler warnings

[David Gibson]

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

On Wed, Oct 28, 2020 at 12:18:17PM +0800, Chen Qun wrote:
> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
> hw/ppc/ppc.c: In function ‘ppc6xx_set_irq’:
> hw/ppc/ppc.c:118:16: warning: this statement may fall through [-Wimplicit-fallthrough=]
>   118 |             if (level) {
>       |                ^
> hw/ppc/ppc.c:123:9: note: here
>   123 |         case PPC6xx_INPUT_INT:
>       |         ^~~~
> 
> Add the corresponding "fall through" comment to fix it.
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
> Cc: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/ppc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index 4a11fb1640..f9eb8f21b4 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -120,6 +120,7 @@ static void ppc6xx_set_irq(void *opaque, int pin, int level)
>              } else {
>                  cpu_ppc_tb_stop(env);
>              }
> +            /* fall through */
>          case PPC6xx_INPUT_INT:
>              /* Level sensitive - active high */
>              LOG_IRQ("%s: set the external IRQ state to %d\n",

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 7/9] ppc: silence the compiler warnings

[Thomas Huth]

On 28/10/2020 05.29, David Gibson wrote:
> On Wed, Oct 28, 2020 at 12:18:17PM +0800, Chen Qun wrote:
>> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
>> hw/ppc/ppc.c: In function ‘ppc6xx_set_irq’:
>> hw/ppc/ppc.c:118:16: warning: this statement may fall through [-Wimplicit-fallthrough=]
>>   118 |             if (level) {
>>       |                ^
>> hw/ppc/ppc.c:123:9: note: here
>>   123 |         case PPC6xx_INPUT_INT:
>>       |         ^~~~
>>
>> Add the corresponding "fall through" comment to fix it.
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> 
> Acked-by: David Gibson <david@gibson.dropbear.id.au>
> 
>> ---
>> Cc: David Gibson <david@gibson.dropbear.id.au>
>> ---
>>  hw/ppc/ppc.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
>> index 4a11fb1640..f9eb8f21b4 100644
>> --- a/hw/ppc/ppc.c
>> +++ b/hw/ppc/ppc.c
>> @@ -120,6 +120,7 @@ static void ppc6xx_set_irq(void *opaque, int pin, int level)
>>              } else {
>>                  cpu_ppc_tb_stop(env);
>>              }
>> +            /* fall through */
>>          case PPC6xx_INPUT_INT:
>>              /* Level sensitive - active high */
>>              LOG_IRQ("%s: set the external IRQ state to %d\n",
> 

Is that fall through actually really the right thing to do here? I'd rather
expect to see a PPC_INTERRUPT_DECR instead of a PPC_INTERRUPT_EXT in case
someone messes with the TBEN pin? So I assume this is likely rather bug and
we should a "break" statement here instead?

 Thomas

Re: [PATCH 7/9] ppc: silence the compiler warnings

[David Gibson]

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

On Wed, Oct 28, 2020 at 03:42:31PM +0100, Thomas Huth wrote:
> On 28/10/2020 05.29, David Gibson wrote:
> > On Wed, Oct 28, 2020 at 12:18:17PM +0800, Chen Qun wrote:
> >> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
> >> hw/ppc/ppc.c: In function ‘ppc6xx_set_irq’:
> >> hw/ppc/ppc.c:118:16: warning: this statement may fall through [-Wimplicit-fallthrough=]
> >>   118 |             if (level) {
> >>       |                ^
> >> hw/ppc/ppc.c:123:9: note: here
> >>   123 |         case PPC6xx_INPUT_INT:
> >>       |         ^~~~
> >>
> >> Add the corresponding "fall through" comment to fix it.
> >>
> >> Reported-by: Euler Robot <euler.robot@huawei.com>
> >> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> > 
> > Acked-by: David Gibson <david@gibson.dropbear.id.au>
> > 
> >> ---
> >> Cc: David Gibson <david@gibson.dropbear.id.au>
> >> ---
> >>  hw/ppc/ppc.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> >> index 4a11fb1640..f9eb8f21b4 100644
> >> --- a/hw/ppc/ppc.c
> >> +++ b/hw/ppc/ppc.c
> >> @@ -120,6 +120,7 @@ static void ppc6xx_set_irq(void *opaque, int pin, int level)
> >>              } else {
> >>                  cpu_ppc_tb_stop(env);
> >>              }
> >> +            /* fall through */
> >>          case PPC6xx_INPUT_INT:
> >>              /* Level sensitive - active high */
> >>              LOG_IRQ("%s: set the external IRQ state to %d\n",
> > 
> 
> Is that fall through actually really the right thing to do here? I'd rather
> expect to see a PPC_INTERRUPT_DECR instead of a PPC_INTERRUPT_EXT in case
> someone messes with the TBEN pin? So I assume this is likely rather bug and
> we should a "break" statement here instead?

Oh.. good catch, I think I misread this.  I thought the change was
correct, because DECRs look somewhat like external interrupts.  But
this is TBEN, not a DECR interrupt per se.  So, yes, I think this was
a bug and it should be a break instead.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 7/9] ppc: silence the compiler warnings

[Chenqun (kuhn)]

> -----Original Message-----
> From: David Gibson [mailto:david@gibson.dropbear.id.au]
> Sent: Thursday, October 29, 2020 7:39 AM
> To: Thomas Huth <thuth@redhat.com>
> Cc: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; qemu-trivial@nongnu.org;
> Euler Robot <euler.robot@huawei.com>; qemu-devel@nongnu.org; ganqixin
> <ganqixin@huawei.com>; Zhanghailiang <zhang.zhanghailiang@huawei.com>;
> qemu-ppc@nongnu.org
> Subject: Re: [PATCH 7/9] ppc: silence the compiler warnings
> 
> On Wed, Oct 28, 2020 at 03:42:31PM +0100, Thomas Huth wrote:
> > On 28/10/2020 05.29, David Gibson wrote:
> > > On Wed, Oct 28, 2020 at 12:18:17PM +0800, Chen Qun wrote:
> > >> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed
> warning:
> > >> hw/ppc/ppc.c: In function ‘ppc6xx_set_irq’:
> > >> hw/ppc/ppc.c:118:16: warning: this statement may fall through
> [-Wimplicit-fallthrough=]
> > >>   118 |             if (level) {
> > >>       |                ^
> > >> hw/ppc/ppc.c:123:9: note: here
> > >>   123 |         case PPC6xx_INPUT_INT:
> > >>       |         ^~~~
> > >>
> > >> Add the corresponding "fall through" comment to fix it.
> > >>
> > >> Reported-by: Euler Robot <euler.robot@huawei.com>
> > >> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> > >
> > > Acked-by: David Gibson <david@gibson.dropbear.id.au>
> > >
> > >> ---
> > >> Cc: David Gibson <david@gibson.dropbear.id.au>
> > >> ---
> > >>  hw/ppc/ppc.c | 1 +
> > >>  1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c index
> > >> 4a11fb1640..f9eb8f21b4 100644
> > >> --- a/hw/ppc/ppc.c
> > >> +++ b/hw/ppc/ppc.c
> > >> @@ -120,6 +120,7 @@ static void ppc6xx_set_irq(void *opaque, int pin,
> int level)
> > >>              } else {
> > >>                  cpu_ppc_tb_stop(env);
> > >>              }
> > >> +            /* fall through */
> > >>          case PPC6xx_INPUT_INT:
> > >>              /* Level sensitive - active high */
> > >>              LOG_IRQ("%s: set the external IRQ state to %d\n",
> > >
> >
> > Is that fall through actually really the right thing to do here? I'd
> > rather expect to see a PPC_INTERRUPT_DECR instead of a
> > PPC_INTERRUPT_EXT in case someone messes with the TBEN pin? So I
> > assume this is likely rather bug and we should a "break" statement here
> instead?
> 
> Oh.. good catch, I think I misread this.  I thought the change was correct,
> because DECRs look somewhat like external interrupts.  But this is TBEN, not
> a DECR interrupt per se.  So, yes, I think this was a bug and it should be a
> break instead.
> 
This bug looks like it's been hidden for years. Thanks for your point.
According to your opinion, I will modify it in the next version.

Thanks,
Chen Qun
> --
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_
> _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

[PATCH 8/9] target/ppc: silence the compiler warnings

[Chen Qun]

When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
target/ppc/mmu_helper.c: In function ‘dump_mmu’:
target/ppc/mmu_helper.c:1351:12: warning: this statement may fall through [-Wimplicit-fallthrough=]
 1351 |         if (ppc64_v3_radix(env_archcpu(env))) {
      |            ^
target/ppc/mmu_helper.c:1358:5: note: here
 1358 |     default:
      |     ^~~~~~~

Add the corresponding "fall through" comment to fix it.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: David Gibson <david@gibson.dropbear.id.au>
---
 target/ppc/mmu_helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
index 8972714775..51749b62df 100644
--- a/target/ppc/mmu_helper.c
+++ b/target/ppc/mmu_helper.c
@@ -1355,6 +1355,7 @@ void dump_mmu(CPUPPCState *env)
             break;
         }
 #endif
+        /* fall through */
     default:
         qemu_log_mask(LOG_UNIMP, "%s: unimplemented\n", __func__);
     }
-- 
2.27.0

Re: [PATCH 8/9] target/ppc: silence the compiler warnings

[David Gibson]

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

On Wed, Oct 28, 2020 at 12:18:18PM +0800, Chen Qun wrote:
> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
> target/ppc/mmu_helper.c: In function ‘dump_mmu’:
> target/ppc/mmu_helper.c:1351:12: warning: this statement may fall through [-Wimplicit-fallthrough=]
>  1351 |         if (ppc64_v3_radix(env_archcpu(env))) {
>       |            ^
> target/ppc/mmu_helper.c:1358:5: note: here
>  1358 |     default:
>       |     ^~~~~~~
> 
> Add the corresponding "fall through" comment to fix it.
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
> Cc: David Gibson <david@gibson.dropbear.id.au>
> ---
>  target/ppc/mmu_helper.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> index 8972714775..51749b62df 100644
> --- a/target/ppc/mmu_helper.c
> +++ b/target/ppc/mmu_helper.c
> @@ -1355,6 +1355,7 @@ void dump_mmu(CPUPPCState *env)
>              break;
>          }
>  #endif
> +        /* fall through */
>      default:
>          qemu_log_mask(LOG_UNIMP, "%s: unimplemented\n", __func__);
>      }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 8/9] target/ppc: silence the compiler warnings

[Philippe Mathieu-Daudé]

On 10/28/20 5:18 AM, Chen Qun wrote:
> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
> target/ppc/mmu_helper.c: In function ‘dump_mmu’:
> target/ppc/mmu_helper.c:1351:12: warning: this statement may fall through [-Wimplicit-fallthrough=]
>  1351 |         if (ppc64_v3_radix(env_archcpu(env))) {
>       |            ^
> target/ppc/mmu_helper.c:1358:5: note: here
>  1358 |     default:
>       |     ^~~~~~~
> 
> Add the corresponding "fall through" comment to fix it.
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> Cc: David Gibson <david@gibson.dropbear.id.au>
> ---
>  target/ppc/mmu_helper.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> index 8972714775..51749b62df 100644
> --- a/target/ppc/mmu_helper.c
> +++ b/target/ppc/mmu_helper.c
> @@ -1355,6 +1355,7 @@ void dump_mmu(CPUPPCState *env)
>              break;
>          }
>  #endif
> +        /* fall through */

I'm surprise the compiler emit a warning for missing comment,
but don't emit one for superfluous and confusing ones (when
building a ppc32-only target). You'd need to put this before
the #endif.

But instead of this band-aid to silent warning, replace the
TODO by a LOG_UNIMP call, and add a break before the #endif.

>      default:
>          qemu_log_mask(LOG_UNIMP, "%s: unimplemented\n", __func__);
>      }
> 

Re: [PATCH 8/9] target/ppc: silence the compiler warnings

[Thomas Huth]

On 28/10/2020 10.56, Philippe Mathieu-Daudé wrote:
> On 10/28/20 5:18 AM, Chen Qun wrote:
>> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
>> target/ppc/mmu_helper.c: In function ‘dump_mmu’:
>> target/ppc/mmu_helper.c:1351:12: warning: this statement may fall through [-Wimplicit-fallthrough=]
>>  1351 |         if (ppc64_v3_radix(env_archcpu(env))) {
>>       |            ^
>> target/ppc/mmu_helper.c:1358:5: note: here
>>  1358 |     default:
>>       |     ^~~~~~~
>>
>> Add the corresponding "fall through" comment to fix it.
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>> ---
>> Cc: David Gibson <david@gibson.dropbear.id.au>
>> ---
>>  target/ppc/mmu_helper.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
>> index 8972714775..51749b62df 100644
>> --- a/target/ppc/mmu_helper.c
>> +++ b/target/ppc/mmu_helper.c
>> @@ -1355,6 +1355,7 @@ void dump_mmu(CPUPPCState *env)
>>              break;
>>          }
>>  #endif
>> +        /* fall through */
> 
> I'm surprise the compiler emit a warning for missing comment,
> but don't emit one for superfluous and confusing ones (when
> building a ppc32-only target). You'd need to put this before
> the #endif.
> 
> But instead of this band-aid to silent warning, replace the
> TODO by a LOG_UNIMP call, and add a break before the #endif.

+1 for replacing the TODO with a LOG_UNIMP call and adding a break instead,
that would look way less messy than the current code.

 Thanks,
  Thomas

Re: [PATCH 8/9] target/ppc: silence the compiler warnings

[David Gibson]

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

On Wed, Oct 28, 2020 at 04:06:03PM +0100, Thomas Huth wrote:
> On 28/10/2020 10.56, Philippe Mathieu-Daudé wrote:
> > On 10/28/20 5:18 AM, Chen Qun wrote:
> >> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
> >> target/ppc/mmu_helper.c: In function ‘dump_mmu’:
> >> target/ppc/mmu_helper.c:1351:12: warning: this statement may fall through [-Wimplicit-fallthrough=]
> >>  1351 |         if (ppc64_v3_radix(env_archcpu(env))) {
> >>       |            ^
> >> target/ppc/mmu_helper.c:1358:5: note: here
> >>  1358 |     default:
> >>       |     ^~~~~~~
> >>
> >> Add the corresponding "fall through" comment to fix it.
> >>
> >> Reported-by: Euler Robot <euler.robot@huawei.com>
> >> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> >> ---
> >> Cc: David Gibson <david@gibson.dropbear.id.au>
> >> ---
> >>  target/ppc/mmu_helper.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> >> index 8972714775..51749b62df 100644
> >> --- a/target/ppc/mmu_helper.c
> >> +++ b/target/ppc/mmu_helper.c
> >> @@ -1355,6 +1355,7 @@ void dump_mmu(CPUPPCState *env)
> >>              break;
> >>          }
> >>  #endif
> >> +        /* fall through */
> > 
> > I'm surprise the compiler emit a warning for missing comment,
> > but don't emit one for superfluous and confusing ones (when
> > building a ppc32-only target). You'd need to put this before
> > the #endif.
> > 
> > But instead of this band-aid to silent warning, replace the
> > TODO by a LOG_UNIMP call, and add a break before the #endif.
> 
> +1 for replacing the TODO with a LOG_UNIMP call and adding a break instead,
> that would look way less messy than the current code.

True, that would be a better approach.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 8/9] target/ppc: silence the compiler warnings

[Chenqun (kuhn)]

> -----Original Message-----
> From: David Gibson [mailto:david@gibson.dropbear.id.au]
> Sent: Thursday, October 29, 2020 7:39 AM
> To: Thomas Huth <thuth@redhat.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>; Chenqun (kuhn)
> <kuhn.chenqun@huawei.com>; qemu-devel@nongnu.org;
> qemu-trivial@nongnu.org; Euler Robot <euler.robot@huawei.com>;
> Zhanghailiang <zhang.zhanghailiang@huawei.com>; ganqixin
> <ganqixin@huawei.com>
> Subject: Re: [PATCH 8/9] target/ppc: silence the compiler warnings
> 
> On Wed, Oct 28, 2020 at 04:06:03PM +0100, Thomas Huth wrote:
> > On 28/10/2020 10.56, Philippe Mathieu-Daudé wrote:
> > > On 10/28/20 5:18 AM, Chen Qun wrote:
> > >> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed
> warning:
> > >> target/ppc/mmu_helper.c: In function ‘dump_mmu’:
> > >> target/ppc/mmu_helper.c:1351:12: warning: this statement may fall
> through [-Wimplicit-fallthrough=]
> > >>  1351 |         if (ppc64_v3_radix(env_archcpu(env))) {
> > >>       |            ^
> > >> target/ppc/mmu_helper.c:1358:5: note: here
> > >>  1358 |     default:
> > >>       |     ^~~~~~~
> > >>
> > >> Add the corresponding "fall through" comment to fix it.
> > >>
> > >> Reported-by: Euler Robot <euler.robot@huawei.com>
> > >> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> > >> ---
> > >> Cc: David Gibson <david@gibson.dropbear.id.au>
> > >> ---
> > >>  target/ppc/mmu_helper.c | 1 +
> > >>  1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> > >> index 8972714775..51749b62df 100644
> > >> --- a/target/ppc/mmu_helper.c
> > >> +++ b/target/ppc/mmu_helper.c
> > >> @@ -1355,6 +1355,7 @@ void dump_mmu(CPUPPCState *env)
> > >>              break;
> > >>          }
> > >>  #endif
> > >> +        /* fall through */
> > >
> > > I'm surprise the compiler emit a warning for missing comment, but
> > > don't emit one for superfluous and confusing ones (when building a
> > > ppc32-only target). You'd need to put this before the #endif.
> > >
> > > But instead of this band-aid to silent warning, replace the TODO by
> > > a LOG_UNIMP call, and add a break before the #endif.
> >
> > +1 for replacing the TODO with a LOG_UNIMP call and adding a break
> > +instead,
> > that would look way less messy than the current code.
> 
> True, that would be a better approach.

Yeah, that's a good idea.  
I'll modify it in the v2.

Thanks,
Chen Qun
> 
> --
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_
> _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson

[PATCH 9/9] hw/timer/renesas_tmr: silence the compiler warnings

[Chen Qun]

When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
../hw/timer/renesas_tmr.c: In function ‘tmr_read’:
../hw/timer/renesas_tmr.c:221:19: warning: this statement may fall through [-Wimplicit-fallthrough=]
  221 |         } else if (ch == 0) {i
      |                   ^
../hw/timer/renesas_tmr.c:224:5: note: here
  224 |     case A_TCORB:
      |     ^~~~

Add the corresponding "fall through" comment to fix it.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
Cc: Magnus Damm <magnus.damm@gmail.com>
---
 hw/timer/renesas_tmr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/timer/renesas_tmr.c b/hw/timer/renesas_tmr.c
index 446f2eacdd..e03a8155b2 100644
--- a/hw/timer/renesas_tmr.c
+++ b/hw/timer/renesas_tmr.c
@@ -221,6 +221,7 @@ static uint64_t tmr_read(void *opaque, hwaddr addr, unsigned size)
         } else if (ch == 0) {
             return concat_reg(tmr->tcora);
         }
+        /* fall through */
     case A_TCORB:
         if (size == 1) {
             return tmr->tcorb[ch];
-- 
2.27.0

Re: [PATCH 9/9] hw/timer/renesas_tmr: silence the compiler warnings

[Philippe Mathieu-Daudé]

On 10/28/20 5:18 AM, Chen Qun wrote:
> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
> ../hw/timer/renesas_tmr.c: In function ‘tmr_read’:
> ../hw/timer/renesas_tmr.c:221:19: warning: this statement may fall through [-Wimplicit-fallthrough=]
>   221 |         } else if (ch == 0) {i
>       |                   ^
> ../hw/timer/renesas_tmr.c:224:5: note: here
>   224 |     case A_TCORB:
>       |     ^~~~
> 
> Add the corresponding "fall through" comment to fix it.
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> ---
>  hw/timer/renesas_tmr.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/timer/renesas_tmr.c b/hw/timer/renesas_tmr.c
> index 446f2eacdd..e03a8155b2 100644
> --- a/hw/timer/renesas_tmr.c
> +++ b/hw/timer/renesas_tmr.c
> @@ -221,6 +221,7 @@ static uint64_t tmr_read(void *opaque, hwaddr addr, unsigned size)
>          } else if (ch == 0) {
>              return concat_reg(tmr->tcora);
>          }
> +        /* fall through */
>      case A_TCORB:
>          if (size == 1) {
>              return tmr->tcorb[ch];
> 

You fixed A_TCORA but not A_TCORB...

How the Euler Robot works? Like Coverity?

Re: [PATCH 9/9] hw/timer/renesas_tmr: silence the compiler warnings

[Thomas Huth]

On 28/10/2020 10.59, Philippe Mathieu-Daudé wrote:
> On 10/28/20 5:18 AM, Chen Qun wrote:
>> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
>> ../hw/timer/renesas_tmr.c: In function ‘tmr_read’:
>> ../hw/timer/renesas_tmr.c:221:19: warning: this statement may fall through [-Wimplicit-fallthrough=]
>>   221 |         } else if (ch == 0) {i
>>       |                   ^
>> ../hw/timer/renesas_tmr.c:224:5: note: here
>>   224 |     case A_TCORB:
>>       |     ^~~~
>>
>> Add the corresponding "fall through" comment to fix it.
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>> ---
>> Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
>> Cc: Magnus Damm <magnus.damm@gmail.com>
>> ---
>>  hw/timer/renesas_tmr.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/timer/renesas_tmr.c b/hw/timer/renesas_tmr.c
>> index 446f2eacdd..e03a8155b2 100644
>> --- a/hw/timer/renesas_tmr.c
>> +++ b/hw/timer/renesas_tmr.c
>> @@ -221,6 +221,7 @@ static uint64_t tmr_read(void *opaque, hwaddr addr, unsigned size)
>>          } else if (ch == 0) {
>>              return concat_reg(tmr->tcora);
>>          }
>> +        /* fall through */
>>      case A_TCORB:
>>          if (size == 1) {
>>              return tmr->tcorb[ch];
>>
> 
> You fixed A_TCORA but not A_TCORB...

A_TCORB cannot fall through, since it always does a "return" in both
branches of the if-statement.

However, it also looks really odd that A_TCORA falls through to A_TCORB here
and return that register value instead. Is this really intended? Yoshinori,
could you please double-check whether this is a bug here, or intended?

 Thanks,
  Thomas

Re: [PATCH 9/9] hw/timer/renesas_tmr: silence the compiler warnings

[Peter Maydell]

On Wed, 28 Oct 2020 at 15:07, Thomas Huth <thuth@redhat.com> wrote:
>
> On 28/10/2020 10.59, Philippe Mathieu-Daudé wrote:
> > On 10/28/20 5:18 AM, Chen Qun wrote:
> >> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
> >> ../hw/timer/renesas_tmr.c: In function ‘tmr_read’:
> >> ../hw/timer/renesas_tmr.c:221:19: warning: this statement may fall through [-Wimplicit-fallthrough=]
> >>   221 |         } else if (ch == 0) {i
> >>       |                   ^
> >> ../hw/timer/renesas_tmr.c:224:5: note: here
> >>   224 |     case A_TCORB:
> >>       |     ^~~~
> >>
> >> Add the corresponding "fall through" comment to fix it.
> >>
> >> Reported-by: Euler Robot <euler.robot@huawei.com>
> >> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> >> ---
> >> Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
> >> Cc: Magnus Damm <magnus.damm@gmail.com>
> >> ---
> >>  hw/timer/renesas_tmr.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/hw/timer/renesas_tmr.c b/hw/timer/renesas_tmr.c
> >> index 446f2eacdd..e03a8155b2 100644
> >> --- a/hw/timer/renesas_tmr.c
> >> +++ b/hw/timer/renesas_tmr.c
> >> @@ -221,6 +221,7 @@ static uint64_t tmr_read(void *opaque, hwaddr addr, unsigned size)
> >>          } else if (ch == 0) {
> >>              return concat_reg(tmr->tcora);
> >>          }
> >> +        /* fall through */
> >>      case A_TCORB:
> >>          if (size == 1) {
> >>              return tmr->tcorb[ch];
> >>
> >
> > You fixed A_TCORA but not A_TCORB...
>
> A_TCORB cannot fall through, since it always does a "return" in both
> branches of the if-statement.
>
> However, it also looks really odd that A_TCORA falls through to A_TCORB here
> and return that register value instead. Is this really intended? Yoshinori,
> could you please double-check whether this is a bug here, or intended?

See the discussion in this thread:
https://lore.kernel.org/qemu-devel/CAFEAcA8c2dywr=Zxz1ExAV-48JwFU5vbBDDfA=_KE98XamnXiA@mail.gmail.com/
from when Coverity spotting the fall-through.

I think this cleanup patch from Yoshinori deals with the fall-through
stuff by cleaning up that area of the timer device:
https://lore.kernel.org/qemu-devel/20200711154242.41222-1-ysato@users.sourceforge.jp/
It just needs somebody to code-review it :-)

thanks
-- PMM

RE: [PATCH 9/9] hw/timer/renesas_tmr: silence the compiler warnings

[Chenqun (kuhn)]

> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Thursday, October 29, 2020 4:15 AM
> To: Thomas Huth <thuth@redhat.com>
> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>; Chenqun (kuhn)
> <kuhn.chenqun@huawei.com>; QEMU Developers <qemu-devel@nongnu.org>;
> QEMU Trivial <qemu-trivial@nongnu.org>; Yoshinori Sato
> <ysato@users.sourceforge.jp>; Magnus Damm <magnus.damm@gmail.com>;
> Zhanghailiang <zhang.zhanghailiang@huawei.com>; ganqixin
> <ganqixin@huawei.com>; Euler Robot <euler.robot@huawei.com>
> Subject: Re: [PATCH 9/9] hw/timer/renesas_tmr: silence the compiler warnings
> 
> On Wed, 28 Oct 2020 at 15:07, Thomas Huth <thuth@redhat.com> wrote:
> >
> > On 28/10/2020 10.59, Philippe Mathieu-Daudé wrote:
> > > On 10/28/20 5:18 AM, Chen Qun wrote:
> > >> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed
> warning:
> > >> ../hw/timer/renesas_tmr.c: In function ‘tmr_read’:
> > >> ../hw/timer/renesas_tmr.c:221:19: warning: this statement may fall
> through [-Wimplicit-fallthrough=]
> > >>   221 |         } else if (ch == 0) {i
> > >>       |                   ^
> > >> ../hw/timer/renesas_tmr.c:224:5: note: here
> > >>   224 |     case A_TCORB:
> > >>       |     ^~~~
> > >>
> > >> Add the corresponding "fall through" comment to fix it.
> > >>
> > >> Reported-by: Euler Robot <euler.robot@huawei.com>
> > >> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> > >> ---
> > >> Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
> > >> Cc: Magnus Damm <magnus.damm@gmail.com>
> > >> ---
> > >>  hw/timer/renesas_tmr.c | 1 +
> > >>  1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/hw/timer/renesas_tmr.c b/hw/timer/renesas_tmr.c index
> > >> 446f2eacdd..e03a8155b2 100644
> > >> --- a/hw/timer/renesas_tmr.c
> > >> +++ b/hw/timer/renesas_tmr.c
> > >> @@ -221,6 +221,7 @@ static uint64_t tmr_read(void *opaque, hwaddr
> addr, unsigned size)
> > >>          } else if (ch == 0) {
> > >>              return concat_reg(tmr->tcora);
> > >>          }
> > >> +        /* fall through */
> > >>      case A_TCORB:
> > >>          if (size == 1) {
> > >>              return tmr->tcorb[ch];
> > >>
> > >
> > > You fixed A_TCORA but not A_TCORB...
> >
> > A_TCORB cannot fall through, since it always does a "return" in both
> > branches of the if-statement.
> >
> > However, it also looks really odd that A_TCORA falls through to
> > A_TCORB here and return that register value instead. Is this really
> > intended? Yoshinori, could you please double-check whether this is a bug here,
> or intended?
> 
> See the discussion in this thread:
> https://lore.kernel.org/qemu-devel/CAFEAcA8c2dywr=Zxz1ExAV-48JwFU5vbBD
> DfA=_KE98XamnXiA@mail.gmail.com/
> from when Coverity spotting the fall-through.
> 
> I think this cleanup patch from Yoshinori deals with the fall-through stuff by
> cleaning up that area of the timer device:
> https://lore.kernel.org/qemu-devel/20200711154242.41222-1-ysato@users.so
> urceforge.jp/
> It just needs somebody to code-review it :-)

This cleanup patch has been modified more thoroughly, so let's wait for it merge :-)

Thanks,
Chen Qun

RE: [PATCH 9/9] hw/timer/renesas_tmr: silence the compiler warnings

[Chenqun (kuhn)]

> -----Original Message-----
> From: Philippe Mathieu-Daudé [mailto:philippe.mathieu.daude@gmail.com]
> On Behalf Of Philippe Mathieu-Daudé
> Sent: Wednesday, October 28, 2020 6:00 PM
> To: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; qemu-devel@nongnu.org;
> qemu-trivial@nongnu.org
> Cc: Zhanghailiang <zhang.zhanghailiang@huawei.com>; Yoshinori Sato
> <ysato@users.sourceforge.jp>; Magnus Damm <magnus.damm@gmail.com>;
> ganqixin <ganqixin@huawei.com>; Euler Robot <euler.robot@huawei.com>
> Subject: Re: [PATCH 9/9] hw/timer/renesas_tmr: silence the compiler warnings
> 
> On 10/28/20 5:18 AM, Chen Qun wrote:
> > When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed
> warning:
> > ../hw/timer/renesas_tmr.c: In function ‘tmr_read’:
> > ../hw/timer/renesas_tmr.c:221:19: warning: this statement may fall through
> [-Wimplicit-fallthrough=]
> >   221 |         } else if (ch == 0) {i
> >       |                   ^
> > ../hw/timer/renesas_tmr.c:224:5: note: here
> >   224 |     case A_TCORB:
> >       |     ^~~~
> >
> > Add the corresponding "fall through" comment to fix it.
> >
> > Reported-by: Euler Robot <euler.robot@huawei.com>
> > Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> > ---
> > Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
> > Cc: Magnus Damm <magnus.damm@gmail.com>
> > ---
> >  hw/timer/renesas_tmr.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/timer/renesas_tmr.c b/hw/timer/renesas_tmr.c index
> > 446f2eacdd..e03a8155b2 100644
> > --- a/hw/timer/renesas_tmr.c
> > +++ b/hw/timer/renesas_tmr.c
> > @@ -221,6 +221,7 @@ static uint64_t tmr_read(void *opaque, hwaddr addr,
> unsigned size)
> >          } else if (ch == 0) {
> >              return concat_reg(tmr->tcora);
> >          }
> > +        /* fall through */
> >      case A_TCORB:
> >          if (size == 1) {
> >              return tmr->tcorb[ch];
> >
> 
> You fixed A_TCORA but not A_TCORB...
> 
Hi Philippe,
 My first feeling is the same as you when this warning found.
But, the number of branches for A_TCORA and A_TCORB is different.

In A_TCORA case："} else if (ch == 0) {"
In A_TCORB case："} else {"

Obviously, A_TCOB have all return values. But A_TCOA is not, it need to fall through or break.

> How the Euler Robot works? Like Coverity?

No, unlike Coverity, Coverity is essentially a good discovery bug tool.
But EulerRobot is a virtualization software quality automation project that integrates some tools and test suites such as gcc/clang make test, qemu ut, qtest, coccinelle scripts and avocado-vt.

Thanks,
Chen Qun