diff mbox series

[02/23] target/i386: fix gen_prepare_size_nz condition

Message ID 20240628124258.832466-3-alex.bennee@linaro.org (mailing list archive)
State New, archived
Headers show
Series July maintainer updates (32bit, testing, plugins, gdbstub) | expand

Commit Message

Alex Bennée June 28, 2024, 12:42 p.m. UTC 
Incorrect brace positions causes an unintended overflow on 32 bit
builds and shenanigans result.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2413
Suggested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/i386/tcg/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Alex Bennée June 28, 2024, 2:34 p.m. UTC | #1 
Alex Bennée <alex.bennee@linaro.org> writes:

> Incorrect brace positions causes an unintended overflow on 32 bit
> builds and shenanigans result.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2413
> Suggested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

This seems to trigger regressions in:

  qtest-x86_64/bios-tables-test
  qtest-x86_64/pxe-test
  qtest-x86_64/vmgenid-test

Could that be down to generated test data?

> ---
>  target/i386/tcg/translate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> index ad1819815a..94f13541c3 100644
> --- a/target/i386/tcg/translate.c
> +++ b/target/i386/tcg/translate.c
> @@ -877,7 +877,7 @@ static CCPrepare gen_prepare_sign_nz(TCGv src, MemOp size)
>          return (CCPrepare) { .cond = TCG_COND_LT, .reg = src };
>      } else {
>          return (CCPrepare) { .cond = TCG_COND_TSTNE, .reg = src,
> -                             .imm = 1ull << ((8 << size) - 1) };
> +                             .imm = (1ull << (8 << size)) - 1 };
>      }
>  }
Richard Henderson June 28, 2024, 5:54 p.m. UTC | #2 
On 6/28/24 05:42, Alex Bennée wrote:
> Incorrect brace positions causes an unintended overflow on 32 bit
> builds and shenanigans result.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2413
> Suggested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   target/i386/tcg/translate.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> index ad1819815a..94f13541c3 100644
> --- a/target/i386/tcg/translate.c
> +++ b/target/i386/tcg/translate.c
> @@ -877,7 +877,7 @@ static CCPrepare gen_prepare_sign_nz(TCGv src, MemOp size)
>           return (CCPrepare) { .cond = TCG_COND_LT, .reg = src };
>       } else {
>           return (CCPrepare) { .cond = TCG_COND_TSTNE, .reg = src,
> -                             .imm = 1ull << ((8 << size) - 1) };
> +                             .imm = (1ull << (8 << size)) - 1 };

This is incorrect -- we want only to test the sign bit.
Perhaps MAKE_64BIT_MASK((8 << size) - 1, 1) would make this more explicit?

I'll have a quick look at the issue and see if I can reproduce.


r~
Richard Henderson June 28, 2024, 10:35 p.m. UTC | #3 
On 6/28/24 10:54, Richard Henderson wrote:
> On 6/28/24 05:42, Alex Bennée wrote:
>> Incorrect brace positions causes an unintended overflow on 32 bit
>> builds and shenanigans result.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2413
>> Suggested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>   target/i386/tcg/translate.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
>> index ad1819815a..94f13541c3 100644
>> --- a/target/i386/tcg/translate.c
>> +++ b/target/i386/tcg/translate.c
>> @@ -877,7 +877,7 @@ static CCPrepare gen_prepare_sign_nz(TCGv src, MemOp size)
>>           return (CCPrepare) { .cond = TCG_COND_LT, .reg = src };
>>       } else {
>>           return (CCPrepare) { .cond = TCG_COND_TSTNE, .reg = src,
>> -                             .imm = 1ull << ((8 << size) - 1) };
>> +                             .imm = (1ull << (8 << size)) - 1 };
> 
> This is incorrect -- we want only to test the sign bit.
> Perhaps MAKE_64BIT_MASK((8 << size) - 1, 1) would make this more explicit?
> 
> I'll have a quick look at the issue and see if I can reproduce.

I can't get the cdrom test to run at all; I have no idea why.

1/1 qemu:qtest+qtest-x86_64 / qtest-x86_64/cdrom-test        SKIP            0.00s

But

QTEST_QEMU_BINARY='./qemu-system-x86_64' ./tests/qtest/bios-tables-test -v -p 
/x86_64/acpi/q35/mmio64

fails for me, and is resolved at 15957eb9e by reverting

diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 4735f084d4..022469845e 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -1084,13 +1084,8 @@ static CCPrepare gen_prepare_eflags_z(DisasContext *s, TCGv reg)
      default:
          {
              MemOp size = (s->cc_op - CC_OP_ADDB) & 3;
-            if (size == MO_TL) {
-                return (CCPrepare) { .cond = TCG_COND_EQ, .reg = cpu_cc_dst,
-                                     .mask = -1 };
-            } else {
-                return (CCPrepare) { .cond = TCG_COND_TSTEQ, .reg = cpu_cc_dst,
-                                     .mask = -1, .imm = (1ull << (8 << size)) - 1 };
-            }
+            TCGv t0 = gen_ext_tl(reg, cpu_cc_dst, size, false);
+            return (CCPrepare) { .cond = TCG_COND_EQ, .reg = t0, .mask = -1 };
          }
      }
  }

I fought all afternoon to try and debug this, but was defeated by qtest.
I really wish we could change our tooling to simplify debugging.


r~
Igor Mammedov July 1, 2024, 9:01 a.m. UTC | #4 
On Fri, 28 Jun 2024 15:34:58 +0100
Alex Bennée <alex.bennee@linaro.org> wrote:

> Alex Bennée <alex.bennee@linaro.org> writes:
> 
> > Incorrect brace positions causes an unintended overflow on 32 bit
> > builds and shenanigans result.
> >
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2413
> > Suggested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>  
> 
> This seems to trigger regressions in:
> 
>   qtest-x86_64/bios-tables-test
>   qtest-x86_64/pxe-test
>   qtest-x86_64/vmgenid-test
> 
> Could that be down to generated test data?

Without context, I'd guess, that
guest doesn't boot/get to randevu point that tests are waiting for
and then it just timeouts => fails.

> 
> > ---
> >  target/i386/tcg/translate.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
> > index ad1819815a..94f13541c3 100644
> > --- a/target/i386/tcg/translate.c
> > +++ b/target/i386/tcg/translate.c
> > @@ -877,7 +877,7 @@ static CCPrepare gen_prepare_sign_nz(TCGv src, MemOp size)
> >          return (CCPrepare) { .cond = TCG_COND_LT, .reg = src };
> >      } else {
> >          return (CCPrepare) { .cond = TCG_COND_TSTNE, .reg = src,
> > -                             .imm = 1ull << ((8 << size) - 1) };
> > +                             .imm = (1ull << (8 << size)) - 1 };
> >      }
> >  }  
>
diff mbox series

Patch

diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index ad1819815a..94f13541c3 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -877,7 +877,7 @@  static CCPrepare gen_prepare_sign_nz(TCGv src, MemOp size)
         return (CCPrepare) { .cond = TCG_COND_LT, .reg = src };
     } else {
         return (CCPrepare) { .cond = TCG_COND_TSTNE, .reg = src,
-                             .imm = 1ull << ((8 << size) - 1) };
+                             .imm = (1ull << (8 << size)) - 1 };
     }
 }