[Qemu-devel] [PATCH v3] sparc : 32bits integer division overflow

[Qemu-devel] [PATCH v3] sparc : 32bits integer division overflow

[Olivier Danet]

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) {
-- 
1.8.1.5

Re: [Qemu-devel] [PATCH v3] sparc : 32bits integer division overflow

[Mark Cave-Ayland]

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.

Re: [Qemu-devel] [PATCH v3] sparc : 32bits integer division overflow

[Richard Henderson]

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~

Re: [Qemu-devel] [PATCH v3] sparc : 32bits integer division overflow

[Richard Henderson]

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~

Re: [Qemu-devel] [PATCH v3] sparc : 32bits integer division overflow

[Mark Cave-Ayland]

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.

Re: [Qemu-devel] [PATCH v3] sparc : 32bits integer division overflow

[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.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3] sparc : 32bits integer division overflow

[Andreas Färber]

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v3] sparc : 32bits integer division overflow

[Mark Cave-Ayland]

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.

Re: [Qemu-devel] [PATCH v3] sparc : 32bits integer division overflow

[Andreas Färber]

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg