| Message ID | 532B94FF.4090809@caramail.com |
|---|---|
| State | New |
| Headers | show |
On 21/03/14 01:25, Olivier Danet wrote: > The signed integer division -0x8000_0000_0000_0000 / -1 must be handled > separately to avoid an overflow on the QEMU host. > > Negative overflow must be a negative number for correct sign > extension in Sparc64 mode. Use<stdint.h> constants. > > Signed-off-by: Olivier Danet<odanet@caramail.com> > --- > target-sparc/helper.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/target-sparc/helper.c b/target-sparc/helper.c > index f3c7fbf..ae7740b 100644 > --- a/target-sparc/helper.c > +++ b/target-sparc/helper.c > @@ -85,8 +85,8 @@ static target_ulong helper_udiv_common(CPUSPARCState *env, target_ulong a, > } > > x0 = x0 / x1; > - if (x0> 0xffffffff) { > - x0 = 0xffffffff; > + if (x0> UINT32_MAX) { > + x0 = UINT32_MAX; > overflow = 1; > } > > @@ -122,12 +122,15 @@ static target_ulong helper_sdiv_common(CPUSPARCState *env, target_ulong a, > if (x1 == 0) { > cpu_restore_state(CPU(cpu), GETPC()); > helper_raise_exception(env, TT_DIV_ZERO); > - } > - > - x0 = x0 / x1; > - if ((int32_t) x0 != x0) { > - x0 = x0< 0 ? 0x80000000 : 0x7fffffff; > + } else if (x1 == -1&& x0 == INT64_MIN) { > + x0 = INT32_MAX; > overflow = 1; > + } else { > + x0 = x0 / x1; > + if ((int32_t) x0 != x0) { > + x0 = x0< 0 ? INT32_MIN : INT32_MAX; > + overflow = 1; > + } > } > > if (cc) { Hi Olivier, This basic patch looks good to me. My only comment is that I suspect for bisection purposes it may be better to split this into 2 patches - one to perform the conversion of all existing constants to INT*_MAX and INT*_MIN, and then a second to add your change to prevent the crash. I'll let Richard have the final say though. Having said that, I will definitely give it a test over the next couple of days when I get a moment. ATB, Mark.
On 03/20/2014 06:25 PM, Olivier Danet wrote: > The signed integer division -0x8000_0000_0000_0000 / -1 must be handled > separately to avoid an overflow on the QEMU host. > > Negative overflow must be a negative number for correct sign > extension in Sparc64 mode. Use <stdint.h> constants. > > Signed-off-by: Olivier Danet <odanet@caramail.com> > --- > target-sparc/helper.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) Reviewed-by: Richard Henderson <rth@twiddle.net> r~
On 03/21/2014 12:07 AM, Mark Cave-Ayland wrote: > > This basic patch looks good to me. My only comment is that I suspect for > bisection purposes it may be better to split this into 2 patches - one to > perform the conversion of all existing constants to INT*_MAX and INT*_MIN, and > then a second to add your change to prevent the crash. I'll let Richard have > the final say though. I think one patch is fine. r~
On 21/03/14 01:25, Olivier Danet wrote: > The signed integer division -0x8000_0000_0000_0000 / -1 must be handled > separately to avoid an overflow on the QEMU host. > > Negative overflow must be a negative number for correct sign > extension in Sparc64 mode. Use<stdint.h> constants. > > Signed-off-by: Olivier Danet<odanet@caramail.com> > --- > target-sparc/helper.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/target-sparc/helper.c b/target-sparc/helper.c > index f3c7fbf..ae7740b 100644 > --- a/target-sparc/helper.c > +++ b/target-sparc/helper.c > @@ -85,8 +85,8 @@ static target_ulong helper_udiv_common(CPUSPARCState *env, target_ulong a, > } > > x0 = x0 / x1; > - if (x0> 0xffffffff) { > - x0 = 0xffffffff; > + if (x0> UINT32_MAX) { > + x0 = UINT32_MAX; > overflow = 1; > } > > @@ -122,12 +122,15 @@ static target_ulong helper_sdiv_common(CPUSPARCState *env, target_ulong a, > if (x1 == 0) { > cpu_restore_state(CPU(cpu), GETPC()); > helper_raise_exception(env, TT_DIV_ZERO); > - } > - > - x0 = x0 / x1; > - if ((int32_t) x0 != x0) { > - x0 = x0< 0 ? 0x80000000 : 0x7fffffff; > + } else if (x1 == -1&& x0 == INT64_MIN) { > + x0 = INT32_MAX; > overflow = 1; > + } else { > + x0 = x0 / x1; > + if ((int32_t) x0 != x0) { > + x0 = x0< 0 ? INT32_MIN : INT32_MAX; > + overflow = 1; > + } > } > > if (cc) { This patch fixes the original bug report, and doesn't appear to have any ill-effects on my SPARC32/SPARC64 image collection boot tests so: Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Peter - given that this prevents a guest from crashing the QEMU host, is it a candidate for 2.0? ATB, Mark.
On 24 March 2014 21:07, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > This patch fixes the original bug report, and doesn't appear to have any > ill-effects on my SPARC32/SPARC64 image collection boot tests so: > > Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > Peter - given that this prevents a guest from crashing the QEMU host, is it > a candidate for 2.0? Yes, I think so -- it's small and self-contained, it fixes a crash bug, and it's been reviewed. If you want to put it in a pull request I'll apply it. thanks -- PMM
Am 24.03.2014 22:36, schrieb Peter Maydell: > On 24 March 2014 21:07, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: >> This patch fixes the original bug report, and doesn't appear to have any >> ill-effects on my SPARC32/SPARC64 image collection boot tests so: >> >> Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> >> Peter - given that this prevents a guest from crashing the QEMU host, is it >> a candidate for 2.0? > > Yes, I think so -- it's small and self-contained, it fixes a crash bug, > and it's been reviewed. If you want to put it in a pull request I'll > apply it. Mark, please fix up the subject then. :) Andreas
On 24/03/14 22:43, Andreas Färber wrote: > Am 24.03.2014 22:36, schrieb Peter Maydell: >> On 24 March 2014 21:07, Mark Cave-Ayland<mark.cave-ayland@ilande.co.uk> wrote: >>> This patch fixes the original bug report, and doesn't appear to have any >>> ill-effects on my SPARC32/SPARC64 image collection boot tests so: >>> >>> Tested-by: Mark Cave-Ayland<mark.cave-ayland@ilande.co.uk> >>> >>> Peter - given that this prevents a guest from crashing the QEMU host, is it >>> a candidate for 2.0? >> >> Yes, I think so -- it's small and self-contained, it fixes a crash bug, >> and it's been reviewed. If you want to put it in a pull request I'll >> apply it. > > Mark, please fix up the subject then. :) Hi Andreas, Do you mean s/sparc/target-sparc/ ? My main observation was that I could also prefix the part after the colon with the word "fix". ATB, Mark.
Hi Mark, Am 26.03.2014 16:03, schrieb Mark Cave-Ayland: > On 24/03/14 22:43, Andreas Färber wrote: >> Am 24.03.2014 22:36, schrieb Peter Maydell: >>>> Peter - given that this prevents a guest from crashing the QEMU >>>> host, is it >>>> a candidate for 2.0? >>> >>> Yes, I think so -- it's small and self-contained, it fixes a crash bug, >>> and it's been reviewed. If you want to put it in a pull request I'll >>> apply it. >> >> Mark, please fix up the subject then. :) > > Hi Andreas, > > Do you mean s/sparc/target-sparc/ ? My main observation was that I could > also prefix the part after the colon with the word "fix". Both, and the space after the topic, i.e. s/sparc :/target-sparc:/. Cheers, Andreas
diff --git a/target-sparc/helper.c b/target-sparc/helper.c index f3c7fbf..ae7740b 100644 --- a/target-sparc/helper.c +++ b/target-sparc/helper.c @@ -85,8 +85,8 @@ static target_ulong helper_udiv_common(CPUSPARCState *env, target_ulong a, } x0 = x0 / x1; - if (x0 > 0xffffffff) { - x0 = 0xffffffff; + if (x0 > UINT32_MAX) { + x0 = UINT32_MAX; overflow = 1; } @@ -122,12 +122,15 @@ static target_ulong helper_sdiv_common(CPUSPARCState *env, target_ulong a, if (x1 == 0) { cpu_restore_state(CPU(cpu), GETPC()); helper_raise_exception(env, TT_DIV_ZERO); - } - - x0 = x0 / x1; - if ((int32_t) x0 != x0) { - x0 = x0 < 0 ? 0x80000000 : 0x7fffffff; + } else if (x1 == -1 && x0 == INT64_MIN) { + x0 = INT32_MAX; overflow = 1; + } else { + x0 = x0 / x1; + if ((int32_t) x0 != x0) { + x0 = x0 < 0 ? INT32_MIN : INT32_MAX; + overflow = 1; + } } if (cc) {
The signed integer division -0x8000_0000_0000_0000 / -1 must be handled separately to avoid an overflow on the QEMU host. Negative overflow must be a negative number for correct sign extension in Sparc64 mode. Use <stdint.h> constants. Signed-off-by: Olivier Danet <odanet@caramail.com> --- target-sparc/helper.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)