[Qemu-devel] [RFC PATCH-for-4.1] target/i386: Correct misplaced break statement in gen_shiftd_rm_T1()

[Qemu-devel] [RFC PATCH-for-4.1] target/i386: Correct misplaced break statement in gen_shiftd_rm_T1()

[Philippe Mathieu-Daudé]

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é <philmd@redhat.com>
---
 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

Re: [Qemu-devel] [RFC PATCH-for-4.1] target/i386: Correct misplaced break statement in gen_shiftd_rm_T1()

[Paolo Bonzini]

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é <philmd@redhat.com>
> ---
>  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

Re: [Qemu-devel] [RFC PATCH-for-4.1] target/i386: Correct misplaced break statement in gen_shiftd_rm_T1()

[Peter Maydell]

On Fri, 19 Jul 2019 at 12:45, Paolo Bonzini <pbonzini@redhat.com> 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é <philmd@redhat.com>
> > ---
> >  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()

[Paolo Bonzini]

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