diff mbox series

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

Message ID 20201028041819.2169003-2-kuhn.chenqun@huawei.com
State New
Headers show
Series silence the compiler warnings | expand

Commit Message

Chenqun (kuhn) Oct. 28, 2020, 4:18 a.m. UTC
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(-)

Comments

Thomas Huth Oct. 28, 2020, 12:57 p.m. UTC | #1
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
Philippe Mathieu-Daudé Oct. 28, 2020, 1:20 p.m. UTC | #2
+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
Richard Henderson Oct. 28, 2020, 3:31 p.m. UTC | #3
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~
Richard Henderson Oct. 28, 2020, 3:31 p.m. UTC | #4
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~
Thomas Huth Oct. 28, 2020, 4:51 p.m. UTC | #5
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>
Chenqun (kuhn) Oct. 29, 2020, 2:40 a.m. UTC | #6
> -----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
diff mbox series

Patch

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