> -----Original Message----- > From: Thomas Huth [mailto:th...@redhat.com] > Sent: Thursday, October 29, 2020 12:52 AM > To: Richard Henderson <richard.hender...@linaro.org>; Chenqun (kuhn) > <kuhn.chen...@huawei.com>; qemu-devel@nongnu.org; > qemu-triv...@nongnu.org > Cc: Eduardo Habkost <ehabk...@redhat.com>; Zhanghailiang > <zhang.zhanghaili...@huawei.com>; ganqixin <ganqi...@huawei.com>; Euler > Robot <euler.ro...@huawei.com>; Paolo Bonzini <pbonz...@redhat.com>; > Richard Henderson <r...@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.ro...@huawei.com> > >>> Signed-off-by: Chen Qun <kuhn.chen...@huawei.com> > >>> --- > >>> Cc: Paolo Bonzini <pbonz...@redhat.com> > >>> Cc: Richard Henderson <richard.hender...@linaro.org> > >>> Cc: Eduardo Habkost <ehabk...@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 <th...@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
