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