| Message ID | 20240628124258.832466-3-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | July maintainer updates (32bit, testing, plugins, gdbstub) | expand |
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 }; > } > }
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~
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~
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 --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 }; } }
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(-)