Re: [Qemu-devel] [RFC PATCH-for-4.1] target/i386: Correct misplaced break statement in gen_shiftd_rm_T1()
On 19/07/19 13:51, Peter Maydell wrote: >> I haven't looked closely at the code but I would guess that the >> fallthrough is intended, because the default label has an "ot == MO_16" >> condition. > Yeah, this code is really weird -- if TARGET_X86_64 then > MO_16 falls through into MO_32, but if only i386 then > MO_16 falls through into the default case ?!? Yes, and in either case MO_16 falls through into the 32-bit code. However, the i386 32-bit version and the x86-64 64-bit version are unified into a single piece of code for TARGET_LONG_BITS-bit operands. Almost, because you still need that ugly special case for MO_16 in the default label. Paolo
Re: [Qemu-devel] [RFC PATCH-for-4.1] target/i386: Correct misplaced break statement in gen_shiftd_rm_T1()
On Fri, 19 Jul 2019 at 12:45, Paolo Bonzini wrote:
>
> On 19/07/19 13:23, Philippe Mathieu-Daudé wrote:
> > Reported by GCC9 when building with CFLAG -Wimplicit-fallthrough=2:
> >
> > CC target/i386/translate.o
> > target/i386/translate.c: In function ‘gen_shiftd_rm_T1’:
> > target/i386/translate.c:1785:12: error: this statement may fall through
> > [-Werror=implicit-fallthrough=]
> >1785 | if (is_right) {
> > |^
> > target/i386/translate.c:1810:5: note: here
> >1810 | default:
> > | ^~~
> > cc1: all warnings being treated as errors
> >
> > Fixes: f437d0a3c24
> > Signed-off-by: Philippe Mathieu-Daudé
> > ---
> > target/i386/translate.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/i386/translate.c b/target/i386/translate.c
> > index 03150a86e2..4b2b5937ca 100644
> > --- a/target/i386/translate.c
> > +++ b/target/i386/translate.c
> > @@ -1805,8 +1805,8 @@ static void gen_shiftd_rm_T1(DisasContext *s,
> > TCGMemOp ot, int op1,
> > tcg_gen_shri_i64(s->tmp0, s->tmp0, 32);
> > tcg_gen_shri_i64(s->T0, s->T0, 32);
> > }
> > -break;
> > #endif
> > +break;
> > default:
> > tcg_gen_subi_tl(s->tmp0, count, 1);
> > if (is_right) {
> >
>
> I haven't looked closely at the code but I would guess that the
> fallthrough is intended, because the default label has an "ot == MO_16"
> condition.
Yeah, this code is really weird -- if TARGET_X86_64 then
MO_16 falls through into MO_32, but if only i386 then
MO_16 falls through into the default case ?!?
thanks
-- PMM
Re: [Qemu-devel] [RFC PATCH-for-4.1] target/i386: Correct misplaced break statement in gen_shiftd_rm_T1()
On 19/07/19 13:23, Philippe Mathieu-Daudé wrote:
> Reported by GCC9 when building with CFLAG -Wimplicit-fallthrough=2:
>
> CC target/i386/translate.o
> target/i386/translate.c: In function ‘gen_shiftd_rm_T1’:
> target/i386/translate.c:1785:12: error: this statement may fall through
> [-Werror=implicit-fallthrough=]
>1785 | if (is_right) {
> |^
> target/i386/translate.c:1810:5: note: here
>1810 | default:
> | ^~~
> cc1: all warnings being treated as errors
>
> Fixes: f437d0a3c24
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> target/i386/translate.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/i386/translate.c b/target/i386/translate.c
> index 03150a86e2..4b2b5937ca 100644
> --- a/target/i386/translate.c
> +++ b/target/i386/translate.c
> @@ -1805,8 +1805,8 @@ static void gen_shiftd_rm_T1(DisasContext *s, TCGMemOp
> ot, int op1,
> tcg_gen_shri_i64(s->tmp0, s->tmp0, 32);
> tcg_gen_shri_i64(s->T0, s->T0, 32);
> }
> -break;
> #endif
> +break;
> default:
> tcg_gen_subi_tl(s->tmp0, count, 1);
> if (is_right) {
>
I haven't looked closely at the code but I would guess that the
fallthrough is intended, because the default label has an "ot == MO_16"
condition.
It certainly needs more comments... :(
Paolo
[Qemu-devel] [RFC PATCH-for-4.1] target/i386: Correct misplaced break statement in gen_shiftd_rm_T1()
Reported by GCC9 when building with CFLAG -Wimplicit-fallthrough=2:
CC target/i386/translate.o
target/i386/translate.c: In function ‘gen_shiftd_rm_T1’:
target/i386/translate.c:1785:12: error: this statement may fall through
[-Werror=implicit-fallthrough=]
1785 | if (is_right) {
|^
target/i386/translate.c:1810:5: note: here
1810 | default:
| ^~~
cc1: all warnings being treated as errors
Fixes: f437d0a3c24
Signed-off-by: Philippe Mathieu-Daudé
---
target/i386/translate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/i386/translate.c b/target/i386/translate.c
index 03150a86e2..4b2b5937ca 100644
--- a/target/i386/translate.c
+++ b/target/i386/translate.c
@@ -1805,8 +1805,8 @@ static void gen_shiftd_rm_T1(DisasContext *s, TCGMemOp
ot, int op1,
tcg_gen_shri_i64(s->tmp0, s->tmp0, 32);
tcg_gen_shri_i64(s->T0, s->T0, 32);
}
-break;
#endif
+break;
default:
tcg_gen_subi_tl(s->tmp0, count, 1);
if (is_right) {
--
2.20.1
