| Message ID | 20201028041819.2169003-2-kuhn.chenqun@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | silence the compiler warnings | expand |
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
+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
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~
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~
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>
> -----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 --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 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(-)